Add a bunch of tests and checks for cookie domain security.

diff --git a/includes/HttpFunctions.php b/includes/HttpFunctions.php
index 2290271..907345a 100644 (file)
--- a/includes/HttpFunctions.php
+++ b/includes/HttpFunctions.php
@@ -453,23 +453,69 @@ class Cookie {
                        $this->path = "/";
                }
                if( isset( $attr['domain'] ) ) {
-                       $this->domain = self::parseCookieDomain( $attr['domain'] );
+                       if( self::validateCookieDomain( $attr['domain'] ) ) {
+                               $this->domain = $attr['domain'];
+                       }
                } else {
                        throw new MWException("You must specify a domain.");
                }
        }
 
-       public static function parseCookieDomain( $domain ) {
-               /* If domain is given, it has to contain at least two dots */
-               if ( strrpos( $domain, '.' ) === false
-                        || strrpos( $domain, '.' ) === strpos( $domain, '.' ) ) {
-                       return;
+       /**
+        * Return the true if the cookie is valid is valid.  Otherwise,
+        * false.  The uses a method similar to IE cookie security
+        * described here:
+        * http://kuza55.blogspot.com/2008/02/understanding-cookie-security.html
+        * A better method might be to use a blacklist like
+        * http://publicsuffix.org/
+        *
+        * @param $domain string the domain to validate
+        * @param $originDomain string (optional) the domain the cookie originates from
+        * @return bool
+        */
+       public static function validateCookieDomain( $domain, $originDomain = null) {
+               // Don't allow a trailing dot
+               if( substr( $domain, -1 ) == "." ) return false;
+
+               $dc = explode(".", $domain);
+
+               // Don't allow cookies for "localhost", "ls" or other dot-less hosts
+               if( count($dc) < 2 ) return false;
+
+               // Only allow full, valid IP addresses
+               if( preg_match( '/^[0-9.]+$/', $domain ) ) {
+                       if( count( $dc ) != 4 ) return false;
+
+                       if( ip2long( $domain ) === false ) return false;
+
+                       if( $originDomain == null || $originDomain == $domain ) return true;
+
                }
-               if ( substr( $domain, 0, 1 ) === '.' ) {
-                       $domain = substr( $domain, 1 );
+
+               // Don't allow cookies for "co.uk" or "gov.uk", etc, but allow "supermarket.uk"
+               if( strrpos( $domain, "." ) - strlen( $domain )  == -3 ) {
+                       if( (count($dc) == 2 && strlen( $dc[0] ) <= 2 )
+                               || (count($dc) == 3 && strlen( $dc[0] ) == "" && strlen( $dc[1] ) <= 2 ) ) {
+                               return false;
+                       }
+                       if( (count($dc) == 2 || (count($dc) == 3 && $dc[0] == "") )
+                               && preg_match( '/(com|net|org|gov|edu)\...$/', $domain) ) {
+                               return false;
+                       }
+               }
+
+               if( $originDomain != null ) {
+                       if( substr( $domain, 0, 1 ) != "." && $domain != $originDomain ) {
+                               return false;
+                       }
+                       if( substr( $domain, 0, 1 ) == "."
+                               && substr_compare( $originDomain, $domain, -strlen( $domain ),
+                                                                  strlen( $domain ), TRUE ) != 0 ) {
+                               return false;
+                       }
                }
 
-               return $domain;
+               return true;
        }
 
        /**
@@ -491,8 +537,10 @@ class Cookie {
        }
 
        protected function canServeDomain( $domain ) {
-               if( $this->domain && substr_compare( $domain, $this->domain, -strlen( $this->domain ),
-                                                                                        strlen( $this->domain ), TRUE ) == 0 ) {
+               if( $domain == $this->domain
+                       || ( substr( $this->domain, 0, 1) == "."
+                                && substr_compare( $domain, $this->domain, -strlen( $this->domain ),
+                                                                       strlen( $this->domain ), TRUE ) == 0 ) ) {
                        return true;
                }
                return false;
@@ -572,13 +620,10 @@ class CookieJar {
 
                        if( !isset( $attr['domain'] ) ) {
                                $attr['domain'] = $domain;
-                       } else {
-                               if ( strlen( $attr['domain'] ) < strlen( $domain )
-                                        && substr_compare( $domain, $attr['domain'], -strlen( $attr['domain'] ),
-                                                                               strlen( $attr['domain'] ), TRUE ) != 0 ) {
-                                       return; /* silently reject a bad cookie */
-                               }
+                       } elseif ( !Cookie::validateCookieDomain( $attr['domain'], $domain ) ) {
+                               return null;
                        }
+
                        $this->setCookie( $name, $value, $attr );
                }
        }

