Bugzilla

Jason Kratzer [:jkratzer]

Comment 2

•

7 years ago

Also crashes a Nightly opt build with a null-deref: bp-2b973f39-3394-45b6-aa20-259840170921

Did we fix this UAF on trunk and my crash is something else, or is that just the normal opt symptom for this testcase?

If it doesn't repro on trunk then finding out where this got fixed will help us identify a potential patch to backport.

Group: core-security → layout-core-security

status-firefox57: --- → ?

status-firefox-esr52: --- → unaffected

Flags: needinfo?(jkratzer)

Keywords: csectype-uaf, sec-high

Version: unspecified → 52 Branch

Reporter

Comment 3

•

7 years ago

(In reply to Daniel Veditz [:dveditz] from comment #2)
> Also crashes a Nightly opt build with a null-deref:
> bp-2b973f39-3394-45b6-aa20-259840170921
> 
> Did we fix this UAF on trunk and my crash is something else, or is that just
> the normal opt symptom for this testcase?
> 
> If it doesn't repro on trunk then finding out where this got fixed will help
> us identify a potential patch to backport.

I thought I had tested this against mozilla-central (that's why I submitted it against ESR) but apparently, I did not.  I just confirmed that this triggers the same crash on mozilla-central rev 47f7b6c64265 (2017-09-21).

Flags: needinfo?(jkratzer)

Jet Villegas (inactive)

Comment 4

•

7 years ago

Changing the test case from display:ruby-base-container to display:block prevents the crash. 

Xidorn: WDYT?

Flags: needinfo?(xidorn+moz)

Jet Villegas (inactive)

Updated

•

7 years ago

status-firefox57: ? → fix-optional

status-firefox58: --- → affected

Priority: -- → P2

Ryan VanderMeulen [:RyanVM]

Comment 5

•

7 years ago

On debug builds, this hits:
Assertion failure: aFloat->GetParent() == mBlock || (aFloat->GetStateBits() & NS_FRAME_IS_PUSHED_FLOAT) (float should be in this block unless it was marked as pushed float), at /builds/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:581

Also, this definitely crashes ESR52 builds as well, so re-marking that as affected.

status-firefox55: --- → wontfix

status-firefox56: --- → wontfix

status-firefox57: fix-optional → affected

status-firefox-esr52: unaffected → affected

tracking-firefox57: --- → ?

tracking-firefox58: --- → ?

tracking-firefox-esr52: --- → ?

Keywords: assertion

Assignee

Comment 6

•

7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> On debug builds, this hits:
> Assertion failure: aFloat->GetParent() == mBlock || (aFloat->GetStateBits()
> & NS_FRAME_IS_PUSHED_FLOAT) (float should be in this block unless it was
> marked as pushed float), at
> /builds/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:581

This... sounds like bug 1229437...

I'll have a look.

Assignee: nobody → xidorn+moz

Flags: needinfo?(xidorn+moz)

Assignee

Comment 7

•

7 years ago

Attached file simplified testcase — Details

I tried to simplify the original testcase, but couldn't make it work without having a somehow weird tree structure (column-width on <html>, and <script> is used as text at the same time as being a script, etc.)

So I tried to look at the frame tree and realized that the key is to have two ruby base containers. This is a much simpler testcase built on top of this assumption, which triggers the same crash.

Assignee

Comment 8

•

7 years ago

So this seems to be because that we are pushing children to next-in-flow without reparenting the floats.

My plan of fixing this is
* always push to overflow list rather than push to next-in-flow directly
* reparent floats when stealing overflow frames from prev-in-flow

This will match how nsInlineFrame handles moving children.

I'll investigate whether there is any code that can be shared between nsInlineFrame and the ruby frames.

Assignee

Comment 9

•

7 years ago

Related code in nsInlineFrame:
* push to overflow list: https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/layout/generic/nsInlineFrame.cpp#913-934
* steal from prev-in-flow and reparent: https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/layout/generic/nsInlineFrame.cpp#379-423

Assignee

Comment 10

•

7 years ago

Attached patch part 1 - Move ReparentFloatsForInlineChild from nsInlineFrame to nsContainerFrame — Details — Splinter Review

Attachment #8912505 - Flags: review?(dholbert)

Assignee

Comment 11

•

7 years ago

Attached patch part 2 - Remove some duplicate function comment from nsContainerFrame — Details — Splinter Review

Attachment #8912506 - Flags: review?(dholbert)

Assignee

Comment 12

•

7 years ago

Attached patch part 3 - Add nsContainerFrame::PushChildrenToOverflow — Details — Splinter Review

Attachment #8912507 - Flags: review?(dholbert)

Assignee

Comment 13

•

7 years ago

Attached patch part 4 - Have ruby frames only push children to overflow list, and reparent floats when adopting children from prev-in-flow — Details — Splinter Review

Attachment #8912509 - Flags: review?(dholbert)

Assignee

Comment 14

•

7 years ago

Attached patch part 5 - Add crashtest for this bug — Details — Splinter Review

Attachment #8912510 - Flags: review?(dholbert)

Assignee

Comment 15

•

7 years ago

It is still unclear to me, though, why this can lead to a use-after-poison (or use-after-free).

The stack in comment 0 shows one path to free the frame, but it doesn't show the other path. From the code, I don't see why a mis-parented float would be released twice.

Assignee

Comment 16

•

7 years ago

OK so the problem here is that, the frame is added into multiple frame lists.

This is because nsLineLayout::ReflowFrame tracks the float frame via placeholder frame, and calls into BlockReflowInput::AddFloat for floats it finds. The latter may call into BlockReflowInput::PushFloatPastBreak to push the float into next continuation.

BlockReflowInput::PushFloatPastBreak calls nsBlockFrame::StealFrame to remove the float frame from its current frame list and add it to the pushed float list. Since the float frame is not a children of the block frame being called in this case, the float frame is still referenced by another frame list. It means at the end of BlockReflowInput::PushFloatPastBreak, we would have two frame list referencing the same float frame.

I'm not sure whether having a use-after-free on frame is a serious issue (e.g. do we poison frame memory when we free it?), if it is, probably we want to also do some special handling (or just release assert) in places like nsBlockFrame::StealFrame to avoid creating multiple strong references to a single frame.

Assignee

Comment 17

•

7 years ago

I realized this via commenting out the assertion for
> aFloat->GetParent() == mBlock || (aFloat->GetStateBits() & NS_FRAME_IS_PUSHED_FLOAT)
and rerun the testcase, then I hit another assertion
> aChild->GetParent() == this
which leads to the analysis in comment 16.

It seems that both the original testcase and the simplified one in comment 7 (the testcase I add as crashtest in part 5) can trigger the same series of assertions, which means landing that crashtest has certain risk of exposing the security problem here.

Comment 18

•

7 years ago

Comment on attachment 8912505 [details] [diff] [review]
part 1 - Move ReparentFloatsForInlineChild from nsInlineFrame to nsContainerFrame

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

r=me

Attachment #8912505 - Flags: review?(dholbert) → review+

Comment 19

•

7 years ago

Comment on attachment 8912506 [details] [diff] [review]
part 2 - Remove some duplicate function comment from nsContainerFrame

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

r=me

Attachment #8912506 - Flags: review?(dholbert) → review+

Comment 20

•

7 years ago

Comment on attachment 8912507 [details] [diff] [review]
part 3 - Add nsContainerFrame::PushChildrenToOverflow

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

r=me

Attachment #8912507 - Flags: review?(dholbert) → review+

Comment 21

•

7 years ago

Comment on attachment 8912509 [details] [diff] [review]
part 4 - Have ruby frames only push children to overflow list, and reparent floats when adopting children from prev-in-flow

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

r=me

Attachment #8912509 - Flags: review?(dholbert) → review+

Comment 22

•

7 years ago

Comment on attachment 8912510 [details] [diff] [review]
part 5 - Add crashtest for this bug

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

r=me

Attachment #8912510 - Flags: review?(dholbert) → review+

Comment 23

•

7 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #16)
> I'm not sure whether having a use-after-free on frame is a serious issue
> (e.g. do we poison frame memory when we free it?)

It shouldn't be too serious -- we do indeed poison frame memory when we free it.  So UAF of a nsFrame shouldn't be too bad, as far as I know. (It'll crash safely.)

(Specifically:
 - nsFrame::DestroyFrom() calls shell->FreeFrame() right after invoking the ~nsFrame destructor.
 - That calls nsPresArena::FreeByFrameID
 - ...which calls nsPresArena::Free
 - ...which calls the mfbt mozWritePoison API, here:
https://dxr.mozilla.org/mozilla-central/rev/35fbf14b96a633c3f66ea13c1a163a3f3a4219b9/layout/base/nsPresArena.cpp#160

Assignee

Comment 24

•

7 years ago

Comment on attachment 8912509 [details] [diff] [review]
part 4 - Have ruby frames only push children to overflow list, and reparent floats when adopting children from prev-in-flow

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This is a frame use-after-free. Based on information from comment 23 that frames are poisoned after being freed, it is probably not exploitable.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I don't think the patch itself indicates any security issue (it is very similar to bugs like bug 1229437, which can simply just be a fix to assertion). But it's from a security-sensitive bug can probably be a problem. As indicated in comment 17, the testcase can lead to an assertion (with another assertion commented out) which can indicate this use-after-free.

Which older supported branches are affected by this flaw?
All supported branches.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patchset should mostly be upliftable. I'm not aware of any significant refactor happened in the related code, so if they cannot be applied cleanly, it shouldn't be hard to create backport patch either.

How likely is this patch to cause regressions; how much testing does it need?
It shouldn't be risky, as it doesn't change behavior in general. Only edge case is affected (from crash to normal).

Attachment #8912509 - Flags: sec-approval?

Assignee

Comment 25

•

7 years ago

(Only request sec-approval for part 4 because that's the real fix. All parts before are just some simple refactoring for the fix.)

If this isn't really exploitable, I would suggest that we land the crashtest together with the patches.

Al Billings [:abillings - ex-MoCo]

Comment 26

•

7 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #24)
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> This is a frame use-after-free. Based on information from comment 23 that
> frames are poisoned after being freed, it is probably not exploitable.

In an opt build I crash on a null deref (in 57), and rax and rcx are clearly frame-poisoned values: 0x7ffffffff0dea7ff. I agree with that assessment -- no one has yet discovered a bypass for framepoisoning that we know of.

I crash on ESR-52 but it's not submitting reports. We should double-check that it's the same crash as on 56/57 but I'm not too concerned and we probably don't need the fix on that branch.

Keywords: csectype-uaf, sec-high → csectype-framepoisoning, sec-low

Comment 27

•

7 years ago

Comment on attachment 8912509 [details] [diff] [review]
part 4 - Have ruby frames only push children to overflow list, and reparent floats when adopting children from prev-in-flow

Clearing sec-approval. As a low risk bug, this can just go into trunk.

Attachment #8912509 - Flags: sec-approval?

Assignee

Comment 28

•

7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3f409faf2f631e6a8990b69913738bf0b885f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/14fe3196aa1e00046ecf452e2f4f12965f060817
https://hg.mozilla.org/integration/mozilla-inbound/rev/2164d769710e6470e22c114deca0c92e1fe00dd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/14190aa23ce612d553ee9afd0b4f734e6a168d7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/32eb992ba469bc2b215ecea1f4e7e99e3de5d3bc

Ryan VanderMeulen [:RyanVM]

Assignee

Comment 29

•

7 years ago

Comment on attachment 8912509 [details] [diff] [review]
part 4 - Have ruby frames only push children to overflow list, and reparent floats when adopting children from prev-in-flow

Approval Request Comment
[Feature/Bug causing the regression]: CSS Ruby
[User impact if declined]: may crash in some edge cases
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: just landed on inbound
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: all patches in this bug
[Is the change risky?]: no
[Why is the change risky/not risky?]: it shouldn't change behavior in general, just some edge case. And I believe CSS ruby has a decent test coverage.
[String changes made/needed]: n/a

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

Comment 30

•

7 years ago

https://hg.mozilla.org/mozilla-central/rev/8f3f409faf2f
https://hg.mozilla.org/mozilla-central/rev/14fe3196aa1e
https://hg.mozilla.org/mozilla-central/rev/2164d769710e
https://hg.mozilla.org/mozilla-central/rev/14190aa23ce6
https://hg.mozilla.org/mozilla-central/rev/32eb992ba469

Probably not worth backporting to ESR52 since it's only a sec-low. Let's get it on Beta still, though.

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox58: affected → fixed

status-firefox-esr52: affected → wontfix

tracking-firefox-esr52: ? → ---

Flags: in-testsuite? → in-testsuite+

Resolution: --- → FIXED

Target Milestone: --- → mozilla58

Phil Ringnalda (:philor)

Updated

•

7 years ago

Depends on: 1404179

Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)

Updated

•

7 years ago

tracking-firefox57: ? → +

tracking-firefox58: ? → +

Assignee

Comment 31

•

7 years ago

Comment on attachment 8912509 [details] [diff] [review]
part 4 - Have ruby frames only push children to overflow list, and reparent floats when adopting children from prev-in-flow

looks like it's more risky than I thought.

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

Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)

Updated

•

7 years ago

Group: layout-core-security → core-security-release

Comment 32

•

7 years ago

This seems a wontfix for 57 based on the previous comment. Lemme know if I am wrong. Xidorn, Dan, FYI.

status-firefox57: affected → wontfix

Flags: needinfo?(xidorn+moz)

Flags: needinfo?(dveditz)

Assignee

Comment 33

•

7 years ago

Right, I think it is wontfix for 57 since this is sec-low that just an edge case which could cause crash, and it is more risky given there's a test intermittent introduced by this.

Flags: needinfo?(xidorn+moz)

Al Billings [:abillings - ex-MoCo]

Updated

•

7 years ago

tracking-firefox57: + → -

Flags: needinfo?(dveditz)

Updated

•

6 years ago

Whiteboard: [adv-main58+]

Matt Wobensmith [:mwobensmith][:matt:]

Updated

•

6 years ago

Flags: qe-verify-

Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]