Debugger::ScriptQuery::innermostForRealm not considered a rooting hazard
Categories
(Core :: JavaScript: GC, defect)
Tracking
()
People
(Reporter: jonco, Assigned: jonco)
Details
(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main70-])
Attachments
(1 file)
This is a normal HashMap containing JSScript pointers:
https://searchfox.org/mozilla-central/source/js/src/debugger/Debugger.cpp#4849
The ScriptQuery class is a stack class and all its other members are Rooted<>s.
The findScripts() method calls IterateScripts and IterateLazyScripts, both of which will do an AutoEmptyNursery. I think the first one will populate this table. So I think this should be considered a hazard.
I don't think this is really a hazard because the nursery will already be empty by the time we call IterateLazyScripts and I didn't see anything that happens between calling IterateScripts and using the contents of this table that could trigger a GC. But I'm not 100% sure and it would be pretty easy to break this.
The analysis does think that findScripts() can GC, so shouldn't calling it with the ScriptQuery class on the stack be considered a hazard?
Assignee | ||
Comment 1•5 years ago
|
||
Depends on D39201
Comment 2•5 years ago
|
||
You're right. The problem is that I tagged GCHashMap with MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS, so if this were a GCHashMap the problem would have been caught. I did not annotation the plain HashMap. I'll have to try doing that and see how many problems other than this one it produces.
Assignee | ||
Comment 3•5 years ago
|
||
It seems that bug 1257982 already has this change in it so I might just dupe this bug.
Comment 4•5 years ago
|
||
That bug hasn't been touched since March. If we need this as a security fix maybe it should stay here where it's active?
Assignee | ||
Comment 5•5 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #4)
This is not a fix, but it does prevent someone unwittingly introducing a vulnerability which the hazard analysis won't catch.
Assignee | ||
Comment 6•5 years ago
|
||
Comment on attachment 9080341 [details]
Bug 1568476 - Root ScriptQuery::innermostForRealm for consistency and in case this ever becomes a hazard r=sfink?
Security Approval Request
- How easily could an exploit be constructed based on the patch?: It could not. This is not a fix, just a change to avoid possibility of a vulnerability being introduced in the future.
- 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?: Everything back to FF32
- 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?: Should be trivial
- How likely is this patch to cause regressions; how much testing does it need?: Unlikely, this is a simple change to root a hash map.
Comment 7•5 years ago
|
||
Comment on attachment 9080341 [details]
Bug 1568476 - Root ScriptQuery::innermostForRealm for consistency and in case this ever becomes a hazard r=sfink?
sec-moderate bugs don't need sec approval, only sec-high and sec-crit bugs that affect more than trunk.
Comment 8•5 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/18a2e793ef0a0c815158214b86f2c942bda33095
https://hg.mozilla.org/mozilla-central/rev/18a2e793ef0a
Updated•5 years ago
|
Comment 9•5 years ago
|
||
I'm on the fence about uplift here - doesn't sound like we really need this if it's mainly just preventing new issues from happening in the future?
Assignee | ||
Comment 10•5 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
Agreed, we don't need to uplift this.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•3 years ago
|
Description
•