Closed Bug 1448612 Opened 2 years ago Closed 2 years ago

Double free in nsStreamLoader (presumably because of FetchConsumer stuff).


(Core :: DOM: Workers, defect)

Not set



Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed


(Reporter: emilio, Assigned: emilio)



(4 keywords, Whiteboard: [adv-main60+][post-critsmash-triage])


(2 files)

So I got this interesting crash while I played with That uses a worker to pull data from github and stuff:

Anyway, looking into how could this happen, I found a very suspicious piece of code. The data is presumably freed the second time here (well, on the ReleaseData call just below):

But something has freed it already.

Looks highly suspicious. If it's not it, that needs fixing as well.

In particular, look how the constructor of AbortConsumeBodyControlRunnable frees the data, yet it doesn't return NS_SUCCESS_ADOPTED_DATA, which means that the nsStreamLoader code will assume it still owns it.
Assignee: nobody → emilio
Attachment #8962115 - Flags: review?(amarchesini)
In the case we couldn't dispatch the runnable, we'd double-free it, which is no good.
Attachment #8962115 - Flags: review?(amarchesini) → review+
Comment on attachment 8962115 [details] [diff] [review]
Don't take ownership of the stream data too early and then lie.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Somewhat hard I'd think, at least intentionally. You need to make the network event arrive at the right time, when the worker thread is already gone.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It's not obvious, but you can see it if you grep for the involved return codes.

Which older supported branches are affected by this flaw?
FF59 and 60.

If not all supported branches, which bug introduced the flaw?
bug 1422036

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?
Not much, the patch is really small and somewhat trivial.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1422036
[User impact if declined]: crashes w/ security implication.
[Is this code covered by automated tests?]: seems like none caught it.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: simple (as in, small and easy to prove correct) resource management fix.
[String changes made/needed]: none
Attachment #8962115 - Flags: sec-approval?
Attachment #8962115 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1448635
Group: core-security → dom-core-security
Are you sure about the regressor? All the crashes I see in the last month with "mozilla::net::nsStreamLoader::OnStopRequest" in the signature happen in 61.0a1 -- none in 60.0 betas.
Blocks: 1422036
Flags: needinfo?(emilio)
Looking at bug 1448635 comment 6, looks like the switch to WorkerRef exposed this, though the code was incorrect before in case dispatch failed. Probably it just didn't fail.
Flags: needinfo?(emilio)
To echo Dan's comment 5, one can look at a lot of double-free signatures that don't necessarily have OnStopRequest in the signatures, but wind up being (almost all) OnStopRequest-related:

...and none of them have any crashes outside of 61.
See Also: → 1449241
Comment on attachment 8962115 [details] [diff] [review]
Don't take ownership of the stream data too early and then lie.

Clearing sec-approval. As a sec-moderate, it doesn't need sec-approval to go into trunk. That also means it may not be back ported to Beta.
Attachment #8962115 - Flags: sec-approval?
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8962115 [details] [diff] [review]
Don't take ownership of the stream data too early and then lie.

Approved for 60.0b8.
Attachment #8962115 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: dom-core-security → core-security-release
Looks like this'll need a rebased patch for Beta uplift.
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Duplicate of this bug: 1449945
Had to add an explicit to make the static analysis happy too.
Whiteboard: [adv-main60+]
Flags: qe-verify-
Whiteboard: [adv-main60+] → [adv-main60+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.