From a126ce6f4873cba428f184131097e23541aa69b4 Mon Sep 17 00:00:00 2001 From: "Mark A. Hershberger" Date: Mon, 8 Feb 2010 05:38:09 +0000 Subject: [PATCH] Add a bunch of tests and checks for cookie domain security. --- includes/HttpFunctions.php | 79 ++++++++++++++++++++++++++-------- maintenance/tests/HttpTest.php | 54 ++++++++++++++++++++++- 2 files changed, 114 insertions(+), 19 deletions(-) diff --git a/includes/HttpFunctions.php b/includes/HttpFunctions.php index 229027184d..907345a82a 100644 --- 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 a27dd7b694..158d454663 100644 --- 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", -- 2.20.1