1660954 - AbortSignal instance must be pinned in AbortSignal::Abort rather than AbortSignalImpl::Abort

Status: RESOLVED FIXED

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

Description

[Simon Giesecke [:sg] [he/him]]

AbortSignal::Abort calls AbortSignalImpl::Abort, which is pinning the instance by Bug 1656957, but it continues to use the instance. Pinning should therefore be moved up to AbortSignal::Abort.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Bug 1660954. r=smaug!

Comment 2

[Daniel Veditz [:dveditz]]

I think you're saying we didn't entirely fix the use-after-free and this is still sec-high? Or is this a minor "sec-audit" kind of clean-up?

Comment 3

[Jeff Walden [:Waldo]]

The UAF was not entirely fixed. Or at least, if the UAF this patch fixes cannot be exploited, it's because of spooky interactions at a distance, like the JS engine preserving the relevant AbortSignal on the stack such that this cannot be invalidated by executing all followers -- and the code is super-smelly but would merely happen not to be buggy.

I would have to think harder about this to say confidently one way or the other (if I even could say so). But it doesn't really seem worth the trouble, versus just doing this, pessimally assuming it must be aggregated with the prior patch to fix the problem completely.

(I observed the fix while poking through AbortSignal code working on bug 1502355 and bug 1660555 and poked :sg to file, after seeing he'd "fixed" but not fixed the original problem. I can't see the original bug, so I don't know if some easily-adapted testcase there would make it easier to give a confident answer.)

Comment 4

[Jeff Walden [:Waldo]]

Current status here is I'm trying to verify this patch doesn't revert bug 1656957. Unfortunately I'm having trouble reproducing that crash, with the rev immediately before that bug's patch landed, so I can't yet verify this patch works right.

Comment 5

[Jeff Walden [:Waldo]]

Managed to repro the original crash, applying my patch atop bug 1656957's patch also results in a properly functional build. Will double-check later whether this means this patch is ready to go for sec-approval, out of time for now.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 9171863 [details]
Bug 1660954. r=smaug!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Changing a raw pointer to a smart pointer for a "loop over this array invoking operations" step has the look of implying pointer invalidation, so someone reading carefully might see there's something to look at. On the other hand, the comment already there is probably a bigger flag. And bug 1656957 did fix the true issue in this, so I don't think an exploit can be constructed for this.
• 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?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This should backport trivially, probably even to most branches without any changes needed.
• How likely is this patch to cause regressions; how much testing does it need?: For the followers in the current array to be there safely, they already have to have nonzero refcount. Incrementing that, invoking the "Abort" operation, then decrementing will not decrement to zero, hence will not deallocate or do anything additional beyond what's done now except the inc/dec. It's hard to see how anything could inadvertently go wrong in that. Still, this relates to memory safety, so there's a modicum of risk.

Comment 7

[Tom Ritter [:tjr]]

Setting tracking flags.

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 9171863 [details]
Bug 1660954. r=smaug!

Approved to land and uplift.

Comment 9

[Jeff Walden [:Waldo]]

Lando'd.

I expect the patch applies without change to branches, but right now I can't easily test, because I have my laptop prepped for wiping (i.e. fully backed up) right now and don't want to do anything very involved (like work on patches, build, etc.) that would require backing up anew. Help landing this on branches, and at least notification if there are any conflicts, is much appreciated.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/c3eec7418d9ec43ecc18d3f431364365bd46e5b5
https://hg.mozilla.org/mozilla-central/rev/c3eec7418d9e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jeff Walden [:Waldo] from comment #9)

Help landing this on branches, and at least notification if there are any conflicts, is much appreciated.

This grafts cleanly to Beta but needs a bit of rebasing around bug 1645339 for ESR78.

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/67f03986ce94

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Bug 1660954. r=smaug!

Comment 14

[Jeff Walden [:Waldo]]

Okay, that should be an esr78-ready patch. Throwing it over to Julien since he landed one branch version already...

Comment 15

[Julien Cristau [:jcristau]]

Landed on esr78 after making the change requested by smaug in review:
https://hg.mozilla.org/releases/mozilla-esr78/rev/5f36d412891c

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.