383181 - Prevent creating/overwriting HttpOnly cookies from web content

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Daniel Veditz [:dveditz]]

The added HttpOnly support (bug 178993) correctly prevents web content from reading cookies set by the server with the HttpOnly flag for parity with IE6. It is possible, however, for web content to _replace_ an HttpOnly cookie with a new non-HttpOnly value. Dave Wichers from OWASP says this leaves some web apps vulnerable and that and IE7 does block overwriting an existing HttpOnly cookie.

http://msdn2.microsoft.com/en-us/ie/aa740486.aspx#Functionality%20Removed%20in%20Internet%20Explorer%207

The OWASP WebGoat training software contains a testcase. I couldn't find an on-line example though, you have to download it and run your own local installation.

Comment 1

[Benjamin Smedberg]

Anatoly, is this something you can take?

Comment 2

[Mike Kaply [:mkaply]]

Attachment: Fix?

Would it be as simple as something like this?

Comment 3

[[e-mail]]

that would prevent even the webserver from modifying an httponly cookie to no longer be one. that may be mostly ok in practice, but it does seem a little drastic. does anyone know if IE does that?

by the way, that raises an interesting question. we not only need to block script from replacing an httponly cookie directly, but also from deleting it - otherwise it could simply delete and re-set it to achieve the same end.

Comment 4

[Daniel Veditz [:dveditz]]

I had a nearly identical patch I've been sitting on, and I hesitated for basically the reason you raise. But I think I'm OK with it. In practice a server isn't going to be replacing one with a non-httponly version, and if they have to they could always delete the httponly one first.

A bigger problem is that our code allows web scripts to create HttpOnly cookies in the first place, we only block reading them. A web script gets around this patch by adding "; httponly" to their cookie string -- bug most definitely not fixed.

The summary says "prevent overwriting", but really you want to prevent creating them from scratch, too.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 268509 [details] [diff] [review]
Fix?

unfortunately doesn't solve the problem, r-

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 268509 [details] [diff] [review]
Fix?

>   nsRefPtr<nsCookie> oldCookie;
>   if (foundCookie) {
>+    if (foundCookie->IsHttpOnly() && !aCookie->IsHttpOnly()) {

Does this even compile? foundCookie is a PRBool!

My version of the patch used oldCookie (after it was set).

Comment 7

[Robert Sayre]

(In reply to comment #4)
> I had a nearly identical patch I've been sitting on, and I hesitated for
> basically the reason you raise. But I think I'm OK with it. In practice a
> server isn't going to be replacing one with a non-httponly version,

If IE7 allows it, we should too.

(In reply to comment #6)
> Does this even compile? foundCookie is a PRBool!
> 
> My version of the patch used oldCookie (after it was set).

This shouldn't land without unit tests. 

Comment 8

[Mike Kaply [:mkaply]]

> Does this even compile? foundCookie is a PRBool!

Probably not. I was more asking the question if something like this was the right idea. I was mainly trying to get discussion going around this bug which worked :)

Comment 9

[[e-mail]]

so, what we want to do is prevent script from a) creating b) modifying and c) deleting httponly cookies. i think we'll have to pass the origin of the request (http or script) down into AddInternal and deal with it there.

Comment 10

[[e-mail]]

Attachment: something like this

dveditz, how does this sound?

