Closed Bug 1504816 Opened 3 years ago Closed 3 years ago
Buffer source patches from 1475228 may have introduced a use-after-free
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().
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
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.
This needs a sec-approval request too before it can land on trunk.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #6) This won't land on trunk. Bug 1503086 will fix this problem there.
(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.
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: approval-mozilla-release? → approval-mozilla-release-
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main64+]
You need to log in before you can comment on or make changes to this bug.