Closed Bug 1478573 Opened 6 years ago Closed 6 years ago

AddressSanitizer: heap-use-after-free [@ IsArray] with READ of size 4 with IndexedDB

Categories

(Core :: Storage: IndexedDB, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed

People

(Reporter: decoder, Assigned: janv)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 63.0a1-20180726001822-https://hg.mozilla.org/mozilla-central/rev/7ba07ef0e4532b644b812942aa38af4510dbc74f (originally reported by %client%).

For detailed crash information, see attachment.
Jan: can you take a look please.
Group: core-security → dom-core-security
Flags: needinfo?(jvarga)
Ok, this looks like bug 1459383, we thought it was fixed by bug 1404274.
I'll investigate it more.
Ok, it took me a while, but I finally know what's going on here.
Flags: needinfo?(jvarga)
Attached patch patchSplinter Review
I think we should add tests for this, will do in a new bug if you don't mind.
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Attachment #8995366 - Flags: review?(bugmail)
Comment on attachment 8995366 [details] [diff] [review]
patch

Review of attachment 8995366 [details] [diff] [review]:
-----------------------------------------------------------------

Love the comment!
Attachment #8995366 - Flags: review?(bugmail) → review+
And yes, agreed new bug for the tests.
Triggering this condition requires a length getter that's overridden -- and changes. Is that right?

I don't write much Javascript these days, but is there a use case for that in every day programming? Is there any chance we captured a real exploit? (Normally I'm not inclined to such hypothesis, but it seems like a strange code pattern to occur non-maliciously).
The idiom is either malicious or bug-hunting/fuzzing.  A getter that deletes an index makes no sense, especially since it only works inside a versionchange transaction.

Given that we thought we fixed bug 1459383, but didn't quite, and the ASAN build is explicitly to report such things, I'd expect the bug reporter there or someone testing the repro on that bug and triggering the test.  I know I've manually triggered the test accidentally, but it wasn't in my ASAN profile.
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #8)
> Triggering this condition requires a length getter that's overridden -- and
> changes. Is that right?

Not a length getter.  Any getter.
Thanks for confirming (and correcting!) Using ASAN-reporter nightly for vuln researcher definitely seems like something someone might do, though I'm a bit uncomfortable assuming that without knowing who the reporter was (I'm assuming "%client%" means we didn't have an email for them).

Dan, do you have an opinion here? Is my spidey-sense overly sensitive?
Flags: needinfo?(dveditz)
Note the "file:// Content" here:

> READ of size 4 at 0x60700012f3b0 thread T0 (file:// Content)

So someone was running this on a local file. But why would an exploit researcher use a reporter build?
Good catch, I totally missed that. Thanks Christian.

I assume because it was a convenient way to get an ASAN. Maybe they were just testing a PoC out locally without thinking about it.
Flags: needinfo?(dveditz)
Comment on attachment 8995366 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch fixes an additional problem/regression that bug 1404274 introduced, it landed only several days ago on m-c (only). However the original security problem in bug 1459383 existed for long time and the fix for it landed as a normal bug fix in 1404274. So we fixed the security problem by landing a correctness fix that makes some web platform tests pass. This makes it much less obvious that there was a security problem.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Tests will be added separately, but the comment in the patch can point to a possible security problem. However, the cloning landed just recently on m-c and we are fixing this additional problem very quickly.

Which older supported branches are affected by this flaw?
None

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
There are no affected branches.

How likely is this patch to cause regressions; how much testing does it need?
It's a simple and safe patch, the patch was tested locally.
Attachment #8995366 - Flags: sec-approval?
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #8)
> Triggering this condition requires a length getter that's overridden -- and
> changes. Is that right?

See the first attachment in bug 1459383.
No longer blocks: asan-nightly-project
Attachment #8995366 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/bfe3c2d7de59
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Group: dom-core-security → core-security-release
Jan, since we uplifted the patch from bug 1404274 to beta, should we also uplift this?
Flags: needinfo?(jvarga)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #19)
> Jan, since we uplifted the patch from bug 1404274 to beta, should we also
> uplift this?

No, we provided a special uplift patch in bug 1404274 which contains changes from this bug.
Flags: needinfo?(jvarga)
Blocks: 1404274
When you find some time, it would be very helpful if you could answer the following questions so I can work on improving ASan Nightly further. In particular, many of the reports we saw about use-after-free were sent only once, not multiple times. So I'm asking myself what changes to ASan Nightly I can make to improve overall uaf detection rate:

1) Do you think more frequent GC/CC would have helped detecting this problem more frequently?

2) Do you think GC/CC on specific DOM events (e.g. beforeunload, pagehide, DOMContentLoaded) would have helped?

3) Do you think triggering the event loop in the specific child processes would have helped here? (e.g. dummy XMLHttpRequest onbeforeunload, useful in fuzzing).

4) Do you have any other ideas to reproduce this particular problem better or do you think any of the above points (or other approaches) might help to find other use-after-free issues in areas that you work in?

Any feedback would be greatly appreciated. Thanks!
Flags: needinfo?(jvarga)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release

(In reply to Christian Holler (:decoder) from comment #22)

When you find some time, it would be very helpful if you could answer the
following questions so I can work on improving ASan Nightly further. In
particular, many of the reports we saw about use-after-free were sent only
once, not multiple times. So I'm asking myself what changes to ASan Nightly
I can make to improve overall uaf detection rate:

  1. Do you think more frequent GC/CC would have helped detecting this problem
    more frequently?

Probably not in this case.

  1. Do you think GC/CC on specific DOM events (e.g. beforeunload, pagehide,
    DOMContentLoaded) would have helped?

Probably not in this case.

  1. Do you think triggering the event loop in the specific child processes
    would have helped here? (e.g. dummy XMLHttpRequest onbeforeunload, useful in
    fuzzing).

Probably not in this case.

  1. Do you have any other ideas to reproduce this particular problem better
    or do you think any of the above points (or other approaches) might help to
    find other use-after-free issues in areas that you work in?

No.

Any feedback would be greatly appreciated. Thanks!

I think the attached crash information was enough in this case.

Flags: needinfo?(jvarga)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: