Use after free in KeyPath::ExtractKey
Categories
(Core :: Storage: IndexedDB, defect, P2)
Tracking
()
People
(Reporter: zhanjiasong45, Assigned: ytausky)
References
Details
(4 keywords, Whiteboard: [reporter-external] [client-bounty-form][fixed in bug 1544750][post-critsmash-triage][adv-main69+][adv-esr68.1+])
Attachments
(8 files, 2 obsolete files)
|
1.10 KB,
text/html
|
Details | |
|
27.15 KB,
text/plain
|
Details | |
|
2.26 KB,
patch
|
tcampbell
:
review-
|
Details | Diff | Splinter Review |
|
17.93 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
|
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Review |
|
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
|
44.34 KB,
patch
|
asuth
:
feedback+
jcristau
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
|
2.10 KB,
patch
|
Details | Diff | Splinter Review |
| Reporter | ||
Updated•6 years ago
|
| Reporter | ||
Comment 1•6 years ago
|
||
Comment 2•6 years ago
|
||
Comment 3•6 years ago
|
||
Comment 4•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 6•6 years ago
|
||
Comment 7•6 years ago
|
||
Comment 8•6 years ago
|
||
Comment 9•6 years ago
|
||
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
Comment 12•6 years ago
|
||
Comment 13•6 years ago
|
||
Comment 14•6 years ago
|
||
Updated•6 years ago
|
Comment 15•6 years ago
|
||
Comment 16•6 years ago
|
||
Comment 17•6 years ago
|
||
Comment 18•6 years ago
|
||
Comment 19•6 years ago
|
||
Comment 20•6 years ago
|
||
Comment 21•6 years ago
|
||
Comment 22•6 years ago
|
||
Comment 23•6 years ago
|
||
Comment 24•6 years ago
|
||
Updated•6 years ago
|
Comment 25•6 years ago
|
||
Any change you can look at this again, Jan? Thanks.
| Reporter | ||
Comment 26•6 years ago
|
||
Is this bug fixed? I wonder when can I disclose it.
Comment 27•6 years ago
|
||
We'd appreciate if you hold off disclosure until the bug is fixed, as I just learned that a fix for this is imminent.
Thank you for reporting the bug and for your patience so far.
| Assignee | ||
Comment 28•6 years ago
|
||
I'll take this over.
First thing, I'll try to refactor the code to be more similar to the spec's control flow and add corresponding excerpts (probably under a different bug number, since it shouldn't look too suspicious). Then I'll fix the wrong JS object accesses here, and finally, see if there's a way to avoid creating a cloned JS object while still maintaining the correctness of the whole thing. I suspect that by weaving key path traversal into the serialization code this would be possible, but let's only do that once correctness and safety are established.
Comment 29•6 years ago
|
||
FWIW, I did propose weaving key path traversal into the serialization, but it added a lot of complexity and potentially required performance-hindering alterations to the JS engine's structured clone logic.
(Specifically, if there was a mechanism for something to listen to the the serialization process, say using the visitor idiom, it could maintain an understanding of the current "path" and capture values at the right path points. Where capturing values could entail listening to further nested traversal. However, I believe I was erroneously assuming that the existing structured clone ops already provided the visibility we needed. I believe reality is that the hooks are not called during normal plain object serialization, and so would need to be made more generic. This was deemed undesirable for complexity reasons and because structured cloning is generally performance-sensitive.)
| Assignee | ||
Comment 30•6 years ago
|
||
Yeah, let's fix it first and then check the performance implications.
Comment 31•6 years ago
|
||
This security vulnerability affects ESR-60 as well.
Updated•6 years ago
|
| Assignee | ||
Comment 32•6 years ago
|
||
Small update: I'm still working on this; making control flow as close as possible to the spec without touching the lower-level implementation details is a little tricky here.
We would probably need to consider more hooks into the structured clone algorithm, though. It seems to be used as a basic building block for optimization-ripe spec operations. There are ways to mitigate potential performance problems for non-specialized use cases (e.g. have versions with and without hooks instantiated from a template), so it would probably boil down to the trade-off between implementation complexity and some DOM components' performance.
Comment 33•6 years ago
|
||
This is going to miss the 67/60.7.0esr releases. If you think we may come up with a fix and it would drive a dot release, we can keep tracking for 67. Otherwise we should wontfix it and move the flags to 68.
| Assignee | ||
Comment 34•6 years ago
|
||
I'm following tcampbell's recommendations concerning this bug, and they necessitate a bigger-ish refactoring of the surrounding code, including iteration on the design of several classes. I'm not sure it would make sense as a point release.
Comment 36•6 years ago
|
||
Too late now for 68.
Comment 37•6 years ago
|
||
Tom is helping here while Yaron is on leave. Thanks, Tom!
Comment 38•6 years ago
|
||
Comment 39•6 years ago
|
||
To clarify, I was under the impression that Yaron's work on bug 1544750 was basically ready to land, modulo the one line: "Blockers: The missing SupressExceptions and calling aRv.Throw without checking it isn't already having an Exception." in https://phabricator.services.mozilla.com/D28976#1020675 with the rest able to be follow-ups. Is there a reason not to land that?
(I realize there may have been some confusion since we didn't explicitly establish a "see also" relationship between bug 1544750 and this one because it would make it clear that bug 1544750 is a security fix pretending to be a cleanup.)
Comment 40•6 years ago
|
||
(In reply to Andrew Sutherland [:asuth] (he/him) from comment #39)
To clarify, I was under the impression that Yaron's work on bug 1544750 was basically ready to land, modulo the one line: "Blockers: The missing SupressExceptions and calling aRv.Throw without checking it isn't already having an Exception." in https://phabricator.services.mozilla.com/D28976#1020675 with the rest able to be follow-ups. Is there a reason not to land that?
(I realize there may have been some confusion since we didn't explicitly establish a "see also" relationship between bug 1544750 and this one because it would make it clear that bug 1544750 is a security fix pretending to be a cleanup.)
Ah, I somehow couldn't connect these two tickets with each other. I'll rebase and try to address the patch in bug 1544750.
Updated•6 years ago
|
Comment 41•6 years ago
•
|
||
Hi Daniel,
The fix for this issue has been landed by pretending itself to be a cleanup in bug 1544750. Since this is not the general procedure for security issue (at least I couldn't find a document for this), I have a few questions for the status of this bug. Would you mind giving me some feedback? Thanks!
My questions here are:
- Can we close this bug now? And would it be safe if we set any dependency between this and bug 1544750 now? (e.g. duplicate or see also)
- Should we uplift the patches in bug 1544750 to Beta since this is a sec-high issue from your perspective? (I wouldn't recommend doing that since the patches also refactor surrounding code)
Comment 42•6 years ago
|
||
One problem with with hiding security fixes in unrelated bugs is that it completely bypasses the sec-approval process which would have answered some of those questions, as well as explored potential solutions to problems such as "if we can't uplift this to Beta, how the heck are we going to secure ESR-68 for the next year it's supported?" This process is linked to in the warning text "sec-approval required on patches before landing" in the attachments area of this bug.
Can we close this bug now? And would it be safe if we set any dependency between this and bug 1544750 now? (e.g. duplicate or see also)
We need to mark this bug FIXED so it triggers the proper advisory and bug bounty treatment. Linking them now is OK.
So now what? If we wait until Firefox 70 ships that's late October which is pretty long for a sec-high fix to sit in the tree. We've done that before but this patch looks fairly straightforward, if large, so maybe it's safe to backport? We definitely need to backport to ESR-68 branch anyway.
We're also still supporting ESR-60. How complex would it be to backport to that? I can't imagine IndexedDB has changed much.
Comment 43•6 years ago
|
||
Tom, are we sure that Bug 1544750 covered all the cases for this bug? For example, [1] still can run arbitrary JS which is what the spec allows. This fixes the prototype stuff (and we now better follow spec), but is the re-entrancy issue addressed in other ways?
[1] https://hg.mozilla.org/integration/autoland/rev/b14b8efda186#l8.169
Comment 44•6 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #42)
One problem with with hiding security fixes in unrelated bugs is that it completely bypasses the sec-approval process which would have answered some of those questions, as well as explored potential solutions to problems such as "if we can't uplift this to Beta, how the heck are we going to secure ESR-68 for the next year it's supported?" This process is linked to in the warning text "sec-approval required on patches before landing" in the attachments area of this bug.
I see. Thanks for the explanation! I will try to ask the sec-approval in the security bug for the patches in the unrelated bugs if I have similar condition next time. So that, we can also cover this problem.
So now what? If we wait until Firefox 70 ships that's late October which is pretty long for a sec-high fix to sit in the tree. We've done that before but this patch looks fairly straightforward, if large, so maybe it's safe to backport? We definitely need to backport to ESR-68 branch anyway.
It makes sense to backport it to ESR-68, but it's better to observe the status for the patch on the Nightly for a while (a week). If the patch is stable (there is no new issues related to that for a week), then I will check and try to uplift the patch to ESR-68.
We're also still supporting ESR-60. How complex would it be to backport to that? I can't imagine IndexedDB has changed much.
I rebased both part1 and part2 on Bug 1544750. There are some conflicts on dom/indexedDB/ActorsParent.cpp and dom/indexedDB/Key.cpp. I'll take a closer look (checking the changes on IDBKey after ESR60 til now since patches on Bug 1544750 refactor the code there) next week when I rebase the patches to ESR68.
Also, we still need to check the performance impact after applying the patches on Bug 1544750.
Keep the ni flag to remind me of checking the complexity of backporting to 60 next week.
Comment 45•6 years ago
|
||
(In reply to Ted Campbell [:tcampbell] from comment #43)
Tom, are we sure that Bug 1544750 covered all the cases for this bug? For example, [1] still can run arbitrary JS which is what the spec allows. This fixes the prototype stuff (and we now better follow spec), but is the re-entrancy issue addressed in other ways?
[1] https://hg.mozilla.org/integration/autoland/rev/b14b8efda186#l8.169
Ah, sorry for the confusion. I was thinking this bug is mainly for the prototype stuff and the rest of the works should be taken care of in other bugs. And I was wrong. I'll ensure that we have taken all the cases before closing this security bug. Thanks for the notice!
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
| Assignee | ||
Comment 46•6 years ago
|
||
In the strictest sense, the re-entrancy issue is already addressed by cloning the value, which strips getters. By avoiding the prototype chain, I believe the security hole is plugged. The PoC posted on this issue doesn't produce an ASan message for me any more.
Comment 47•6 years ago
|
||
I think there are still some theoretical edge cases in spidermonkey, I don't remember exactly, maybe when some wrappers or proxies are involved, so long term it would be safer to fix bug 1570585.
Comment 48•6 years ago
•
|
||
Quick update: the plan for uplift is that if I/Yaron can implement a quick & simple fix for bug 1570585 (I have a naive plan for fixing that in mind, but it need to be verified), then maybe it's better to uplift the patch there to Beta or ESR since I expect the patch there would be smaller and should also fix the issue here. If we cannot make it, I would suggest just uplifting patches here.
| Assignee | ||
Comment 49•6 years ago
|
||
Jan (and Ted, privately) are correct, I forgot about the possibility of using proxies. So this is not fixed yet.
| Assignee | ||
Comment 50•6 years ago
|
||
Back-flipping yet again, step 23 of StructuredSerializeInternal makes clones of objects with proxies impossible, so I once again think this is actually fixed in m-c. I think this highlights the need for a systematic approach to dealing with bug 1570585.
Comment 51•6 years ago
|
||
We have one beta left before 69 goes to RC next week. If we plan to do something for Fx69/68.1esr, that needs to be sorted out ASAP.
| Assignee | ||
Comment 52•6 years ago
|
||
I'm compiling the patches from bug 1544750 for beta, when that's done I'll do it for the ESR as well.
| Assignee | ||
Comment 53•6 years ago
|
||
This commit adds the text of the spec as inline comments and refactors
the code such that it directly corresponds to the spec's steps. This
makes it easier to understand how the spec's algorithm is implemented.
| Assignee | ||
Comment 54•6 years ago
|
||
This commit adds the text of the spec as inline comments and refactors
the code such that it directly corresponds to the spec's steps. This
makes it easier to understand how the spec's algorithm is implemented.
| Assignee | ||
Comment 55•6 years ago
|
||
Comment on attachment 9087003 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell
Beta/Release Uplift Approval Request
- User impact if declined: Potentially exploitable use-after-free
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): It's been on nightly and we haven't seen regressions.
- String changes made/needed: None
Security Approval Request
- How easily could an exploit be constructed based on the patch?: Easy to trigger UAF, not sure how easy it is to exploit
- Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
- Which older supported branches are affected by this flaw?: esr68
- If not all supported branches, which bug introduced the flaw?: None
- Do you have backports for the affected branches?: Yes
- If not, how different, hard to create, and risky will they be?:
- How likely is this patch to cause regressions; how much testing does it need?: It's possible that it introduces regressions since it's a new implementation of existing functionality, but it's been on nightly and we haven't seen any regressions.
| Assignee | ||
Updated•6 years ago
|
| Assignee | ||
Comment 56•6 years ago
|
||
Comment on attachment 9087298 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration:
- User impact if declined: Potentially exploitable UAF
- Fix Landed on Version: 70
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): It's been tested on nightly and we haven't seen regressions.
- String or UUID changes made by this patch: None
Comment 57•6 years ago
|
||
Can you clarify the status for esr60? The sec-approval says this only goes back as far as esr68, but comment 31 says esr60 is also affected. Also, updating the flags to reflect that this is fixed in 70 already.
Comment 58•6 years ago
|
||
Comment on attachment 9087003 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell
This has baked on Nightly for awhile now, let's take it for 69.0b16 also.
Comment 59•6 years ago
|
||
| uplift | ||
Comment 60•6 years ago
|
||
Comment on attachment 9087003 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell
Giving sec-approval+ since it is already in all the things. :-)
| Assignee | ||
Comment 61•6 years ago
•
|
||
I forgot about esr60, I'll backport it now.
Update: it does not apply cleanly, so this may require more work and testing.
Updated•6 years ago
|
Comment 62•6 years ago
|
||
Comment on attachment 9087003 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell
This isn't obsolete just because it was uplifted already.
Comment 63•6 years ago
|
||
Comment on attachment 9087298 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell
Approved for 68.1esr.
Comment 64•6 years ago
|
||
| uplift | ||
Comment 65•6 years ago
|
||
| backout | ||
Backed out for from ESR68 bustage in IDBResult.h:
https://hg.mozilla.org/releases/mozilla-esr68/rev/9fd0d4493c77bfed937299c2b7fd3843f8543a3a
Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&revision=091473bf2727d37a4107cfca1185ef99c3f2eeff
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=263243122&repo=mozilla-esr68
[task 2019-08-23T20:05:13.369Z] 20:05:13 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/IDBCursor.h:14:
[task 2019-08-23T20:05:13.369Z] 20:05:13 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/indexedDB/Key.h:10:
[task 2019-08-23T20:05:13.369Z] 20:05:13 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/indexedDB/IDBResult.h:164:35: error: bad implicit conversion constructor for 'IDBResult'
[task 2019-08-23T20:05:13.369Z] 20:05:13 INFO - using IDBResult::IDBResultBase::IDBResultBase;
[task 2019-08-23T20:05:13.369Z] 20:05:13 INFO - ^
[task 2019-08-23T20:05:13.370Z] 20:05:13 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/indexedDB/IDBResult.h:164:35: note: consider adding the explicit keyword to the constructor
[task 2019-08-23T20:05:13.370Z] 20:05:13 INFO - using IDBResult::IDBResultBase::IDBResultBase;
[task 2019-08-23T20:05:13.370Z] 20:05:13 INFO - ^
[task 2019-08-23T20:05:13.370Z] 20:05:13 INFO - explicit
Updated•6 years ago
|
| Assignee | ||
Comment 66•6 years ago
|
||
Right, that patch triggered a bug in the static analysis, which was fixed in bug 1554989. I'll have to uplift that one as well.
| Assignee | ||
Comment 67•6 years ago
|
||
Inheriting constructors are implicitly introduced via a using-declaration.
Since the C++ grammar doesn't allow attributes on using-declarations, it
is currently impossible to add MOZ_IMPLICIT to implicit inheriting
constructors.
This commit changes the AST matcher such that it ignores inheriting
constructors altogether. If they are inheriting from an implicit inherited
constructor, that constructor's check should be enough to ensure that no
constructors are unintentionally implicit.
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell a=RyanVM
This commit adds the text of the spec as inline comments and refactors
the code such that it directly corresponds to the spec's steps. This
makes it easier to understand how the spec's algorithm is implemented.
Comment 68•6 years ago
|
||
| uplift | ||
Updated•6 years ago
|
Updated•6 years ago
|
| Assignee | ||
Comment 70•6 years ago
|
||
I'm slowed down with it because my local build of esr60 fails and there are some merge conflicts that need to be solved and tested.
Comment 71•6 years ago
|
||
I've used Fx65.0a1 (2018-10-22) asan build and tried loading the test-case to reproduce the issue, but the tabs keep crashing so I cannot run the test-case.
Is there anything that can be done by QA to verify this?
Updated•6 years ago
|
| Assignee | ||
Comment 72•6 years ago
|
||
You could verify that there are no crashes with the newest builds for ESR68 and Beta; all other versions are known to be affected (I'm still working on ESR60).
Comment 73•6 years ago
|
||
Verified with the fixed Fx70.0a1, Fx69.0b16 and ESR68 builds. The tabs no longer crash when loading the test-case. Waiting for the fix on ESR60.
Comment 74•6 years ago
|
||
Andrew, can you sanity-check this?
There were some conflicts in Key.cpp around bug 1499854, and a dependency on bug 1429613 for IDBResult (the latter fixed by janv with https://paste.mozilla.org/1CY8myH6). This applies on top of https://hg.mozilla.org/mozilla-central/rev/e8c59393c809 (which grafts cleanly to esr60).
Comment 75•6 years ago
|
||
Comment 76•6 years ago
|
||
Sorry, the pastebin lasted just one hour.
Comment 77•6 years ago
|
||
I have rustc 1.36.0 too and I can build it. However, one need to do this before configure/build:
export RUSTFLAGS="--cap-lints warn"
to workaround bug 1519629, this was all discovered by Julien (thanks for the investigation).
Comment 78•6 years ago
|
||
Comment 79•6 years ago
|
||
| uplift | ||
Comment 80•6 years ago
|
||
Confirming the tabs no longer crash on Fx 60.9.0ESR buildID: 20190901094603. Marking the entire ticket as verified fixed.
Updated•5 years ago
|
Updated•3 years ago
|
Updated•11 months ago
|
Description
•