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

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({csectype-race, sec-high})

unspecified
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox38 wontfix, firefox39+ fixed, firefox38.0.5 wontfix, firefox40+ fixed, firefox41+ fixed, firefox-esr3139+ fixed, firefox-esr3839+ fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main39+][adv-esr31.8+][adv-esr38.1+])

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

4 years ago
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.
Assignee

Comment 2

4 years ago
Attachment #8605391 - Flags: review?(amarchesini)
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?
Assignee

Comment 4

4 years ago
(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.
Attachment #8605389 - Flags: review?(jwwang) → review+
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.
Attachment #8605391 - Attachment is obsolete: true
Attachment #8605391 - Flags: review?(amarchesini)
Attachment #8605802 - Flags: review?(bugs)
Assignee

Comment 7

4 years ago
(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.
Flags: needinfo?(amarchesini)
I think it's safe enough. In the end, instead storing a nsIPrincipal in WebSocketImpl, we take it all the time as a parameter.
Flags: needinfo?(amarchesini)
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?
Attachment #8605802 - Flags: review?(bugs) → review+
(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.
Assignee

Comment 11

4 years ago
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.
Attachment #8605802 - Flags: sec-approval?
I'll approve this to land with bug 1164292. Do you have a suggestion as to a rating? Sec-high?
Assignee

Comment 13

4 years ago
[: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
Keywords: sec-high
Attachment #8605802 - Flags: sec-approval? → sec-approval+
> baku - I'll apply this review comment and take care of pushing these
> patches.(In reply to Al Billings

Ok! thanks.
https://hg.mozilla.org/mozilla-central/rev/4a1b5563fdcc
https://hg.mozilla.org/mozilla-central/rev/415a89038ea1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee

Comment 18

4 years ago
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
Attachment #8605802 - Flags: approval-mozilla-esr38?
Attachment #8605802 - Flags: approval-mozilla-esr31?
Attachment #8605802 - Flags: approval-mozilla-beta?
Attachment #8605802 - Flags: approval-mozilla-aurora?
Comment on attachment 8605802 [details] [diff] [review]
Part 2 - Get rid of mPrincipal from WebSocket

[Triage Comment]
Taking it in 38.0.5 too.
Attachment #8605802 - Flags: approval-mozilla-release+
Attachment #8605802 - Flags: approval-mozilla-esr38?
Attachment #8605802 - Flags: approval-mozilla-esr38+
Attachment #8605802 - Flags: approval-mozilla-esr31?
Attachment #8605802 - Flags: approval-mozilla-esr31+
Attachment #8605802 - Flags: approval-mozilla-beta?
Attachment #8605802 - Flags: approval-mozilla-beta+
Attachment #8605802 - Flags: approval-mozilla-aurora?
Attachment #8605802 - Flags: approval-mozilla-aurora+
Comment on attachment 8605802 [details] [diff] [review]
Part 2 - Get rid of mPrincipal from WebSocket

This needs rebasing for b2g37.
Flags: needinfo?(amarchesini)
(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?
Flags: needinfo?(bobbyholley)
Flags: needinfo?(amarchesini)
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
Attachment #8605802 - Flags: approval-mozilla-release+
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.
Assignee

Updated

4 years ago
Flags: needinfo?(bobbyholley)
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.
Assignee

Comment 30

4 years ago
(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!
Bobby, is there anything manual QA could do to help verify this fix?
Flags: needinfo?(bobbyholley)
Assignee

Comment 32

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

Nope.
Flags: needinfo?(bobbyholley)
Flags: qe-verify-
Whiteboard: [adv-main39+][adv-esr31.8+][adv-esr38.1+]

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.