(the cookie interfaces are a bit of a mess right now, because of the half-landed dom cookies stuff. i'm going to clean all that up soon, so ignore it for now.)

Comment 11

[Daniel Veditz [:dveditz]]

Looks good conceptually, but aren't those APIs part of nsICookieService? We can't change that on the 1.8 branch so we'd need something slightly different there

Comment 12

[[e-mail]]

sure, the patch i attached should work for branch, we'll have a slightly simpler one for trunk (less methods to modify).

Comment 13

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> This shouldn't land without unit tests. 

This should be easy enough to do with Mochitest's HTTP header support, I believe:

http://developer.mozilla.org/en/docs/Mochitest#How_do_I_change_the_HTTP_headers_or_status_sent_with_a_file_used_in_a_Mochitest.3F

You'll need to have several different files you load to check that the cookie is appropriately set and modified, because there's currently no way to dynamically generate headers (a bug, will eventually fix with JS CGI support in the server, but not in the immediate future).

If you have any questions on this, feel free to ask me and I can help as needed.

Since I'm just that nice a person, I note that the current posted patch does not have any tests and shouldn't land as-is due to this absence.  :-P

Comment 14

[Daniel Veditz [:dveditz]]

There are unit tests for basic HttpOnly support at
http://lxr.mozilla.org/mozilla/source/netwerk/test/TestCookie.cpp#646

They don't cover this new change but that's where to add them.

The second testcase is probably wrong: what do we do with web scripts that try to set HttpOnly cookies? Assuming they're not changing or deleting an already-existing one (which should fail) we could either
 - fail ("naughty, you're trying to trick us")
 - set cookie as non-httponly.

The testcase assumes the cookie gets set as httponly which this bug is going to change. IE7 appears to do the first, fail, and if we follow suit--which your patch does, and I agree we should--then the testcase will still pass, but for the wrong reason.

Comment 15

[Daniel Veditz [:dveditz]]

Attachment: http-only unit tests

Would these cover all the relevant cases?

Comment 16

[Daniel Veditz [:dveditz]]

Attachment: http-only unit tests v2

Ones that compile are better.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 268908 [details] [diff] [review]
something like this

> aren't those APIs part of nsICookieService? We can't change that
> on the 1.8 branch

I see, this is a second SetCookieString. Could you name it SetCookieStringCommon or similar instead to avoid confusion? Yeah, I know that's not C++, but we've already got SetCookieStringFromHttp and AddInternal so let's go with the "different names" flow.

Comment 18

[[e-mail]]

Comment on attachment 269682 [details] [diff] [review]
http-only unit tests v2

>+      // Non-Http cookies should not replace HttpOnly cookies
>+      SetACookie(cookieService, "http://httponly.test/", nsnull, "test=httponly; httponly", nsnull);
>+      SetACookieNoHttp(cookieService, "http://httponly.test/", "test=not-httponly ");

extraneous space (also below)

>+      GetACookie(cookieService, "http://httponly.test/", nsnull, getter_Copies(cookie));
>+      rv[3] = CheckResult(cookie.get(), MUST_EQUAL, "test=httponly");

for completeness we could also add a check here:

      GetACookieNoHttp(cookieService, "http://httponly.test/", getter_Copies(cookie));
      rv[x] = CheckResult(cookie.get(), MUST_NOT_CONTAIN, "test=not-httponly");

to make sure we get back nothing (i.e. not the non-http version of the cookie we tried to set). could use MUST_BE_NULL but would have to be careful the previous cookies we've set in this test don't cause trouble. (just use a different host if necessary)

also need to add a deletion test, since it'll exercise a different codepath in AddInternal:

      SetACookie(cookieService, "http://httponly.test/", nsnull, "test=httponly", nsnull);
      SetACookieNoHttp(cookieService, "http://httponly.test/", "test=httponly; max-age=-1");
      GetACookie(cookieService, "http://httponly.test/", nsnull, getter_Copies(cookie));
      rv[y] = CheckResult(cookie.get(), MUST_EQUAL, "test=httponly");
      SetACookie(cookieService, "http://httponly.test/", "test=httponly; max-age=-1");
      GetACookie(cookieService, "http://httponly.test/", nsnull, getter_Copies(cookie));
      rv[z] = CheckResult(cookie.get(), MUST_BE_NULL);

with similar care re host. r=me with those changes.

Comment 19

[[e-mail]]

Attachment: trunk patch v2

updated patch for trunk, with name change to avoid overloading.

dveditz, if you manage to get to this tomorrow (before freeze) i'll land it; and we can make sure there isn't any unexpected fallout in preparation for landing on branch.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 269803 [details] [diff] [review]
trunk patch v2

r/sr=dveditz

Comment 21

[[e-mail]]

Attachment: unit tests (checked in)

landed trunk patch v2. these are the unit tests i checked in.

Comment 22

[[e-mail]]

fixed on trunk

Comment 23

[[e-mail]]

Attachment: branch patch

identical patch to that used for trunk, but unrotted for 1.8 branch (where nsICookieServiceInternal never landed). requesting approval since we want this as a followup to bug 178993.

Comment 24

[[e-mail]]

in-testsuite+ on trunk.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 271561 [details] [diff] [review]
branch patch

approved for 1.8.1.5, a=dveditz

Found a bug in the patch, but since it's already checked into the trunk I filed it as bug 387543 rather than insist on a fix here. When you land on the branch you can combine them.

Comment 26

[[e-mail]]

checked in on 1.8 branch.

Comment 27

[Greg Supina]

After the most recent update of Firefox, a user name (in lessons on an educational web site) cannot be selected, the cookie cannot be deleted or modified, and the whole system is basically barred from the user's access.

The first time a user accesses the educational site, client-side JavaScript scripting allows the user to enter a name and data (like an alternative e-mail address) into a cookie record. The cookie then tracks the user's progress through the lessons.

When a user starts a new session, the cookie uses the record to return the user to the last lesson worked on during the previous session. It also reports on the user's progress, using data stored in the cookie, accumulated as the user progresses through the lessons. In addition to this, the user has the option of deleting an old record in the cookie, or modifying the personal data they enter into their cookie record.

Is there any solution to this problem, of allowing the user to control their own cookie record, and allowing them to access the lessons? This has worked for years, until this recent update?

Comment 28

[Daniel Veditz [:dveditz]]

Greg: Please file that as a separate bug, it's going to get lost buried down here. Since we changed this three versions ago it's not at all clear this change is the cause of your problems. If possible, in the new bug give a link to the broken site or perhaps a working test example if the site requires a login.

Does IE7 "work" on the site? Our behavior should match theirs with respect to HttpOnly cookies.

Comment 29

[Alexander Sack]

blocking 1.8.0.15 - see: https://bugzilla.mozilla.org/show_bug.cgi?id=178993#c150

Comment 30

[Christopher Aillon (sabbatical, not receiving bugmail)]

fixed with combined checkin in bug 178993