Closed Bug 1568476 Opened 5 years ago Closed 5 years ago

Debugger::ScriptQuery::innermostForRealm not considered a rooting hazard

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

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?

Flags: needinfo?(sphink)

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.

It seems that bug 1257982 already has this change in it so I might just dupe this bug.

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?

Keywords: sec-moderate

(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.

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.
Attachment #9080341 - Flags: sec-approval?

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.

Attachment #9080341 - Flags: sec-approval?
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Assignee: nobody → jcoppeard

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?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
Agreed, we don't need to uplift this.

Flags: needinfo?(sphink)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main70-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: