455598 - nsICookieManager2.cookieExists incorrectly assumes a nsCookie param

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[(no longer active)]

Attachment: Patch (v1) [Checkin: Comment 5]

The implementation of nsICookieManager2.cookieExists incorrectly uses static_cast to cast the nsICookie2 parameter to an nsCookie pointer, and this results in a crash when called from JavaScript (and it can possibly lead to other crashes, if some C++ code decides to implements its own nsICookie2 and pass it to cookieExists.

For an example of the crash, see bug 248970 comment 257.  This patch makes the implementation not dependent on nsCookie.

Comment 1

[Shawn Wilsher :sdwilsh]

I imagine the fast path exists for a reason here, so you might want to keep it with a private interface to QI to.

Comment 2

[(no longer active)]

(In reply to comment #1)
> I imagine the fast path exists for a reason here, so you might want to keep it
> with a private interface to QI to.

We don't seem to be using this method anywhere in the tree <http://mxr.mozilla.org/mozilla-central/ident?i=cookieExists&tree=mozilla-central&filter=&scriptidly=1>.  Do you still think that the fast path exists intentionally?

Comment 3

[(no longer active)]

This can affect extensions authors, both on trunk and 1.9.0 branch, so I'm requesting blocking on both.

Comment 4

[[e-mail]]

Comment on attachment 338957 [details] [diff] [review]
Patch (v1)
[Checkin: Comment 5]

it is used, here: http://mxr.mozilla.org/mozilla/source/extensions/cookie/nsCookiePermission.cpp#368

the fast path isn't that important here, so r=me. hitting up biesi for sr to cut bz some slack...

thanks for the patch!

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 338957 [details] [diff] [review]
Patch (v1)
[Checkin: Comment 5]

http://hg.mozilla.org/mozilla-central/rev/fe79a762d6fe

Comment 6

[(no longer active)]

Thanks for the check-in, Serge!

I was going to write a crash test for this as well, but unfortunately the reftest framework seems to be limited to non-privileged content (or I don't know how to work around it) and testing this requires privileged code.  Is there any other way to test this?

I'll post a patch for the branch soon.

Comment 7

[(no longer active)]

Attachment: 1.9.0 branch patch

I'm not quite sure whether the branch patch needs its own s/sr (it's nearly identical to the trunk patch), but I'm requesting them anyway to be on the safe side. :-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 339062 [details] [diff] [review]
1.9.0 branch patch

Isn't this exactly identical?

Comment 9

[(no longer active)]

Yes... s/near/exact/ :-)

Comment 10

[[e-mail]]

Comment on attachment 339062 [details] [diff] [review]
1.9.0 branch patch

extra points!

Comment 11

[(no longer active)]

Comment on attachment 339062 [details] [diff] [review]
1.9.0 branch patch

This bug can cause crashes if an extension uses this public cookie manager API.  The fix is simple and well-contained, and low-risk.  Nominating for approval to land on 1.9.0.3.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #6)
> I was going to write a crash test for this as well, but unfortunately the
> reftest framework seems to be limited to non-privileged content (or I don't
> know how to work around it) and testing this requires privileged code.  Is
> there any other way to test this?

Martijn, do you have an answer to Ehsan's question?

Comment 13

[Martijn Wargers (dead)]

You could use the mochitest framework, instead: http://developer.mozilla.org/en/Mochitest

Comment 14

[(no longer active)]

(In reply to comment #13)
> You could use the mochitest framework, instead:
> http://developer.mozilla.org/en/Mochitest

Can it perform crash tests?  In other words, what happens when the test fails?  Will the whole mochitest session crash?  Is this detectable and does it provide useful output on the Tinderbox?

Comment 15

[Martijn Wargers (dead)]

If it crashes, the test has failed. The whole mochitest session will stop at that point. Afaik, tinderbox reports this. If you want to know more, perhaps you should ask on irc in #developers. They know more about this stuff than I do.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Probably it could look similar to all the other crash tests, e.g. http://mxr.mozilla.org/mozilla-central/source/layout/tables/crashtests/404301-1.xhtml

Comment 17

[(no longer active)]

(In reply to comment #16)
> Probably it could look similar to all the other crash tests, e.g.
> http://mxr.mozilla.org/mozilla-central/source/layout/tables/crashtests/404301-1.xhtml

I would have liked it as well, but it's not possible.  Such crash tests are also loaded by the reftest framework, and it's not possible to get UniversalXPConnect privileges in reftests, which is needed in order to reproduce the crash in this bug.  Bug 448676 tracks enabling UniversalXPConnect in the reftests (which would happen other reftests as well), so I'm marking it as a blocker here.

Regarding Martijn's suggestion in comment 15, I'm going to need to ask in #developers to see if a Mochitest would be feasible, but I don't remember seeing a Mochitest for testing a crash...

Worst case, I can create an extension to use this API in js and produce a crash, and we can add a Litmus test case to install that extension and verifying that the browser doesn't crash, but that wouldn't be so elegant... :-/

CCing sayrer to see if he can help here.

Comment 18

[Robert Sayre]

Can you write an xpcshell test?

Comment 19

[(no longer active)]

(In reply to comment #18)
> Can you write an xpcshell test?

Yes, sure.  Will crash for an xpcshell test be correctly reported by the Tinderbox as the failure of that test case?

Comment 20

[Robert Sayre]

Yes

Comment 21

[(no longer active)]

(In reply to comment #20)
> Yes

That's good.  Test forthcoming.

Comment 22

[(no longer active)]

Attachment: Test [Checkin: Comment 25]

Test for both trunk and 1.9.0 branch.

Comment 23

[(no longer active)]

Checkin needed for attachment 339198 [details] [diff] [review].

Comment 24

[(no longer active)]

Comment on attachment 339198 [details] [diff] [review]
Test
[Checkin: Comment 25]

This is the unit test for the main patch.  This should land on branch together with the main patch (attachment 339062 [details] [diff] [review]).

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 339198 [details] [diff] [review]
Test
[Checkin: Comment 25]

http://hg.mozilla.org/mozilla-central/rev/8e60ce0be3e6

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 339062 [details] [diff] [review]
1.9.0 branch patch

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 27

[(no longer active)]

Landed on CVS trunk (1.9 branch):

Checking in netwerk/cookie/src/nsCookieService.cpp;
/cvsroot/mozilla/netwerk/cookie/src/nsCookieService.cpp,v  <--  nsCookieService.cpp
new revision: 1.95; previous revision: 1.94
done
RCS file: /cvsroot/mozilla/netwerk/test/unit/test_bug455598.js,v
done
Checking in netwerk/test/unit/test_bug455598.js;
/cvsroot/mozilla/netwerk/test/unit/test_bug455598.js,v  <--  test_bug455598.js
initial revision: 1.1
done

Comment 28

[[e-mail]]

thanks ehsan!

Comment 29

[Al Billings [:abillings - ex-MoCo]]

I assume that the unit test is passing for 1.9?

Comment 30

[(no longer active)]

(In reply to comment #29)
> I assume that the unit test is passing for 1.9?

Yes, why?

See <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.0/1224785686.1224788396.19521.gz> for example...

Comment 31

[Al Billings [:abillings - ex-MoCo]]

I'm tasked with verifying the fix but there are no manual steps, just the unit test. I wanted to be doubly sure things were good before marking this as verified for 1.9.0.4. I will do so now since I can see it passing in the log file.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Test passes on Linux, Windows, and OS X. Marking as verified.