Ticket #7677 (closed defect: fixed)

Opened 3 months ago

Last modified 2 months ago

WordPress should implement HttpOnly Cookies to slow down XSS

Reported by: _ck_ Assigned to: anonymous
Priority: high Milestone: 2.7
Component: Security Version:
Severity: major Keywords: cookies dev-reviewed close-2.7
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

7677.diff (1.8 kB) - added by ryan on 09/03/08 16:28:51.

Change History

09/03/08 15:17:17 changed by _ck_

  • 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 );
}

09/03/08 16:23:14 changed by westi

  • keywords changed from cookies to cookies needs-patch.
  • 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.

09/03/08 16:28:51 changed by ryan

  • attachment 7677.diff added.

09/03/08 16:30:54 changed by ryan

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

09/03/08 16:31:36 changed by _ck_

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.

(follow-up: ↓ 7 ) 09/03/08 16:36:20 changed by 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.

09/03/08 16:41:47 changed by _ck_

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.

(in reply to: ↑ 5 ) 09/03/08 16:48:49 changed by westi

  • keywords changed from cookies needs-patch to cookies needs-patch dev-reviewed.

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

09/03/08 17:11:09 changed by _ck_

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

09/03/08 18:05:18 changed by ryan

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.

09/03/08 18:24:28 changed by ryan

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

09/04/08 07:43:51 changed by _ck_

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).

09/04/08 17:39:04 changed by ryan

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

09/04/08 19:38:13 changed by _ck_

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).

09/04/08 19:49:27 changed by ryan

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.

09/04/08 19:55:30 changed by ryan

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

09/05/08 00:14:26 changed by ryan

[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.

09/05/08 05:35:59 changed by ryan

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

09/05/08 05:36:53 changed by ryan

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

Extra stuff slipped into that commit. Ignore.

10/12/08 15:37:17 changed by jacobsantos

  • keywords changed from cookies needs-patch dev-reviewed to cookies dev-reviewed close-2.7.

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

10/14/08 21:03:47 changed by matt

  • status changed from new to closed.
  • resolution set to fixed.

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