SMF: Security vs. Functionality

Started by elfishtroll, December 29, 2007, 04:48:03 PM

Previous topic - Next topic

elfishtroll

Quote from: Dannii on December 31, 2007, 01:43:54 AM
...
You\\\\\\\'re using a different meaning of \\\\\\\'security\\\\\\\' than everyone else. Community management systems could definitely be improved, and Captcha for guest posting would be great, but they\\\\\\\'re not security issues.

Quote(ok, these damned slashes are pissing me off! lol\\\\\\\\\\\\\\\")
Then don\\\\\\\'t use the WYSIWYG editor.
[/quote]
! I\\\'m not!..I\\\'m not even using Javascript enabled! ???


Actually, I dont think I am using \\\'a different meaning of security\\\' than everyone else!  (it may be argued in rebuttal that only the FIRST THREE WORDS of my sentence should be taken seriously - but I digress*)

Think of your forum as a little walled Medieval village (for all you Dungeons and Dragons/Lord of the Rings fans)  or even as a Police State (for all you Republicans :P)

you will recognize that you have PERIMETER SECURITY for drive by attacks
- SQL flooding by via abusing queries,
-Untrusting ALL input, reg Ex sanitization, SQL query escaping

AND INTERNAL SECURITY
Where you secure your Forum from those who would attack it from within.
These considerations include functions such as:

IP tracking
post tracking
post moderation and Post DELAY (like time delay for live broadcasts) I allow kids to post and guests too, but their posts are AUTOMATICALLY DELAYED a set number of minutes or unless approved by a moderator.

Limiting retroactive editing of posts (which pisses me off no end to go back to a thread and see it butchered with edits)

PM behavior monitoring (to  detect PM spam bots)

Login captha

*Post captha (selectable by group,post count)
This can be turned on to act as a small measure of post moderation, moving a member from the \\\"TRUSTED GROUP\\\" to the \\\"UNTRUSTED GROUP.. Prove you are not a robot/butthead!


@GRUDGE, the IP spoofing issue most definitely HAS NOT BEEN FIXED as of 1.14
plz see /sources/QueryString.php (lines 245..331 approx)
Quote
example:
      // Otherwise just use the only one.
      elseif (preg_match(\\\\\\\'~^((0|10|172\\\\\\\\.16|192\\\\\\\\.168|255|127\\\\\\\\.0)\\\\\\\\.|unknown)~\\\\\\\', $_SERVER[\\\\\\\'HTTP_X_FORWARDED_FOR\\\\\\\']) == 0 || preg_match(\\\\\\\'~^((0|10|172\\\\\\\\.16|192\\\\\\\\.168|255|127\\\\\\\\.0)\\\\\\\\.|unknown)~\\\\\\\', $_SERVER[\\\\\\\'REMOTE_ADDR\\\\\\\']) != 0)
         $_SERVER[\\\\\\\'REMOTE_ADDR\\\\\\\'] = $_SERVER[\\\\\\\'HTTP_X_FORWARDED_FOR\\\\\\\']; this can never be right :(
In the above code extract, you will see that if the USER SUPPLIED  FORWARDED variable resolves to something that looks like an IP address, IT REPLACES THE \\\"true\\\" IP ADDRESS!

worse. it overwrites the variables that your programs are expected to use! so if YOU do any IP tracking of your own, you will be tracking the contaminated variable also. :(





*digress v. what you do to food in your tummy :)

elfishtroll

P.S. the Who Online function shows the SPOOFED variable as the user IP, for example

metallica48423

#22
Theres more to the code than just that.  It is also a conditional statement -- that code isn't always executed.


// Store the REMOTE_ADDR for later - even though we HOPE to never use it...
    $_SERVER['BAN_CHECK_IP'] = isset($_SERVER['REMOTE_ADDR']) && preg_match('~^((([1]?\d)?\d|2[0-4]\d|25[0-5])\.){3}(([1]?\d)?\d|2[0-4]\d|25[0-5])$~', $_SERVER['REMOTE_ADDR']) === 1 ? $_SERVER['REMOTE_ADDR'] : 'unknown';

    // Find the user's IP address. (but don't let it give you 'unknown'!)
    if (!empty($_SERVER['HTTP_X_FORWARDED_FOR']) && !empty($_SERVER['HTTP_CLIENT_IP']) && (preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['HTTP_CLIENT_IP']) == 0 || preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['REMOTE_ADDR']) != 0))
    {
        // We have both forwarded for AND client IP... check the first forwarded for as the block - only switch if it's better that way.
        if (strtok($_SERVER['HTTP_X_FORWARDED_FOR'], '.') != strtok($_SERVER['HTTP_CLIENT_IP'], '.') && '.' . strtok($_SERVER['HTTP_X_FORWARDED_FOR'], '.') == strrchr($_SERVER['HTTP_CLIENT_IP'], '.') && (preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['HTTP_X_FORWARDED_FOR']) == 0 || preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['REMOTE_ADDR']) != 0))
            $_SERVER['REMOTE_ADDR'] = implode('.', array_reverse(explode('.', $_SERVER['HTTP_CLIENT_IP'])));
        else
            $_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_CLIENT_IP'];
    }
    if (!empty($_SERVER['HTTP_CLIENT_IP']) && (preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['HTTP_CLIENT_IP']) == 0 || preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['REMOTE_ADDR']) != 0))
    {
        // Since they are in different blocks, it's probably reversed.
        if (strtok($_SERVER['REMOTE_ADDR'], '.') != strtok($_SERVER['HTTP_CLIENT_IP'], '.'))
            $_SERVER['REMOTE_ADDR'] = implode('.', array_reverse(explode('.', $_SERVER['HTTP_CLIENT_IP'])));
        else
            $_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_CLIENT_IP'];
    }
    elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
    {
        // If there are commas, get the last one.. probably.
        if (strpos($_SERVER['HTTP_X_FORWARDED_FOR'], ',') !== false)
        {
            $ips = array_reverse(explode(', ', $_SERVER['HTTP_X_FORWARDED_FOR']));

            // Go through each IP...
            foreach ($ips as $i => $ip)
            {
                // Make sure it's in a valid range...
                if (preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $ip) != 0 && preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['REMOTE_ADDR']) == 0)
                    continue;

                // Otherwise, we've got an IP!
                $_SERVER['REMOTE_ADDR'] = trim($ip);
                break;
            }
        }
        // Otherwise just use the only one.
        elseif (preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['HTTP_X_FORWARDED_FOR']) == 0 || preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['REMOTE_ADDR']) != 0)
            $_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_FORWARDED_FOR'];
    }


