Closed Bug 1281874 Opened 4 years ago Closed 4 years ago
Crypto Task dispatches to worker thread without holding it alive
2.23 KB, patch
|Details | Diff | Splinter Review|
8.63 KB, patch
|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.
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.
Ritu, FYI, this would probably be worth taking in a point release for 47.
Looking at bug 842818 it seems web Crypto is only enabled on workers starting in FF48. So we can avoid some extreme uplifts here.
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.
Yea, that's what I am working on now.
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?
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.
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.
(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.
Still working on the test, but here is my proposed fix.
Attachment #8765028 - Flags: review?(khuey)
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.
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.
This is the P1 patch rebased for beta/aurora. I had to convert it back to use WorkerFeature. I verified it passes the test.
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?
Comment on attachment 8766820 [details] [diff] [review] Beta/Aurora P1 patch See comment 17. This is the rebased beta/aurora patch.
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.
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.
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.
And some windows bustage. Really glad I did the try runs. https://treeherder.mozilla.org/#/jobs?repo=try&revision=edd57a970be6
Attachment #8765028 - Flags: sec-approval? → sec-approval+
Comment on attachment 8766820 [details] [diff] [review] Beta/Aurora P1 patch Approvals given.
sorry backed out from beta and aurora for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2903541&repo=mozilla-aurora
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.
I will land this in all trees in about 2 hours. I just have to take my daughter to school first.
Updated patch with try fixes.
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
You need to log in before you can comment on or make changes to this bug.