1164567 - Various consumers in the tree use nsIPrincipal off-main-thread

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

nsIPrincipal doesn't have threadsafe refcounting, but it also rolls its own refcounting scheme, so it doesn't presently have any ownerthread assertions either. I just added some in bug 1164292, and hit the assertion in various places.

This can lead to UAF/double-free, though we'll have to see exactly how exploitable the culprits are.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Grab the principal when we need it in MediaDecodeTask. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Proxy-release mPrincipal in websockets. v1

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8605389 [details] [diff] [review]
Part 1 - Grab the principal when we need it in MediaDecodeTask. v1

Why the intermediate QI to nsPIDOMWindow?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #3)
> Comment on attachment 8605389 [details] [diff] [review]
> Part 1 - Grab the principal when we need it in MediaDecodeTask. v1
> 
> Why the intermediate QI to nsPIDOMWindow?

Just moving the existing code. I could fix it I guess.

Comment 5

[Bobby Holley (:bholley)]

Green try push with some other stuff:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55ad99bcd0b5

Comment 6

[Andrea Marchesini [:baku]]

Attachment: Part 2 - Get rid of mPrincipal from WebSocket

Sorry bholley but actually we can get rid of mPrincipal completely.
This is cleaner because we don't have to depend on the quite complex logic of mChannel.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini (:baku) from comment #6)
> Created attachment 8605802 [details] [diff] [review]
> Part 2 - Get rid of mPrincipal from WebSocket
> 
> Sorry bholley but actually we can get rid of mPrincipal completely.
> This is cleaner because we don't have to depend on the quite complex logic
> of mChannel.

Is this patch going to be safe enough to backport? If so, it's probably preferable, because it's less clear what the problem is. But if not, we should move it to a followup bug, since we'll want something backportable here.

Comment 8

[Andrea Marchesini [:baku]]

I think it's safe enough. In the end, instead storing a nsIPrincipal in WebSocketImpl, we take it all the time as a parameter.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8605802 [details] [diff] [review]
Part 2 - Get rid of mPrincipal from WebSocket

>-class InitRunnable final : public WorkerMainThreadRunnable
>+class MainThreadRunnable : public WorkerMainThreadRunnable
Too generic name, IMO.
Perhaps WebSocketMainThreadRunnable?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Bobby Holley (:bholley) from comment #0)
> nsIPrincipal doesn't have threadsafe refcounting, but it also rolls its own
> refcounting scheme
I wonder how high % of unusual AddRef/Release implementations have ended up causing security bugs :/
Something to add to reviewers' check list: be very careful with manual AddRef/Release implementations.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8605802 [details] [diff] [review]
Part 2 - Get rid of mPrincipal from WebSocket

Requesting sec-approval on both patches.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not super easily. The security problem isn't _immediately_ obvious (though not too much of a stretch), but exploiting it requires triggering the race condition in just the right way, and _then_ exploiting the ensuing memory hazard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

See above. I think baku's patch for part two makes it significantly less obvious that this is a refcounting problem.

Which older supported branches are affected by this flaw?

All, as far as I know.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Should hopefully be straightforward.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely to cause regressions, but this bug blocks other time-sensitive stuff (though I _could_ work around it if I really needed to). I think we should get this in the tree. I can land it along with some of my subsequent work in bug 1164292 to make it less obvious.

Comment 12

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

I'll approve this to land with bug 1164292. Do you have a suggestion as to a rating? Sec-high?

Comment 13

[Bobby Holley (:bholley)]

[:abillings] from comment #12)
> I'll approve this to land with bug 1164292. Do you have a suggestion as to a
> rating? Sec-high?

sec-high sounds right.

(In reply to Olli Pettay [:smaug] from comment #9)
> >-class InitRunnable final : public WorkerMainThreadRunnable
> >+class MainThreadRunnable : public WorkerMainThreadRunnable
> Too generic name, IMO.
> Perhaps WebSocketMainThreadRunnable?

baku - I'll apply this review comment and take care of pushing these patches.(In reply to Al Billings

Comment 14

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

sec-approval+ and marking for branches.

Comment 15

[Andrea Marchesini [:baku]]

> baku - I'll apply this review comment and take care of pushing these
> patches.(In reply to Al Billings

Ok! thanks.

Comment 16

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=6e940b4892ee

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4a1b5563fdcc
https://hg.mozilla.org/mozilla-central/rev/415a89038ea1

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8605802 [details] [diff] [review]
Part 2 - Get rid of mPrincipal from WebSocket

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: security 
Fix Landed on Version: 41
Risk to taking this patch (and alternatives if risky): Low risk 
String or UUID changes made by this patch: None

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8605802 [details] [diff] [review]
Part 2 - Get rid of mPrincipal from WebSocket

[Triage Comment]
Taking it in 38.0.5 too.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b84a7fded1d
https://hg.mozilla.org/releases/mozilla-aurora/rev/27775cefea29

We're taking security fixes in 38.0.5?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f191235f22ef
https://hg.mozilla.org/releases/mozilla-beta/rev/34ad06cb05a0

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/cee3ab8147e0
https://hg.mozilla.org/releases/mozilla-esr38/rev/8e123bd0a98e

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8605802 [details] [diff] [review]
Part 2 - Get rid of mPrincipal from WebSocket

This needs rebasing for b2g37.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/6b84a7fded1d
> https://hg.mozilla.org/releases/mozilla-aurora/rev/27775cefea29
> 
> We're taking security fixes in 38.0.5?
It is a sec high, it landed everywhere. However, I didn't realize that the patch was that big.
Bobby, can it wait for 39?

Comment 25

[Andrea Marchesini [:baku]]

Attachment: patch 2 for b2g37

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8605802 [details] [diff] [review]
Part 2 - Get rid of mPrincipal from WebSocket

wontfix for 38.0.5 per IRC discussion w/ Sylvestre

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Part 2 only affects 35+ (feature landed in bug 504553).

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e5df85ba6839
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/08fb23c0597f

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ddb03c402946

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/ddb03c402946

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/b62060031bc2

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/b62060031bc2

https://hg.mozilla.org/releases/mozilla-esr31/rev/9235e36083fb

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Of course, I'm assuming that the older release branches don't have other occurrences of their own besides those were present on trunk. Not sure how good of an assumption that is.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Bobby had me run a Try push of bug 1164292 part 1 applied to b2g32, and none of the test suites that were expected to pass had any issues.

Comment 30

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #29)
> Bobby had me run a Try push of bug 1164292 part 1 applied to b2g32, and none
> of the test suites that were expected to pass had any issues.

Thanks Ryan!

Comment 31

[Andrei Vaida [:avaida]]

Bobby, is there anything manual QA could do to help verify this fix?

Comment 32

[Bobby Holley (:bholley)]

(In reply to Andrei Vaida, QA [:avaida] from comment #31)
> Bobby, is there anything manual QA could do to help verify this fix?

Nope.