essentially, it only uses it as a last resort.  Its not just overwriting it with X_FORWARDED_FOR (which i agree would indeed be an issue ;))

I'll have to have grudge or one of the devs explain it more thoroughly though, as i am out for a meeting at the moment and am short on time.

Thanks for the example though, its good for us to review things and ensure they are as intended :)

We aren't a police state, we just expect a mutual respect in interacting with one side or the other... criticism is one thing, trolling is another.  I wanted to ban you, as a kneejerk reaction,  but didn't because for one, that would be pretty nazi-ish, and secondly, i feel we simply have a misunderstanding of intentions here.  Point of the matter is, respect is given where it is received and vice versa.  Thats just, in my view, how it should be. 

I feel that more transparency is needed as to development to a point, which is one thing thats been brought up for discussion in our meetings here in AZ.  I feel that there is a difference in direction between three groups

1.) Developers
2.) Team Members
3.) Community Members

I think each group expects something different out of things, and i think that only the devs really know what direction they truely want to go in. 

I think i'll bring that up during the meeting, but for now, i'm off

edit: i split this out of the current topic to keep it on topic :)
Justin O'Leary
Ex-Project Manager
Ex-Lead Support Specialist

QuoteMicrosoft wants us to "Imagine life without walls"...
I say, "If there are no walls, who needs Windows?"


Useful Links:
Online Manual!
How to Help us Help you
Search
Settings Repair Tool

Grudge

Actually elfishtroll is correct. I forgot that we discussed using REMOTE_ADDR everywhere and concluded that on balance that was not necessarily in the interest of administrators. At the time of 1.1 we came to the conclusion that:
1) In general X_FORWARDED_FOR is a more accurate IP address prediction as it helps protect against the issue of one proxy IP covering thousands of users.
2) It can however be spoofed so should not be relied upon for true security purposes.