diff --git a/maintenance/tests/HttpTest.php b/maintenance/tests/HttpTest.php
index a27dd7b..158d454 100644 (file)
--- a/maintenance/tests/HttpTest.php
+++ b/maintenance/tests/HttpTest.php
@@ -346,15 +346,56 @@ class HttpTest extends PhpUnit_Framework_TestCase {
        function testIsValidUrl() {
        }
 
-       function testSetCookie() {
+       function testValidateCookieDomain() {
+               $this->assertFalse( Cookie::validateCookieDomain( "co.uk" ) );
+               $this->assertFalse( Cookie::validateCookieDomain( ".co.uk" ) );
+               $this->assertFalse( Cookie::validateCookieDomain( "gov.uk" ) );
+               $this->assertFalse( Cookie::validateCookieDomain( ".gov.uk" ) );
+               $this->assertTrue( Cookie::validateCookieDomain( "supermarket.uk" ) );
+               $this->assertFalse( Cookie::validateCookieDomain( "uk" ) );
+               $this->assertFalse( Cookie::validateCookieDomain( ".uk" ) );
+               $this->assertFalse( Cookie::validateCookieDomain( "127.0.0." ) );
+               $this->assertFalse( Cookie::validateCookieDomain( "127." ) );
+               $this->assertFalse( Cookie::validateCookieDomain( "127.0.0.1." ) );
+               $this->assertTrue( Cookie::validateCookieDomain( "127.0.0.1" ) );
+               $this->assertFalse( Cookie::validateCookieDomain( "333.0.0.1" ) );
+               $this->assertTrue( Cookie::validateCookieDomain( "example.com" ) );
+               $this->assertFalse( Cookie::validateCookieDomain( "example.com." ) );
+               $this->assertTrue( Cookie::validateCookieDomain( ".example.com" ) );
+
+               $this->assertTrue( Cookie::validateCookieDomain( ".example.com", "www.example.com" ) );
+               $this->assertFalse( Cookie::validateCookieDomain( "example.com", "www.example.com" ) );
+               $this->assertTrue( Cookie::validateCookieDomain( "127.0.0.1", "127.0.0.1" ) );
+               $this->assertFalse( Cookie::validateCookieDomain( "127.0.0.1", "localhost" ) );
+
+
+       }
+
+       function testSetCooke() {
                $c = new MockCookie( "name", "value",
                                                         array(
-                                                                "domain" => ".example.com",
+                                                                "domain" => "ac.th",
+                                                                "path" => "/path/",
+                                                        ) );
+               $this->assertFalse($c->canServeDomain("ac.th"));
+
+               $c = new MockCookie( "name", "value",
+                                                        array(
+                                                                "domain" => "example.com",
                                                                 "path" => "/path/",
                                                         ) );
 
                $this->assertTrue($c->canServeDomain("example.com"));
+               $this->assertFalse($c->canServeDomain("www.example.com"));
+
+               $c = new MockCookie( "name", "value",
+                                                        array(
+                                                                "domain" => ".example.com",
+                                                                "path" => "/path/",
+                                                        ) );
+
                $this->assertFalse($c->canServeDomain("www.example.net"));
+               $this->assertFalse($c->canServeDomain("example.com"));
                $this->assertTrue($c->canServeDomain("www.example.com"));
 
                $this->assertFalse($c->canServePath("/"));
@@ -368,6 +409,15 @@ class HttpTest extends PhpUnit_Framework_TestCase {
                $this->assertEquals("", $c->serializeToHttpRequest("/", "www.example.com"));
                $this->assertEquals("name=value", $c->serializeToHttpRequest("/path/", "www.example.com"));
 
+               $c = new MockCookie( "name", "value",
+                                                        array(
+                                                                "domain" => "www.example.com",
+                                                                "path" => "/path/",
+                                                        ) );
+               $this->assertFalse($c->canServeDomain("example.com"));
+               $this->assertFalse($c->canServeDomain("www.example.net"));
+               $this->assertTrue($c->canServeDomain("www.example.com"));
+
                $c = new MockCookie( "name", "value",
                                                 array(
                                                         "domain" => ".example.com",