Closed
Bug 1281874
Opened 8 years ago
Closed 8 years ago
WebCryptoTask dispatches to worker thread without holding it alive
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | + | fixed |
firefox49 | + | fixed |
firefox-esr45 | --- | unaffected |
firefox50 | + | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
(Keywords: sec-critical, Whiteboard: [post-critsmash-triage])
Attachments
(3 files, 2 obsolete files)
2.23 KB,
patch
|
khuey
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
8.63 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
8.84 KB,
patch
|
Details | Diff | Splinter Review |
It seems WebCryptoTask dispatches runnables to a thread pool and then back to the original thread when its complete. When used on a worker thread, though, it does not hold the Worker alive using a WorkerHolder (used to be called a WorkerFeature). I'm fairly certain this means we can end up running on top of garbage memory if the worker shuts down before the crypto returns. I'm marking this secure since it seems like it could be reliably forced to happen. I believe this is the cause of bug 1281868, but don't want to link the secure bug to it.
Assignee | ||
Comment 1•8 years ago
|
||
I haven't verified yet, but looking at the code I'm fairly certain this is in every version of firefox that supported WebCrypto in workers. In the case we saw in the wild it was triggered by a service worker, but I think it can be triggered using a normal dedicated worker as well. I'll write a test.
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox-esr45:
--- → affected
tracking-firefox47:
--- → ?
tracking-firefox48:
--- → ?
tracking-firefox49:
--- → ?
tracking-firefox50:
--- → ?
tracking-firefox-esr45:
--- → ?
Assignee | ||
Comment 2•8 years ago
|
||
Ritu, FYI, this would probably be worth taking in a point release for 47.
Flags: needinfo?(rkothari)
Assignee | ||
Updated•8 years ago
|
Keywords: sec-critical
Assignee | ||
Comment 3•8 years ago
|
||
Looking at bug 842818 it seems web Crypto is only enabled on workers starting in FF48. So we can avoid some extreme uplifts here.
tracking-firefox47:
? → ---
tracking-firefox-esr45:
? → ---
This would appear to be a problem, yes. An easy way to fix this might be to make the SubtleCrypto add itself as a WorkerFeature (soon to be WorkerHolder) when an asynchronous operation is in progress.
Assignee | ||
Comment 5•8 years ago
|
||
Yea, that's what I am working on now.
Assignee | ||
Comment 6•8 years ago
|
||
I think the WebCryptoTask needs to be the WorkerHolder.
(In reply to Ben Kelly [:bkelly] from comment #6) > I think the WebCryptoTask needs to be the WorkerHolder. Why?
Assignee | ||
Comment 8•8 years ago
|
||
I'll look again tomorrow, but it seemed SubtleCrypto was basically a passive factory. It just sits there until someone calls encrypt() or something. When that happens a WebCryptoTask is created to perform the work on the thread pool. So WebCryptoTask is the "op" in this case. That suggests to me it's the most appropriate to be the WorkerHolder since we want to stop holding the worker alive once all operations complete.
Comment 9•8 years ago
|
||
FYI, the guardian notifications that prompted this issue work fine for me in nightly. I think we need a test case that actually triggers the bad behaviour.
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #9) > FYI, the guardian notifications that prompted this issue work fine for me in > nightly. I think we need a test case that actually triggers the bad > behaviour. That is likely because we are not killing the service worker as aggressively as we used to. But this is partly why I wrote this separate bug. I think they are related, but could be separate issues. My first attempt at a test didn't trigger anything yesterday. I have another idea I'll try today. Regardless, we definitely need a WorkerHolder here.
Assignee | ||
Comment 11•8 years ago
|
||
Still working on the test, but here is my proposed fix.
Attachment #8765028 -
Flags: review?(khuey)
Assignee | ||
Comment 12•8 years ago
|
||
This test reliably triggers a JSContext assert without the P1, but passes with the P1 applied.
Attachment #8765051 -
Flags: review?(khuey)
(In reply to Ben Kelly [:bkelly] from comment #3) > Looking at bug 842818 it seems web Crypto is only enabled on workers > starting in FF48. So we can avoid some extreme uplifts here. Phew! I am glad 47 is unaffected. Thanks for the due diligence on this one Ben.
Flags: needinfo?(rkothari)
Updated•8 years ago
|
Component: DOM: Security → Security
Updated•8 years ago
|
Group: core-security → dom-core-security
Comment on attachment 8765028 [details] [diff] [review] P1 Hold the worker alive while performing web crypto async work. r=khuey Review of attachment 8765028 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/WebCryptoTask.cpp @@ +20,5 @@ > #include "mozilla/dom/WebCryptoCommon.h" > #include "mozilla/dom/WebCryptoTask.h" > #include "mozilla/dom/WebCryptoThreadPool.h" > +#include "mozilla/dom/WorkerPrivate.h" > +#include "mozilla/dom/workers/bindings/WorkerHolder.h" Ugh, we really should just export that to mozilla/dom.
Attachment #8765028 -
Flags: review?(khuey) → review+
Attachment #8765051 -
Flags: review?(khuey) → review+
A+ for writing a test, seriously.
Assignee | ||
Comment 16•8 years ago
|
||
This is the P1 patch rebased for beta/aurora. I had to convert it back to use WorkerFeature. I verified it passes the test.
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8765028 [details] [diff] [review] P1 Hold the worker alive while performing web crypto async work. r=khuey [Security approval request comment] How easily could an exploit be constructed based on the patch? I think someone could fairly easily construct a worker which triggers the lifetime issues fixed by the patch. For example, the P2 test does just this. Using that lifetime issue to execute code at the UAF memory location would require additional work. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? I think the patch is pretty clearly about a worker lifetime issue. I could remove the comments, but I don't think it would obfuscate the issue very much. The P2 patch clearly demonstrates the problem. Which older supported branches are affected by this flaw? This issue was introduced in bug 842818. It only affects beta 48, aurora 49, and nightly 50. If not all supported branches, which bug introduced the flaw? Bug 842818. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Yes. I've attached a rebased P1 that I have verified works in beta/aurora. I had to convert WorkerHolder to WorkerFeature. How likely is this patch to cause regressions; how much testing does it need? I would say the risk is minimal. We've had WorkerFeature/WorkerHolder in the tree for a while. I've written a test to verify the behavior. Existing crypto tests continue to pass. I have not run a full try yet, but I don't expect any bustage.
Attachment #8765028 -
Flags: sec-approval?
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8766820 [details] [diff] [review] Beta/Aurora P1 patch See comment 17. This is the rebased beta/aurora patch.
Attachment #8766820 -
Flags: sec-approval?
Attachment #8766820 -
Flags: approval-mozilla-beta?
Attachment #8766820 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8765051 [details] [diff] [review] P2 Verify that terminating a worker running web crypto works correctly. r=khuey See comment 17. Since this only affects beta/aurora/nightly I'm hoping its safe to land the tests at the same time.
Attachment #8765051 -
Flags: sec-approval?
Attachment #8765051 -
Flags: approval-mozilla-beta?
Attachment #8765051 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 20•8 years ago
|
||
Daniel has given me the go-ahead to push a try build with sanitized commit messages. I'll do that once the try repo opens up again.
Assignee | ||
Comment 21•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4531d43c3305
Assignee | ||
Comment 22•8 years ago
|
||
There was opt build bustage. Another try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c5a384501f1 I'll upload fixed patches after I see this build run.
Assignee | ||
Comment 23•8 years ago
|
||
And some windows bustage. Really glad I did the try runs. https://treeherder.mozilla.org/#/jobs?repo=try&revision=edd57a970be6
Updated•8 years ago
|
Attachment #8765028 -
Flags: sec-approval? → sec-approval+
Updated•8 years ago
|
Attachment #8765051 -
Flags: sec-approval?
Attachment #8765051 -
Flags: sec-approval+
Attachment #8765051 -
Flags: approval-mozilla-beta?
Attachment #8765051 -
Flags: approval-mozilla-beta+
Attachment #8765051 -
Flags: approval-mozilla-aurora?
Attachment #8765051 -
Flags: approval-mozilla-aurora+
Comment 24•8 years ago
|
||
Comment on attachment 8766820 [details] [diff] [review] Beta/Aurora P1 patch Approvals given.
Attachment #8766820 -
Flags: sec-approval?
Attachment #8766820 -
Flags: sec-approval+
Attachment #8766820 -
Flags: approval-mozilla-beta?
Attachment #8766820 -
Flags: approval-mozilla-beta+
Attachment #8766820 -
Flags: approval-mozilla-aurora?
Attachment #8766820 -
Flags: approval-mozilla-aurora+
Comment 27•8 years ago
|
||
sorry backed out from beta and aurora for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2903541&repo=mozilla-aurora
Assignee | ||
Comment 28•8 years ago
|
||
Yes, I have fixes from the try to fold in first. I tried to explain this in comment 22 and comment 23. I was letting try run over night to let it finish.
Assignee | ||
Comment 29•8 years ago
|
||
I will land this in all trees in about 2 hours. I just have to take my daughter to school first.
Assignee | ||
Comment 30•8 years ago
|
||
Updated patch with try fixes.
Attachment #8765028 -
Attachment is obsolete: true
Attachment #8767165 -
Flags: review+
Assignee | ||
Comment 31•8 years ago
|
||
Attachment #8766820 -
Attachment is obsolete: true
Assignee | ||
Comment 32•8 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/f55bcdf46a0e remote: https://hg.mozilla.org/releases/mozilla-beta/rev/5e6976a28642 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/0ccbc3447026 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/9056c1ff97a6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/00f81b0e1993 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/28f16fa4bc48
Comment 33•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/00f81b0e1993 https://hg.mozilla.org/mozilla-central/rev/28f16fa4bc48
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Updated•8 years ago
|
Group: dom-core-security → core-security-release
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Component: Security → DOM: Security
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•