Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-9802

Optimize session_begin REMOTE_ADDR validation

    Details

    • Type: Improvement
    • Status: Unverified Fix
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.7-PL1
    • Fix Version/s: 3.0.9-RC1
    • Component/s: Sessions
    • Labels:
      None
    • Environment:
      PHP Version 5.2.13, MySQL(i) 5.0.77

      Description

      Detailed info (including demo code, results, and execution time tests) in this thread: http://area51.phpbb.com/phpBB/viewtopic.php?f=3&t=33891&p=213615#p213603

      I'm setting priority to Major as I suspect that issue #5, with proper understanding, could be exploited in an attack (which might be combined with other vectors) to cause a Denial-of-Service, or at least resource starvation...

      Issue #1

      the regex done on both $this->ip and $this->forwarded_for is redundant and adds a considerable delay

      $this->ip = preg_replace('#[ ]{2,}#', ' ', str_replace(array(',', ' '), ' ', $this->ip));
      

      (Took: 0.046015 ms)

      Modified regex:

      $this->ip = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $this->ip));
      

      (Took: 0.015974 ms)
      (And same results in all scenarios)

      Issue #2

      Additional code, which is seldom needed, is executed every time. That is, REMOTE_ADDR rarely contains spaces or commas (signifying multiple addresses), so this overhead could be avoided by using strpos in an if statement to first check if $this->ip contains a comma or a space.

      Assuming REMOTE_ADDR contains standard ipv4 IP address, say '1.2.3.4':

      Without strpos:
      Took: 0.030994 ms

      Modified:
      Took: 0.007868 ms

      if (strpos($this->ip, ',') !== FALSE || strpos($this->ip, ' ') !== FALSE)
      {
          $this->ip = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $this->ip));
       
          // split the list of IPs
          $ips = explode(' ', $this->ip);
       
          // Default IP if REMOTE_ADDR is invalid
          $this->ip = '127.0.0.1';
       
          foreach ($ips as $ip)
          {
              // check IPv4 first, the IPv6 is hopefully only going to be used very seldomly
              if (!empty($ip) && !preg_match(get_preg_expression('ipv4'), $ip) && !preg_match(get_preg_expression('ipv6'), $ip))
              {
                  // Just break
                  break;
              }
       
              // Use the last in chain
              $this->ip = $ip;
          }
      }
      else
      {
          if (empty($this->ip) || (!preg_match(get_preg_expression('ipv4'), $this->ip) && !preg_match(get_preg_expression('ipv6'), $this->ip)))
          {
              $this->ip = '127.0.0.1';
          }
      }
      

      Issue #3

      A logic issue in the foreach loop above: the way it is structured, if $ip is empty, $this->ip will get set to an empty string.

      This could be avoided by checking:
      if(empty($ip)) continue;
      else if (!ipv4 && !ipv6) break;

      or even better, use trim() before exploding, and get rid of the empty() check in the loop:

      // split the list of IPs
      $ips = explode(' ', trim($this->ip));
       
      // Default IP if REMOTE_ADDR is invalid
      $this->ip = '127.0.0.1';
       
      foreach ($ips as $ip)
      {
          if (!preg_match(get_preg_expression('ipv4'), $ip) && !preg_match(get_preg_expression('ipv6'), $ip))
          {
              break;
          }
              $this->ip = $ip;    // Use the last in chain
      }
      

      Issue #4

      htmlspecialchars is unneeded, as $this->ip will be set to '127.0.0.1' if the IP does not validate as ipv4 or ipv6 addr, and neither can be valid and yet contain html special chars (ampersand, single/double quote, less/greater than).

      $this->ip = (!empty($_SERVER['REMOTE_ADDR'])) ? htmlspecialchars((string) $_SERVER['REMOTE_ADDR']) : '';
      

      same applies to $this->forwarded_for:

      $this->forwarded_for        = (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) ? htmlspecialchars((string) $_SERVER['HTTP_X_FORWARDED_FOR']) : '';
      

      which is later double htmlspecialchar'ed:

      add_log('critical', 'LOG_IP_BROWSER_FORWARDED_CHECK', $u_ip, $s_ip, $u_browser, $s_browser, htmlspecialchars($u_forwarded_for), htmlspecialchars($s_forwarded_for));
      

      Issue #5

      For 'forwarded_for_check', we should not loop through all the IPs in the HTTP_X_FORWARDED_FOR header (if there are more than one), because the possibly real address is the first IP in the $ips array ( $ips[0] ), the rest (if there are any) are most likely chained proxies, and we shouldn't really care about those. Matter of fact, we should really ignore them, to prevent possible abuse... Keep in mind that HTTP_X_FORWARDED_FOR header can be forged by the user-agent! (think DoS)..

      So this modified snippet would make more sense (and less work):

      // if the forwarded for header shall be checked we have to validate its contents
      if ($config['forwarded_for_check'] && strpos($this->forwarded_for, ',') !== FALSE || strpos($this->forwarded_for, ' ') !== FALSE)
      {
      	$this->forwarded_for = preg_replace('# {2,}#', ' ', str_replace(',', ' ', $this->forwarded_for));
       
      	// split the list of IPs
      	$ips = explode(' ', trim($this->forwarded_for));
       
      	// Possibly real address is the first IP in the array. That's all we need.
      	// The rest are most-likely chained proxy, and we should just ignore them,
      	// rather than loop through the list, to prevent possible abuse.
      	// (Yes, HTTP_X_FORWARDED_FOR header can be forged by the user-agent!)
      	if (!preg_match(get_preg_expression('ipv4'), $ips[0]) && !preg_match(get_preg_expression('ipv6'), $ips[0]))
      	{
      		// contains invalid data, don't use the forwarded for header
      		$this->forwarded_for = '';
      	}
      	else
      	{
      		$this->forwarded_for = $ips[0];
      	}
      }
      else
      {
      	$this->forwarded_for = '';
      }
      

      Hope that helps..

      1. test.php
        1 kB
        jasmineaura [X]
      2. test2.php
        3 kB
        jasmineaura [X]

        Activity

        Hide
        jasmineaura jasmineaura [X] (Inactive) added a comment -

        Demo-code for comparing default session_begin() behavior vs. modified code, per issues 1 through 4 in attached files; test.php and test2.php
        test2.php (more comprehensive edits) needs phpbb3 in place to include common.php and be able to use get_preg_expression() (from includes/functions.php)

        test.php example output:

        phpBB:

        1.2.3.4
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5 1.2.3.6
        1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.7
        1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.7

        Took: 0.053167 ms

        MOD:

        1.2.3.4
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5
        1.2.3.4 1.2.3.5 1.2.3.6
        1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.7
        1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.7

        Took: 0.021935 ms

        Output after trim():

        ..shows print_r($ips_ary_trimmed) ..
        (same as above, except that leading and ending spaces are removed, so no empty array elements)

        test2.php example output:

        REMOTE_ADDR w/ one valid IP (no commas or spaces)

        "1.2.3.4"

        phpBB:

        "1.2.3.4"

        Took: 0.030041 ms

        MOD:

        "1.2.3.4"

        Took: 0.008106 ms

        More than one IP, variable seperator

        "1.2.3.4,1.2.3.5, 1.2.3.6 1.2.3.7"

        phpBB:

        "1.2.3.7"

        Took: 0.025988 ms

        MOD:

        "1.2.3.7"

        Took: 0.023127 ms

        Same as above + extraneous leading/ending spaces

        " 1.2.3.4,1.2.3.5, 1.2.3.6 1.2.3.7 "

        phpBB:

        ""

        Took: 0.025034 ms

        MOD:

        "1.2.3.7"

        Took: 0.023127 ms

        Show
        jasmineaura jasmineaura [X] (Inactive) added a comment - Demo-code for comparing default session_begin() behavior vs. modified code, per issues 1 through 4 in attached files; test.php and test2.php test2.php (more comprehensive edits) needs phpbb3 in place to include common.php and be able to use get_preg_expression() (from includes/functions.php) test.php example output: phpBB: 1.2.3.4 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.7 1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.7 Took: 0.053167 ms MOD: 1.2.3.4 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.7 1.2.3.4 1.2.3.5 1.2.3.6 1.2.3.7 Took: 0.021935 ms Output after trim(): ..shows print_r($ips_ary_trimmed) .. (same as above, except that leading and ending spaces are removed, so no empty array elements) test2.php example output: REMOTE_ADDR w/ one valid IP (no commas or spaces) "1.2.3.4" phpBB: "1.2.3.4" Took: 0.030041 ms MOD: "1.2.3.4" Took: 0.008106 ms More than one IP, variable seperator "1.2.3.4,1.2.3.5, 1.2.3.6 1.2.3.7" phpBB: "1.2.3.7" Took: 0.025988 ms MOD: "1.2.3.7" Took: 0.023127 ms Same as above + extraneous leading/ending spaces " 1.2.3.4,1.2.3.5, 1.2.3.6 1.2.3.7 " phpBB: "" Took: 0.025034 ms MOD: "1.2.3.7" Took: 0.023127 ms
        Hide
        naderman Nils Adermann added a comment -

        The speed improvements look good however a few of the choices were made intentionally. As you note yourself X_FORWARDED_FOR can easily be forged. As such we intentionally validate all IPs because we do not want to even consider an X_FORWARDED_FOR that is not perfectly formed. The header cannot possibly be made so long that it would be usable for a DOS because the webserver would rejct the request first. The cases where htmlspecialchars is used on strings which should really be valid IPs are really defensive cautious coding where we do not trust our own code. However it could probably be removed.

        Show
        naderman Nils Adermann added a comment - The speed improvements look good however a few of the choices were made intentionally. As you note yourself X_FORWARDED_FOR can easily be forged. As such we intentionally validate all IPs because we do not want to even consider an X_FORWARDED_FOR that is not perfectly formed. The header cannot possibly be made so long that it would be usable for a DOS because the webserver would rejct the request first. The cases where htmlspecialchars is used on strings which should really be valid IPs are really defensive cautious coding where we do not trust our own code. However it could probably be removed.
        Hide
        bantu Andreas Fischer added a comment - - edited

        The htmlspecialchars() calls are there because the logic that checks for valid IP has been added later on.
        Edit: Sure, calling htmlspecialchars() more than once on the same string is a bug.

        Show
        bantu Andreas Fischer added a comment - - edited The htmlspecialchars() calls are there because the logic that checks for valid IP has been added later on. Edit: Sure, calling htmlspecialchars() more than once on the same string is a bug.
        Hide
        jasmineaura jasmineaura [X] (Inactive) added a comment - - edited

        @Nils

        Understandable. Please note however that I said "possible attack vector" and also between parenthesis "which might be combined with other vectors".

        Although most (sane) web servers would limit each header to about 8KB (and up to 100 headers in each request), some others, like IIS, allow up to 16KB per header.

        Please note that lines 341-342 trim $this->data['session_forwarded_for'] and $this->forwarded_for to 255 chars. So why bother possibly validating 8KB filled with (forged IPs) if we'll later only really look at the first 255 bytes?

        To simplify, in 255 bytes, I could send 32 IP addresses, comma seperated. That is: a.b.c.d (7 chars)
        32 * 7 = 224 chars
        31 commas + 224 chars = 255 bytes

        Imagine, with such 255 bytes, causing the foreach loop @ lines 228-237 to call preg_match(get_preg_expression()) 32 times
        Edit: and that is after explode creating an array, whose number of elements are up to the number of comma-separated IPs in the forwarded header. Furthermore, foreach works on a copy of the array, rather than the array (which had just been created).

        Ex.
        1.0.0.1, .. 1.0.0.9, 1.1.0.1, ... 1.1.0.9 .. etc..
        up to 900 IPs just in the range 1.0.0.1 through 1.9.9.9

        And with up to 8KB header limit, I can easily fit 900 IPs
        900 IPs * 7 chars = 6300 bytes
        899 commas + 6300 = 7199 bytes (under 8KB)

        I don't think this really needs a PoC. A simple (threaded) script, given enough bandwidth, could make quite a noticeable effect on server's resources (or availability).

        And if DEBUG_EXTRA is on, perhaps the attacker could even change the IP at the last few bytes in the 255byte range of the HTTP_X_FORWARDED_FOR header, and maybe emulate a bot in the HTTP_USER_AGENT to bypass the second condition of the if statement (line 437):
        $this->data['user_id'] != ANONYMOUS
        and flood the log as well (plus 2 extra htmlspecialchars calls, on something that was already previously htmlspecialchar'd, and preg_match ip validated)...

        Which reminds me, shouldn't that rather be && $this->data['is_registered'] ?

        Show
        jasmineaura jasmineaura [X] (Inactive) added a comment - - edited @Nils Understandable. Please note however that I said "possible attack vector" and also between parenthesis "which might be combined with other vectors". Although most (sane) web servers would limit each header to about 8KB (and up to 100 headers in each request), some others, like IIS, allow up to 16KB per header. Please note that lines 341-342 trim $this->data ['session_forwarded_for'] and $this->forwarded_for to 255 chars. So why bother possibly validating 8KB filled with (forged IPs) if we'll later only really look at the first 255 bytes? To simplify, in 255 bytes, I could send 32 IP addresses, comma seperated. That is: a.b.c.d (7 chars) 32 * 7 = 224 chars 31 commas + 224 chars = 255 bytes Imagine, with such 255 bytes, causing the foreach loop @ lines 228-237 to call preg_match(get_preg_expression()) 32 times Edit: and that is after explode creating an array, whose number of elements are up to the number of comma-separated IPs in the forwarded header. Furthermore, foreach works on a copy of the array, rather than the array (which had just been created). Ex. 1.0.0.1, .. 1.0.0.9, 1.1.0.1, ... 1.1.0.9 .. etc.. up to 900 IPs just in the range 1.0.0.1 through 1.9.9.9 And with up to 8KB header limit, I can easily fit 900 IPs 900 IPs * 7 chars = 6300 bytes 899 commas + 6300 = 7199 bytes (under 8KB) I don't think this really needs a PoC. A simple (threaded) script, given enough bandwidth, could make quite a noticeable effect on server's resources (or availability). And if DEBUG_EXTRA is on, perhaps the attacker could even change the IP at the last few bytes in the 255byte range of the HTTP_X_FORWARDED_FOR header, and maybe emulate a bot in the HTTP_USER_AGENT to bypass the second condition of the if statement (line 437): $this->data ['user_id'] != ANONYMOUS and flood the log as well (plus 2 extra htmlspecialchars calls, on something that was already previously htmlspecialchar'd, and preg_match ip validated)... Which reminds me, shouldn't that rather be && $this->data ['is_registered'] ?
        Hide
        Oleg Oleg [X] (Inactive) added a comment - - edited

        I could not find any problems in the diff, however I see two possible improvements.

        One, if there is a concern that a malicious user would supply huge ip addresses, they can be length-checked before feeding them to pcre.

        Two, instead of doing a regexp replacement of spaces, then another replacement of commas, and then exploding, do a regexp split.

        Lastly there are no tests here. The code doing ip validation should be split into a separate function for 3.1 with tests added for it.

        Show
        Oleg Oleg [X] (Inactive) added a comment - - edited I could not find any problems in the diff, however I see two possible improvements. One, if there is a concern that a malicious user would supply huge ip addresses, they can be length-checked before feeding them to pcre. Two, instead of doing a regexp replacement of spaces, then another replacement of commas, and then exploding, do a regexp split. Lastly there are no tests here. The code doing ip validation should be split into a separate function for 3.1 with tests added for it.
        Hide
        naderman Nils Adermann added a comment -

        Refactoring and tests should come as a part of the 3.1 refactoring.

        Show
        naderman Nils Adermann added a comment - Refactoring and tests should come as a part of the 3.1 refactoring.

          People

          • Assignee:
            bantu Andreas Fischer
            Reporter:
            jasmineaura jasmineaura [X] (Inactive)
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development