However, we concluded that an absolute minority of banned users would be likely to attempt to spoof their IP address through such a means (Compared to, for example, using an anoymous proxy) and it was actually more likely that a user would be banned using a proxy address that would inadvertently ban innocent users too. Therefore we decided that we would use *both* IP addresses for any action that involved banning (i.e. both appear on profile, managebans and in the ban checker) but that for normal display purposes (Who's online, Display etc) we would use the X_FORWARDED_FOR information as that was most likely to be the accurate IP in the vast majority of cases.

However, whilst this is what we did in 1.1 in 2.0 we decided that we did not want to overwrite the REMOTE_ADDR variable (For integrations etc) and so would use REMOTE_ADDR for all purposes, using X_FORWARDED_FOR only as an additional ban check. For information here is the code from SMF 2.0 Beta 1:

// Make sure we have a valid REMOTE_ADDR.
if (!isset($_SERVER['REMOTE_ADDR']))
{
$_SERVER['REMOTE_ADDR'] = '';
// A new magic variable to indicate we think this is command line.
$_SERVER['is_cli'] = true;
}
elseif (preg_match('~^((([1]?\d)?\d|2[0-4]\d|25[0-5])\.){3}(([1]?\d)?\d|2[0-4]\d|25[0-5])$~', $_SERVER['REMOTE_ADDR']) === 0)
$_SERVER['REMOTE_ADDR'] = 'unknown';

// Try to calculate their most likely IP for those people behind proxies (And the like).
$_SERVER['BAN_CHECK_IP'] = $_SERVER['REMOTE_ADDR'];

// Find the user's IP address. (but don't let it give you 'unknown'!)
if (!empty($_SERVER['HTTP_X_FORWARDED_FOR']) && !empty($_SERVER['HTTP_CLIENT_IP']) && (preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['HTTP_CLIENT_IP']) == 0 || preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['REMOTE_ADDR']) != 0))
{
// We have both forwarded for AND client IP... check the first forwarded for as the block - only switch if it's better that way.
if (strtok($_SERVER['HTTP_X_FORWARDED_FOR'], '.') != strtok($_SERVER['HTTP_CLIENT_IP'], '.') && '.' . strtok($_SERVER['HTTP_X_FORWARDED_FOR'], '.') == strrchr($_SERVER['HTTP_CLIENT_IP'], '.') && (preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['HTTP_X_FORWARDED_FOR']) == 0 || preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['REMOTE_ADDR']) != 0))
$_SERVER['BAN_CHECK_IP'] = implode('.', array_reverse(explode('.', $_SERVER['HTTP_CLIENT_IP'])));
else
$_SERVER['BAN_CHECK_IP'] = $_SERVER['HTTP_CLIENT_IP'];
}
if (!empty($_SERVER['HTTP_CLIENT_IP']) && (preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['HTTP_CLIENT_IP']) == 0 || preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['REMOTE_ADDR']) != 0))
{
// Since they are in different blocks, it's probably reversed.
if (strtok($_SERVER['REMOTE_ADDR'], '.') != strtok($_SERVER['HTTP_CLIENT_IP'], '.'))
$_SERVER['BAN_CHECK_IP'] = implode('.', array_reverse(explode('.', $_SERVER['HTTP_CLIENT_IP'])));
else
$_SERVER['BAN_CHECK_IP'] = $_SERVER['HTTP_CLIENT_IP'];
}
elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
{
// If there are commas, get the last one.. probably.
if (strpos($_SERVER['HTTP_X_FORWARDED_FOR'], ',') !== false)
{
$ips = array_reverse(explode(', ', $_SERVER['HTTP_X_FORWARDED_FOR']));

// Go through each IP...
foreach ($ips as $i => $ip)
{
// Make sure it's in a valid range...
if (preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $ip) != 0 && preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['REMOTE_ADDR']) == 0)
continue;

// Otherwise, we've got an IP!
$_SERVER['BAN_CHECK_IP'] = trim($ip);
break;
}
}
// Otherwise just use the only one.
elseif (preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['HTTP_X_FORWARDED_FOR']) == 0 || preg_match('~^((0|10|172\.16|192\.168|255|127\.0)\.|unknown)~', $_SERVER['REMOTE_ADDR']) != 0)
$_SERVER['BAN_CHECK_IP'] = $_SERVER['HTTP_X_FORWARDED_FOR'];
}


We didn't implement this in 1.1 as:
1) It's a lot of code changes for an incremental minor release, and:
2) What we implemented in the original change provides all the security required for the purposes of banning etc, so was more a "nice to have" rather than a "must have".

Grudge
I'm only a half geek really...

metallica48423

Ahh, thats right, i misread the code entirely.  I see now what you mean after looking at it again from another point of view.  I pulled the code from the SVN viewer (my bandwidth is incredibly slow here, and didn't want to wait forever to download 1.1.4 for one file :P) so i didn't have the comparison. 

I guess it really is a trade off in either case.  I mean you have the potential of spoofing vs. the potential of proxying.

But in reality, if someone wants to hide their real IP address theres not much we can do to truely catch the real IP. 

Thanks for the insight (i actually find things like that very interesting)
Justin O'Leary
Ex-Project Manager
Ex-Lead Support Specialist

QuoteMicrosoft wants us to "Imagine life without walls"...
I say, "If there are no walls, who needs Windows?"


Useful Links:
Online Manual!
How to Help us Help you
Search
Settings Repair Tool

Advertisement: