Style struct may refer to removed CounterStyle object

RESOLVED FIXED in Firefox 34, Firefox OS v2.1

Status

()

Core
CSS Parsing and Computation
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: xidorn, Assigned: mats)

Tracking

({csectype-uaf, regression, sec-high})

Trunk
mozilla36
csectype-uaf, regression, sec-high
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 wontfix, firefox34+ fixed, firefox35+ fixed, firefox36 fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main34+] bug 1077718 protects with frame poisoning, backported to Fx34)

Attachments

(5 attachments, 1 obsolete attachment)

Per bug 1075336 comment 11, there is some other bug other than the lifetime management problem of CounterStyle, which causes bug 1075336.

In normal case, when any change of styles affect any CounterStyle object, style structs should be reconstructed so that all style structs will use the latest information of counter styles. Hence, when that happens, no style struct should refer to removed CounterStyle objects anymore.

However, bug 1075336 uncovers that there is cases that after a CounterStyle object is remove by the CounterStyleManager, someone could still be using it.
Keywords: csectype-uaf, sec-high
(Assignee)

Comment 1

3 years ago
With bug 1077718 fixed I think any use-after-free here would be protected by
the poisoning that the arena does.

Xidorn, do you think that the derived class that we didn't arena-allocate
(AnonymousCounterStyle), b/c it's ref-counted anyway, poses any risk for
use-after-free here?
Severity: normal → critical
Flags: needinfo?(quanxunzhen)
If the ref-count mechanism is reliable, I don't think AnonymousCounterStyle poses use-after-free risk here. But as what David said in bug 1075336 comment 11, that bug indicates some other unknown bug.
Flags: needinfo?(quanxunzhen)
(Assignee)

Comment 3

3 years ago
OK, it seems to me sec-high is a bit over-pessimistic then, but I guess we should
assume the worst until we know exactly what the problem is.

This bug is rather vague to me; Xidorn, could you debug this and provide more
details about what the error condition is and how to reproduce that error
condition, with relevant details about the sequence of events that leads
us there and why the style system is at fault for that condition to occur?
Assignee: nobody → quanxunzhen
Created attachment 8511799 [details]
testcase

The very simplified testcase of this bug.

The conditions to trigger this bug are:
1. <style>#1 with "@counter-style disc {...}" and "li { float: left/right; }"
2. <style>#1 is in <li>#1
2. another non-empty <li> exists
3. the <li>#1 is removed dynamically

setTimeout in the file is not necessary, but it seems that, without a delay, sometimes this bug won't be triggered.
Created attachment 8511801 [details] [diff] [review]
patch for explicit crash when the bug appears

Apply this patch to crash the application when the bug appears.
Assignee: quanxunzhen → nobody
BTW, I think attachment 8511799 [details] could be the crashtest for bug 1075336.
(Assignee)

Comment 7

3 years ago
Created attachment 8512047 [details]
stack

Thanks, that makes it clear what the issue is.

In nsPresContext::FlushCounterStyles()
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp?rev=968aa79b1200#2203
we first call 1. NotifyCounterStylesAreDirty() which is what removes the
style from mCacheTable in the CounterStyleManager (synchronously) and
then 2. PostRebuildAllStyleDataEvent which posts a request to rebuild
all style data at the next refresh driver tick (asynchronously) .

This is the stack with Xidorn's patch above, indicating the bad access.
We entered ProcessPendingRestyles with mRebuildAllStyleData == true
which indicates that 2 hasn't happened yet, so we're operating with
stale style data.
(Assignee)

Comment 8

3 years ago
Created attachment 8512052 [details] [diff] [review]
wip

I don't think ProcessPendingRestyles should process individual restyles when
there's a pending request to rebuild all style data.  It should just rebuild
all style data directly.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee12645c45f6
Assignee: nobody → mats
(Assignee)

Comment 9

3 years ago
Interesting, the patch triggers the following assertion on Android 4.0 (but nowhere else?!):

ASSERTION: NS_BLOCK_HAS_FIRST_LETTER_STYLE state out of sync: 'haveFirstLetterStyle == ((mState & NS_BLOCK_HAS_FIRST_LETTER_STYLE) != 0)', file /builds/slave/try-and-d-00000000000000000000/build/layout/generic/nsBlockFrame.cpp, line 6540

layout/reftests/forms/button/first-letter-1.html  (4 assertions)
layout/reftests/forms/button/first-letter-1-ref.html (2 assertions)

I believe this is an existing bug that is uncovered by the patch.
Not really a layout bug though, just a false positive assertion.
New Try run with that fixed:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5518af68d629
(Assignee)

Comment 10

3 years ago
Created attachment 8512703 [details] [diff] [review]
fix

(I've submitted a fix for the first-letter assertions separately in bug 399262.)

FWIW, the patch does cause a log warning on Android to occur more often
than before: "W/GeckoFaviconDecoder( 2100): Can't decode null data: URI."
I have no explanation for that currently; it's investigated/fixed in bug 1089940.

It also tickles the underlying bug behind the assertions in bug 1019192
slightly differently which is the reason for the assert count changes.

Green on Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=62388857169b
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f74fe4424784
Attachment #8512052 - Attachment is obsolete: true
Attachment #8512703 - Flags: review?(roc)
Attachment #8512703 - Flags: review?(roc) → review+
Comment on attachment 8512703 [details] [diff] [review]
fix

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

::: layout/base/RestyleManager.cpp
@@ +1533,5 @@
> +    RebuildAllStyleData(nsChangeHint(0), nsRestyleHint(0));
> +    MOZ_ASSERT(mPendingRestyles.Count() == 0);
> +    return;
> +  }
> +

