Opened 16 years ago
Closed 16 years ago
#7677 closed defect (bug) (fixed)
WordPress should implement HttpOnly Cookies to slow down XSS
| Reported by: |
|
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)
Change History (21)
#2
@
16 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.
#3
@
16 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
@
16 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:
↓ 7
@
16 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
@
16 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
@
16 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
@
16 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
@
16 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.
#11
@
16 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
@
16 years ago
Okay, how about a patch that adds ';HttpOnly' for the < 5.2 case that maintains the $secure argument?
#13
@
16 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
@
16 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.
#16
@
16 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.
#18
@
16 years ago
The problem was with HTTPOnly being appended when the cookie domain was empty.
Extra stuff slipped into that commit. Ignore.
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 ); }