WordPress.org

Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#7677 closed defect (bug) (fixed)

WordPress should implement HttpOnly Cookies to slow down XSS

Reported by: _ck_ Owned by:
Milestone: 2.7 Priority: high
Severity: major Version:
Component: Security Keywords: cookies dev-reviewed close-2.7
Focuses: Cc:

Description

While it's far from perfect and there are complex ways around it, HttpOnly Cookies are supported now by all major browsers and will prevent many kinds of XSS attacks.

HttpOnly Cookies simply prevent cookies from being accessed via javascript's document.cookie so an admin's WP cookie cannot be easily forwarded to another domain via injected javascript.

I need to do more research but it should be fairly easy to implement. I'll suggest this for bbPress and BackPress too.

Attachments (1)

7677.diff (1.8 KB) - added by ryan 13 years ago.

Download all attachments as: .zip

Change History (21)

#1 @_ck_
13 years ago

  • Component changed from General to Security
  • Severity changed from normal to major

It's this freaking simple. Should be added ASAP:

if (PHP_VERSION < 5.2) {
@setcookie( $name, $value, $expires, $path, $domain. '; HttpOnly' );
} else {
@setcookie( $name, $value, $expires, $path, $domain, NULL, TRUE );
}

#2 @westi
13 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.6.2 to 2.7

I think it is not just that simple as document.cookie is used in some of the WordPress js at the moment from what I see from a quick grep in the code.

We need to review which of those are reading rather than writing cookies and see if they need the auth/login/ssl cookies or not before we do this.

2.7 is a more reasonable target for this change.

@ryan
13 years ago

#3 @ryan
13 years ago

westi, I think AJAX requests can still send httponly cookies, but they shouldn't be allowed to read them. Firefox allows reading them though: https://bugzilla.mozilla.org/show_bug.cgi?id=380418

#4 @_ck_
13 years ago

On the bbPress side, data is passed to the javascript client via var's set in the <head></head> section by PHP. That way only the critical data like the user id and user name are passed instead of relying on the cookie. WordPress should definitely use that technique too.

#5 follow-up: @ryan
13 years ago

I think all of the document.cookie references in our JS are for cookies other than the auth cookies. Those cookies won't be httponly.

#6 @_ck_
13 years ago

I should point out the only currently known way to hack around HttpOnly is to trick the user to pass through a proxy to capture the cookie header directly from the server (in which case the user has a much bigger problem than just their wordpress login). So this is a really helpful lockdown.

#7 in reply to: ↑ 5 @westi
13 years ago

  • Keywords dev-reviewed added

Replying to ryan:

I think all of the document.cookie references in our JS are for cookies other than the auth cookies. Those cookies won't be httponly.

Cool

+1 to patch

#8 @_ck_
13 years ago

On that patch - you're not going to use the workaround for PHP less than 5.2 ? There are tens of thousands of servers still using 4.8/4.9

#9 @ryan
13 years ago

The patch to make it work for < 5.2 is kinda ghetto. Since this is a defense-in-depth security addition and not essential, I think requiring 5.2 is okay, especially since those concerned with security will be moving to PHP 5 now that PHP 4 is eol.

#10 @ryan
13 years ago

(In [8798]) Try out httponly for auth cookies. see #7677

#11 @_ck_
13 years ago

The 'ghetto' technique you are referring to (simply appending the flag onto the domain) is what PHP 5.2 does internally anyway. The PHP setcookie function does not modify or filter the domain string, it's passed directly to the browser. In theory, you shouldn't even have to use the 5.2 "TRUE" flag method and could use the domain append method for all versions (though I have not tested that notion).

If it's left not dealing with PHP 4.x I'll just have release a plugin to replace the pluggable function for PHP 4.x users. There are far too many who cannot control what version of PHP their shared host uses. But the sad part is that most of those users won't know to go looking for the plugin so they'll miss out on the extra security and just blame WordPress instead of their PHP/host when they get hacked.

I'm just saying WordPress could use as few complaints about security as possible - if it's this easy to help, why not do all we can (ounce of prevention and all that).

#12 @ryan
13 years ago

Okay, how about a patch that adds ';HttpOnly' for the < 5.2 case that maintains the $secure argument?

#13 @_ck_
13 years ago

I'm not sure if you are asking me or if you are saying it won't work?

The $secure argument is just telling PHP to only send the cookie if the connection should be SSL (https).

It does not interfere with the domain and therefore should not be affected by the HttpOnly hack?

Based on your patch it should be as simple as:

 } else { 
	                setcookie($auth_cookie_name, $auth_cookie, $expire, PLUGINS_COOKIE_PATH, COOKIE_DOMAIN.'; HttpOnly', $secure); 
	                setcookie($auth_cookie_name, $auth_cookie, $expire, ADMIN_COOKIE_PATH, COOKIE_DOMAIN.'; HttpOnly', $secure); 
	                setcookie(LOGGED_IN_COOKIE, $logged_in_cookie, $expire, COOKIEPATH, COOKIE_DOMAIN.'; HttpOnly'); 
	                if ( COOKIEPATH != SITECOOKIEPATH ) 
	                        setcookie(LOGGED_IN_COOKIE, $logged_in_cookie, $expire, SITECOOKIEPATH, COOKIE_DOMAIN.'; HttpOnly');  
	        }


ps. I have a survey of 4000 bbPress sites and one out of three of them are still running PHP 4.3 or 4.4. I suspect WordPress will have similar stats (Matt probably knows exactly from the phone home data during the upgrade check).

#14 @ryan
13 years ago

I'm just talking out loud while waiting for a spare minute to actually do some coding. :-)

Last I checked, the WP phone home stats indicated a similar ratio.

#15 @ryan
13 years ago

(In [8808]) HttpOnly for PHP < 5.2. Props _ck_. see #7677

#16 @ryan
13 years ago

[8810] removes HTTPOnly for Safari. We had some weird problems with Safari not setting the cookie when testing on wordpress.com. Looks like it was only when setting cookies via remote login, so it might not be something a regular WP blog will see. Disabling for now while I track it down.

#17 @ryan
13 years ago

(In [8811]) Don't append HTTPOnly if cookie domain is empty. see #7677

#18 @ryan
13 years ago

The problem was with HTTPOnly being appended when the cookie domain was empty.

Extra stuff slipped into that commit. Ignore.

#19 @jacobsantos
13 years ago

  • Keywords close-2.7 added; needs-patch removed

This ticket probably needs to be closed when 2.7 is complete.

#20 @matt
13 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Appears to be working well - closing ticket. Open new ticket on 2.7 if you find any bugs with implementation.

Note: See TracTickets for help on using tickets.