It seems that there is effectively same code at the very end of this function, should we delete that code? If not, I guess we may need some comments about that?
(Assignee)

Comment 12

3 years ago
BTW, the "W/GeckoFaviconDecoder" thing turned out to be an unrelated issue - my Try push
was based on a rev that was buggy.  I'll provide the details in bug 1089940.
(Assignee)

Comment 13

3 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #11)
> It seems that there is effectively same code at the very end of this
> function, should we delete that code? 

No, because 'mRebuildAllStyleData' could have been set true during the
course of this method.

> If not, I guess we may need some comments about that?

What do you think it should say?  The code itself already implies the
above (even more so after my patch) so adding a comment that just points
that out seems redundant.
(Assignee)

Comment 14

3 years ago
Comment on attachment 8512703 [details] [diff] [review]
fix

>[Security approval request comment]
>How easily could an exploit be constructed based on the patch?

I don't think the patch reveals anything specific, other than "restyling".

>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?

Gecko 33 and later.

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

bug 966166.

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

Same patch should apply on all affected branches.

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

Hard to say; I'm guessing medium risk for regressions.  No special testing needed,
this path is well covered by our tests.

A note on the sec rating: I'm pretty sure it's not exploitable in v36 where we
have bug 1077718 because the arena poisoning will protect us.  We might want to
uplift that patch too as a belt-and-suspenders fix, because I have some doubts
that my patch here fixes the problem completely.  I'm pretty sure though, that
it would be a non-exploitable crash with that poisoning.
Attachment #8512703 - Flags: sec-approval?
(Assignee)

Comment 15

3 years ago
> because I have some doubts that my patch here fixes the problem completely

The reason for that is that, as described in comment 7, we're setting the
style data in an invalid state and then waiting for an asynchronous event
to correct that later.  My patch here restores the valid state in
ProcessPendingRestyles(), which probably is the next thing that happens
after setting the invalid state, but I'm not entirely sure something else
couldn't slip in and use this invalid data before that.  It seems unlikely,
but I can't prove it's impossible.

It seems to me we should take another look at the fragile design here and
see if we can avoid the invalid state altogether.  I think the patch here
is good in any case.
Blocks: 966166
Keywords: regression
Whiteboard: Less severe in Fx36 because bug 1077718 adds frame poisoning
> A note on the sec rating: I'm pretty sure it's not exploitable in v36 where we
> have bug 1077718 because the arena poisoning will protect us.

Fair enough, but in terms of tracking severity for our advisory writing about bugs affecting Firefox 33 users the lack of both bugs equals sec-high. I'd rather leave this one sec-high than move it to the mitigation bug (which is public).

> We might want to uplift that patch too as a belt-and-suspenders fix

Looks like this was done earlier today -- thanks for pushing for it.
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox-esr31: --- → unaffected
tracking-firefox34: --- → +
tracking-firefox35: --- → +
Whiteboard: Less severe in Fx36 because bug 1077718 adds frame poisoning → bug 1077718 protects with frame poisoning, backported to Fx34
Comment on attachment 8512703 [details] [diff] [review]
fix

sec-approval+
a=dveditz for landing on aurora and beta
Attachment #8512703 - Flags: sec-approval?
Attachment #8512703 - Flags: sec-approval+
Attachment #8512703 - Flags: approval-mozilla-beta+
Attachment #8512703 - Flags: approval-mozilla-aurora+
status-b2g-v1.4: affected → unaffected
status-b2g-v2.0: affected → unaffected
status-b2g-v2.0M: affected → unaffected
(Assignee)

Comment 18

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2edb76fa2c0

@sheriff I think we should let this bake for a few days before landing on branches
status-firefox36: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/d2edb76fa2c0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(In reply to Mats Palmgren (:mats) from comment #18)
> @sheriff I think we should let this bake for a few days before landing on
> branches

Long enough now?
Flags: needinfo?(mats)
(Assignee)

Comment 21

3 years ago
Yeah, go ahead and land it please.  Thanks.
Flags: needinfo?(mats)
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb4e1d969e8d
https://hg.mozilla.org/releases/mozilla-beta/rev/fdb8b52bea5c
status-b2g-v2.2: affected → fixed
status-firefox34: affected → fixed
status-firefox35: affected → fixed
Backed out from beta for bustage.
https://hg.mozilla.org/releases/mozilla-beta/rev/5b4bac2ebf6c

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=126309&repo=mozilla-beta
status-firefox34: fixed → affected
Flags: needinfo?(mats)
(Assignee)

Comment 24

3 years ago
Created attachment 8517597 [details] [diff] [review]
patch for beta

Sorry about that, here's the correct patch for beta.
(trivial change, so no need for re-review)
Flags: needinfo?(mats) → needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-beta/rev/4e78f69ca4a9
status-firefox34: affected → fixed
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/4e78f69ca4a9
status-b2g-v2.1: affected → fixed
Whiteboard: bug 1077718 protects with frame poisoning, backported to Fx34 → [adv-main34+] bug 1077718 protects with frame poisoning, backported to Fx34

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.