Closed Bug 1504816 Opened 3 years ago Closed 3 years ago

Buffer source patches from 1475228 may have introduced a use-after-free

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- wontfix
firefox64 + fixed
firefox65 + fixed

People

(Reporter: bzbarsky, Assigned: jonco)

References

Details

(Keywords: csectype-uaf, regression, sec-high, Whiteboard: [post-critsmash-triage][adv-main64+])

Attachments

(1 file)

The changes in bug 1475228 may have introduced a use-after-free in nsJSUtils::CompileFunction.   The new code looks like this:


  JS::Rooted<JSFunction*> fun(cx);
  JS::SourceBufferHolder source(PromiseFlatString(aBody).get(), aBody.Length(),
                                JS::SourceBufferHolder::NoOwnership);
  if (!JS::CompileFunction(cx, aScopeChain, aOptions,
                           PromiseFlatCString(aName).get(),
                           aArgCount, aArgArray,
                           source, &fun))

if aBody is not flat, PromiseFlatString(aBody) makes a copy and constructs a temporary, .get() returns a pointer into that temporary, and the whole thing goes out of scope before JS::CompileFunction is called so at that point "source" is pointing into freed memory.

Now it's possible that all callsites that reach this point are passing flat strings; I haven't really dug through so far.   But it would be a lot safer to not rely on that.

Bug 1503086 is going to fix this up on nightly, but we don't really want to backport it, so may want a more-targeted fix for backport.

Since SourceBufferHolder doesn't require null-termination, we presumably don't even need PromiseFlatString here.  We can just BeginReading().
Flags: needinfo?(jcoppeard)
Though I guess for backport purposes, might be best to just have an appropriately-scoped temporary instead  of changing the null-termination bits.
Maybe it would be worth adding ref-qualifiers to that get() function so you can't call it on an rvalue?  If you prohibit that, you get rid of this problem, albeit at cost of disallowing passing PFS(...).get() to a caller that doesn't retain the passed pointer til later.
The main poin t of having PromiseFlatString is to be able to pass PromiseFlatString(..).get() to OS APIs...
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attached patch buffer-sourceSplinter Review
As suggested, keep the string returned by PromiseFlatString() in scope while we compile its contents.
Attachment #9022974 - Flags: review?(amarchesini)
Attachment #9022974 - Flags: review?(amarchesini) → review+
Comment on attachment 9022974 [details] [diff] [review]
buffer-source

m-c fix is addressed by bug 1503086.  This patch is for uplift only.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1475228

User impact if declined: Possible UB / crash / security vulnerability.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

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): This is a very simple fix and is low risk.

String changes made/needed: None.
Attachment #9022974 - Flags: approval-mozilla-release?
Attachment #9022974 - Flags: approval-mozilla-beta?
Priority: -- → P2
Priority: P2 → P1
This needs a sec-approval request too before it can land on trunk.
Flags: needinfo?(jcoppeard)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
This won't land on trunk.  Bug 1503086 will fix this problem there.
Flags: needinfo?(jcoppeard)
(In reply to Jon Coppeard (:jonco) from comment #7)
Hmm, maybe I need security approval anyway.
Comment on attachment 9022974 [details] [diff] [review]
buffer-source

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Very difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: 63 and onwards

If not all supported branches, which bug introduced the flaw?: Bug 1475228

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: Same patch applies.

How likely is this patch to cause regressions; how much testing does it need?: Very unlikely.
Attachment #9022974 - Flags: sec-approval?
sec-approval+ for trunk.
I'd like a beta patch nominated as well.
Comment on attachment 9022974 [details] [diff] [review]
buffer-source

Never mind. I see the beta request and am approving it.

[Triage Comment]
Attachment #9022974 - Flags: sec-approval?
Attachment #9022974 - Flags: sec-approval+
Attachment #9022974 - Flags: approval-mozilla-beta?
Attachment #9022974 - Flags: approval-mozilla-beta+
Group: dom-core-security → core-security-release
Whiteboard: [65+ will be fixed by bug 1503086]
Attachment #9022974 - Flags: approval-mozilla-release? → approval-mozilla-release-
Status: NEW → RESOLVED
Closed: 3 years ago
Depends on: 1503086
Resolution: --- → FIXED
Whiteboard: [65+ will be fixed by bug 1503086]
Target Milestone: --- → mozilla65
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main64+]
Component: DOM → DOM: Core & HTML
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.