Boris Zbarsky [:bzbarsky] Reporter
	Description • 6 years ago

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

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 1 • 6 years ago

Though I guess for backport purposes, might be best to just have an appropriately-scoped temporary instead  of changing the null-termination bits.

	Jeff Walden [:Waldo]
	Comment 2 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 3 • 6 years ago

The main poin t of having PromiseFlatString is to be able to pass PromiseFlatString(..).get() to OS APIs...

	Jon Coppeard (:jonco) Assignee
	Comment 4 • 6 years ago

As suggested, keep the string returned by PromiseFlatString() in scope while we compile its contents.

	Jon Coppeard (:jonco) Assignee
	Comment 5 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 6 years ago

This needs a sec-approval request too before it can land on trunk.

	Jon Coppeard (:jonco) Assignee
	Comment 7 • 6 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
This won't land on trunk.  Bug 1503086 will fix this problem there.

	Jon Coppeard (:jonco) Assignee
	Comment 8 • 6 years ago

(In reply to Jon Coppeard (:jonco) from comment #7)
Hmm, maybe I need security approval anyway.

	Jon Coppeard (:jonco) Assignee
	Comment 9 • 6 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 10 • 6 years ago

sec-approval+ for trunk.
I'd like a beta patch nominated as well.

	Al Billings [:abillings - ex-MoCo]
	Comment 11 • 6 years ago

Comment on attachment 9022974 [details] [diff] [review]
buffer-source

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

[Triage Comment]

	Jon Coppeard (:jonco) Assignee
	Comment 12 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/4d29ca8acec1cfe9adab52f35aba27f749cb1b22