r62118 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r62117‎ | r62118 | r62119 >
Date:05:38, 8 February 2010
Author:mah
Status:deferred
Tags:
Comment:
Add a bunch of tests and checks for cookie domain security.
Modified paths:
  • /trunk/phase3/includes/HttpFunctions.php (modified) (history)
  • /trunk/phase3/maintenance/tests/HttpTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/tests/HttpTest.php
——@@ -346,15 +346,56 @@
347347 function testIsValidUrl() {
348348 }
349349
350 - function testSetCookie() {
 350+ function testValidateCookieDomain() {
 351+ $this->assertFalse( Cookie::validateCookieDomain( "co.uk" ) );
 352+ $this->assertFalse( Cookie::validateCookieDomain( ".co.uk" ) );
 353+ $this->assertFalse( Cookie::validateCookieDomain( "gov.uk" ) );
 354+ $this->assertFalse( Cookie::validateCookieDomain( ".gov.uk" ) );
 355+ $this->assertTrue( Cookie::validateCookieDomain( "supermarket.uk" ) );
 356+ $this->assertFalse( Cookie::validateCookieDomain( "uk" ) );
 357+ $this->assertFalse( Cookie::validateCookieDomain( ".uk" ) );
 358+ $this->assertFalse( Cookie::validateCookieDomain( "127.0.0." ) );
 359+ $this->assertFalse( Cookie::validateCookieDomain( "127." ) );
 360+ $this->assertFalse( Cookie::validateCookieDomain( "127.0.0.1." ) );
 361+ $this->assertTrue( Cookie::validateCookieDomain( "127.0.0.1" ) );
 362+ $this->assertFalse( Cookie::validateCookieDomain( "333.0.0.1" ) );
 363+ $this->assertTrue( Cookie::validateCookieDomain( "example.com" ) );
 364+ $this->assertFalse( Cookie::validateCookieDomain( "example.com." ) );
 365+ $this->assertTrue( Cookie::validateCookieDomain( ".example.com" ) );
 366+
 367+ $this->assertTrue( Cookie::validateCookieDomain( ".example.com", "www.example.com" ) );
 368+ $this->assertFalse( Cookie::validateCookieDomain( "example.com", "www.example.com" ) );
 369+ $this->assertTrue( Cookie::validateCookieDomain( "127.0.0.1", "127.0.0.1" ) );
 370+ $this->assertFalse( Cookie::validateCookieDomain( "127.0.0.1", "localhost" ) );
 371+
 372+
 373+ }
 374+
 375+ function testSetCooke() {
351376 $c = new MockCookie( "name", "value",
352377 array(
 378+ "domain" => "ac.th",
 379+ "path" => "/path/",
 380+ ) );
 381+ $this->assertFalse($c->canServeDomain("ac.th"));
 382+
 383+ $c = new MockCookie( "name", "value",
 384+ array(
 385+ "domain" => "example.com",
 386+ "path" => "/path/",
 387+ ) );
 388+
 389+ $this->assertTrue($c->canServeDomain("example.com"));
 390+ $this->assertFalse($c->canServeDomain("www.example.com"));
 391+
 392+ $c = new MockCookie( "name", "value",
 393+ array(
353394 "domain" => ".example.com",
354395 "path" => "/path/",
355396 ) );
356397
357 - $this->assertTrue($c->canServeDomain("example.com"));
358398 $this->assertFalse($c->canServeDomain("www.example.net"));
 399+ $this->assertFalse($c->canServeDomain("example.com"));
359400 $this->assertTrue($c->canServeDomain("www.example.com"));
360401
361402 $this->assertFalse($c->canServePath("/"));
——@@ -369,6 +410,15 @@
370411 $this->assertEquals("name=value", $c->serializeToHttpRequest("/path/", "www.example.com"));
371412
372413 $c = new MockCookie( "name", "value",
 414+ array(
 415+ "domain" => "www.example.com",
 416+ "path" => "/path/",
 417+ ) );
 418+ $this->assertFalse($c->canServeDomain("example.com"));
 419+ $this->assertFalse($c->canServeDomain("www.example.net"));
 420+ $this->assertTrue($c->canServeDomain("www.example.com"));
 421+
 422+ $c = new MockCookie( "name", "value",
373423 array(
374424 "domain" => ".example.com",
375425 "path" => "/path/",
Index: trunk/phase3/includes/HttpFunctions.php
——@@ -453,23 +453,69 @@
454454 $this->path = "/";
455455 }
456456 if( isset( $attr['domain'] ) ) {
457 - $this->domain = self::parseCookieDomain( $attr['domain'] );
 457+ if( self::validateCookieDomain( $attr['domain'] ) ) {
 458+ $this->domain = $attr['domain'];
 459+ }
458460 } else {
459461 throw new MWException("You must specify a domain.");
460462 }
461463 }
462464
463 - public static function parseCookieDomain( $domain ) {
464 - /* If domain is given, it has to contain at least two dots */
465 - if ( strrpos( $domain, '.' ) === false
466 - || strrpos( $domain, '.' ) === strpos( $domain, '.' ) ) {
467 - return;
 465+ /**
 466+ * Return the true if the cookie is valid is valid. Otherwise,
 467+ * false. The uses a method similar to IE cookie security
 468+ * described here:
 469+ * http://kuza55.blogspot.com/2008/02/understanding-cookie-security.html
 470+ * A better method might be to use a blacklist like
 471+ * http://publicsuffix.org/
 472+ *
 473+ * @param $domain string the domain to validate
 474+ * @param $originDomain string (optional) the domain the cookie originates from
 475+ * @return bool
 476+ */
 477+ public static function validateCookieDomain( $domain, $originDomain = null) {
 478+ // Don't allow a trailing dot
 479+ if( substr( $domain, -1 ) == "." ) return false;
 480+
 481+ $dc = explode(".", $domain);
 482+
 483+ // Don't allow cookies for "localhost", "ls" or other dot-less hosts
 484+ if( count($dc) < 2 ) return false;
 485+
 486+ // Only allow full, valid IP addresses
 487+ if( preg_match( '/^[0-9.]+$/', $domain ) ) {
 488+ if( count( $dc ) != 4 ) return false;
 489+
 490+ if( ip2long( $domain ) === false ) return false;
 491+
 492+ if( $originDomain == null || $originDomain == $domain ) return true;
 493+
468494 }
469 - if ( substr( $domain, 0, 1 ) === '.' ) {
470 - $domain = substr( $domain, 1 );
 495+
 496+ // Don't allow cookies for "co.uk" or "gov.uk", etc, but allow "supermarket.uk"
 497+ if( strrpos( $domain, "." ) - strlen( $domain ) == -3 ) {
 498+ if( (count($dc) == 2 && strlen( $dc[0] ) <= 2 )
 499+ || (count($dc) == 3 && strlen( $dc[0] ) == "" && strlen( $dc[1] ) <= 2 ) ) {
 500+ return false;
 501+ }
 502+ if( (count($dc) == 2 || (count($dc) == 3 && $dc[0] == "") )
 503+ && preg_match( '/(com|net|org|gov|edu)\...$/', $domain) ) {
 504+ return false;
 505+ }
471506 }
472507
473 - return $domain;
 508+ if( $originDomain != null ) {
 509+ if( substr( $domain, 0, 1 ) != "." && $domain != $originDomain ) {
 510+ return false;
 511+ }
 512+ if( substr( $domain, 0, 1 ) == "."
 513+ && substr_compare( $originDomain, $domain, -strlen( $domain ),
 514+ strlen( $domain ), TRUE ) != 0 ) {
 515+ return false;
 516+ }
 517+ }
 518+
 519+ return true;
474520 }
475521
476522 /**
——@@ -491,8 +537,10 @@
492538 }
493539
494540 protected function canServeDomain( $domain ) {
495 - if( $this->domain && substr_compare( $domain, $this->domain, -strlen( $this->domain ),
496 - strlen( $this->domain ), TRUE ) == 0 ) {
 541+ if( $domain == $this->domain
 542+ || ( substr( $this->domain, 0, 1) == "."
 543+ && substr_compare( $domain, $this->domain, -strlen( $this->domain ),
 544+ strlen( $this->domain ), TRUE ) == 0 ) ) {
497545 return true;
498546 }
499547 return false;
——@@ -572,13 +620,10 @@
573621
574622 if( !isset( $attr['domain'] ) ) {
575623 $attr['domain'] = $domain;
576 - } else {
577 - if ( strlen( $attr['domain'] ) < strlen( $domain )
578 - && substr_compare( $domain, $attr['domain'], -strlen( $attr['domain'] ),
579 - strlen( $attr['domain'] ), TRUE ) != 0 ) {
580 - return; /* silently reject a bad cookie */
581 - }
 624+ } elseif ( !Cookie::validateCookieDomain( $attr['domain'], $domain ) ) {
 625+ return null;
582626 }
 627+
583628 $this->setCookie( $name, $value, $attr );
584629 }
585630 }

Status & tagging log