Bugzilla

Assignee

Comment 2

•

8 years ago

In a debug build, this triggers the following assertion-failure:
> Assertion failure: mMargin.ConvertsToLength(), at layout/style/nsStyleStruct.h:839

Indeed, mMargin does *not* convert to length; it has these units:
>{nsStyleUnit::eStyleUnit_Coord, nsStyleUnit::eStyleUnit_Auto, 
>  nsStyleUnit::eStyleUnit_Coord, nsStyleUnit::eStyleUnit_Auto}
...which match the specified style in the testcase.

We're querying the margin via nsIFrame::GetUsedMargin (as shown in comment 0), which normally would grab a cached value out of the frame's property-table. But in this case, there is no cached margin in the property table, so we're falling back to trying to force the style struct's "mMargin" into length values, and failing.

I'll take a closer look later on today.  Assigning.

Assignee: nobody → dholbert

Assignee

Updated

•

8 years ago

Flags: needinfo?(dholbert)

Keywords: assertion

Summary: Heap-buffer-overflow in nsIFrame::GetUsedMargin → Heap-buffer-overflow in nsIFrame::GetUsedMargin (or in debug build: "Assertion failure: mMargin.ConvertsToLength(), at layout/style/nsStyleStruct.h:839")

Assignee

Updated

•

8 years ago

Attachment #8752600 - Attachment description: margin.html → testcase 1 (margin.html)

Assignee

Comment 3

•

8 years ago

Attached file testcase 2 (no scripting) — Details

Here's a second testcase, with no JS and with less CSS.  
This still aborts my debug build when loaded, with the same fatal assert.

The key CSS needed to trigger the issue here is the following, set on a single div:
 margin-bottom: 50%; /* Can be any margin property; set to % or auto. */
 display: -moz-box;  /* Or some other XUL display type. */
 direction: rtl;

Assignee

Comment 4

•

8 years ago

Mm, looks like this GetMarginNoPercentage API (and its assertion that's failing here) appeared recently in bug 1269901. Pretty sure this was a regression from that bug.

Blocks: 1269901

Assignee

Comment 5

•

8 years ago

In a build with bug 1269901 backed out, I get this (non-fatal) assertion and no crash/corruption AFAICT:
###!!! ASSERTION: We should have a margin here! (out of memory?): 'hasMargin', file layout/generic/nsFrame.cpp, line 971

bug 1269901 removed that assertion, and proceeded with the assumption that it should pass.  But, it turns out we have several other bugs filed on testcases that are known to trigger this assertion, some of them super-ancient: bug 391157, bug 586628, bug 731350.

Those bugs' testcases are all now crashes with an uninitialized-memory-read, as of bug 1269901 landing.

Comment 6

•

8 years ago

Ok - maybe we should just do GetMargin at that callsite instead?

Assignee

Comment 7

•

8 years ago

Or, we could add a safe bail-out in GetMarginNoPercentage, which would safely return a trivial margin if we unexpectedly get a call with non-px-convertable values.

We wouldn't expect to hit that bail-out clause, but the testcases here & the bugs on mentioned in comment 5 would hit it for now (and return safely instead of crashing), until they're fixed.

Assignee

Comment 8

•

8 years ago

Oh, I guess that's kinda what GetMargin() would let us do. Yeah, that seems reasonable.

Assignee

Comment 9

•

8 years ago

Attached patch fix v1 (obsolete) — Details — Splinter Review

EXPLANATION OF PATCH:
This patch restores a piece of fallback behavior we had before bug 1269901 landed.

Right now, mozilla-central has a GetMarginNoPercentage() call here, which blindly asserts that the margin's nsStyleCoords can be converted to pixel values. (and calls ToLength() on them, which causes an uninitialized-memory-read if they actually represent the value "auto" and haven't initialized the piece of the nsStyleCoord where the length is stored)

This patch converts that call to a GetMargin() call, which will explicitly fail (return false) if there are non-length-convertable sides.  Then we can handle that by spamming an assertion, as we used to do via this old "We should have a margin here! (out of memory?)" assertion (removed in bug 1269901):
https://hg.mozilla.org/mozilla-central/rev/581279cd287e#l2.12

Attachment #8753694 - Flags: review?(dbaron)

Assignee

Comment 10

•

8 years ago

So, this patch should take us back to a state where the testcases on various bugs [1] will simply trip a non-fatal assertion & maybe giving us an incorrectly-0-sized margin value at some particular GetUsedMargin callsite, instead of causing an uninitialized-memory-read and crashing (current mozilla-central behavior).

The assertion failures for these testcases are all probably worth investigating & fixing in their own right, but I'm not sure that should be a priority right now.  They've been causing nonfatal assertions for ages, and they seem to require weird scenarios (like mixing XUL display types with HTML) and they don't seem to have caused trouble for real-world sites.

[1] e.g. bug 391157, bug 586628, bug 731350, and this bug here.

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

Assignee

Updated

•

8 years ago

Keywords: crash

Summary: Heap-buffer-overflow in nsIFrame::GetUsedMargin (or in debug build: "Assertion failure: mMargin.ConvertsToLength(), at layout/style/nsStyleStruct.h:839") → Heap-buffer-overflow & crash in nsIFrame::GetUsedMargin (or in debug build: "Assertion failure: mMargin.ConvertsToLength(), at layout/style/nsStyleStruct.h:839")

Comment 11

•

8 years ago

Tracking for 49 since this is sec-critical. Does this affect earlier versions?

status-firefox48: --- → ?

tracking-firefox49: --- → +

Flags: needinfo?(dholbert)

Assignee

Comment 12

•

8 years ago

No, this only affects Firefox 49 (regressed by bug 1272983).

Flags: needinfo?(dholbert)

Keywords: regression, testcase

David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)

Comment 13

•

8 years ago

Note that this was also reported in bug 1273360.

Comment 14

•

8 years ago

Comment on attachment 8753694 [details] [diff] [review]
fix v1

r=dbaron, but I suspect this isn't the only caller we need to fix.  Could you look through the others?

Attachment #8753694 - Flags: review?(dbaron) → review+

Updated

•

8 years ago

Group: core-security → layout-core-security

Assignee

Comment 15

•

8 years ago

(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #14)
> I suspect this isn't the only caller we need to fix.  Could
> you look through the others?

Good point; AFAICT, none of the calls to GetMarginNoPercentage / GetPaddingNoPercentage have any reason to believe that "no percentage" is guaranteed.

I suspect we should probably make these getters private and fix all of the callers to use GetMargin/GetPadding.

Assignee

Comment 16

•

8 years ago

Attached file testcase 3 (using "margin: auto" in a button's -moz-focus-inner rule) — Details

This testcase crashes due to the other GetMarginNoPercentage callsite.  (And if I use GetPaddingNoPercentage instead, it produces a broken layout instead of crashing.)

Assignee

Comment 17

•

8 years ago

Attached patch part 1, v2: If GetUsed{Margin,Padding} get called for a value that can't be resolved & isn't cached, assert & return 0-sized nsMargin. (obsolete) — Details — Splinter Review

This is the same patch you already reviewed, except:
 * I've now made the same change in nsIFrame::GetUsedPadding.
 * I've included my reduced testcase ("testcase 2") as a crashtest, as well as a variant that uses a percent padding, which exercises the newer piece of this patch.

Each of these crashtests trip 2 copies of the relevant NS_ERROR that I'm adding in this patch (in GetUsed{Margin|Padding}).  Hence, I'm tagging them as "asserts(2)".

(I may need to broaden that asserts annotation a bit once I've tested this on Android, because IIRC android reftests do an extra reflow and get extra copies of assertions as a result.)

Attachment #8753694 - Attachment is obsolete: true

Attachment #8754061 - Flags: review?(dbaron)

Assignee

Comment 18

•

8 years ago

Attached patch part 1, v3: If GetUsed{Margin,Padding} get called for a value that can't be resolved & isn't cached, assert & return 0-sized nsMargin. — Details — Splinter Review

(Sorry, forgot to qref; reposting.)

Attachment #8754061 - Attachment is obsolete: true

Attachment #8754061 - Flags: review?(dbaron)

Attachment #8754062 - Flags: review?(dbaron)

Assignee

Comment 19

•

8 years ago

Attached patch part 2: Use more forgiving margin/padding getters in nsButtonFrameRenderer, for better behavior when percent or auto values are encountered (obsolete) — Details — Splinter Review

This "part 2" patch fixes up the callsites that are related to handling margin + padding in :-moz-focus-{inner,outer} rules for buttons.

In these cases, I'm not bothering to handle failure (or spam NS_ERRORs or anything), because this is just the behavior we've always had and this is a prefixed implementation detail anyway.

The pseudo-class is technically web-exposed & usable by authors, so there's an argument to be made that maybe we should try to do something more graceful. (e.g. only collapsing away the margin/padding-components that actually have a percent or auto value, instead of collapsing away all four sides).  But I'm not sure that this pseudo-class gets used enough (let alone with broken margin/padding values) to make this worth it.

So, this patch just aims to return us to the behavior we had before bug 1269901 (just behaving as if these nsMargins were entirely 0, when there's any percent or auto component).  I've included reftests to test this behavior, and I've verified that the testcases & reference cases match in current Firefox release -- i.e. I've verified that this patch does indeed restore our prior behavior.

Attachment #8754072 - Flags: review?(dbaron)

Assignee

Comment 20

•

8 years ago

Attached patch part 3: Use more forgiving padding getter in nsTreeBodyFrame.cpp, for better behavior with percent padding — Details — Splinter Review

This fixes the last of these Get{Margin|Padding}NoPercentage() calls -- in nsTreeBodyFrame.cpp.

As with earlier patches, this restores our earlier behavior (just treating any "padding" that involves a percentage as if it were 0 on all 4 sides).

I've included a simple reftest, which triggers the nsStylePadding::GetPaddingNoPercentage fatal assertion in an unpatched build.

Attachment #8754094 - Flags: review?(dbaron)

Comment 21

•

8 years ago

Thanks a ton for digging into this dholbert! Sorry for the mess :-(

Assignee

Comment 22

•

8 years ago

No worries! Without preexisting reftest coverage for these scenarios, it's understandable that you'd be able to trigger this sort of thing by accident without noticing.

Assignee

Comment 23

•

8 years ago

Attached patch part 4: Remove the "NoPercentage" getters (fold them into main getters) (obsolete) — Details — Splinter Review

With all of the external GetPaddingNoPercentage/GetMarginNoPercentage callsites removed (in earlier patches here), we might as well just fold these functions into the main getters (their sole remaining callsites), since they've proven themselves to be footguns and there's no clear benefit to keeping them around.

So, this patch does that.  I'm flipping the logic in GetPadding/GetMargin so that they fail early, and then I fold in the body of the corresponding "NoPercentage" getter after that.

(This patch doesn't affect behavior at all; it's simply folding functions together.)

Attachment #8754101 - Flags: review?(dbaron)

Assignee

Comment 24

•

8 years ago

Attached patch part 4, v2: Remove the "NoPercentage" getters for margin/padding, and fold their impls into the main getters — Details — Splinter Review

(Sorry -- the just-attached "part 4" had a stale commit message. I've fixed it now to actually describe what the patch is doing.)

Attachment #8754101 - Attachment is obsolete: true

Attachment #8754101 - Flags: review?(dbaron)

Attachment #8754102 - Flags: review?(dbaron)

Comment 25

•

8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #23)
> Created attachment 8754101 [details] [diff] [review]
> part 4: Remove the "NoPercentage" getters (fold them into main getters)
> 
> With all of the external GetPaddingNoPercentage/GetMarginNoPercentage
> callsites removed (in earlier patches here), we might as well just fold
> these functions into the main getters (their sole remaining callsites),
> since they've proven themselves to be footguns and there's no clear benefit
> to keeping them around.

Well, this code was super-hot, to the extent that I regressed it on CART+TART when I implemented them in a slightly-less-than-optimal way. So I would be wary of the overhead of unconditionally calling ConvertsToLength in all the callpaths...

Comment 26

•

8 years ago

But I guess it's possible that my optimization was enough. Talos will tell - might be worth giving jmaher a heads-up about it?

Assignee

Comment 27

•

8 years ago

(In reply to Bobby Holley (busy) from comment #25)
> Well, this code was super-hot, to the extent that I regressed it on
> CART+TART when I implemented them in a slightly-less-than-optimal way. So I
> would be wary of the overhead of unconditionally calling ConvertsToLength in
> all the callpaths...

Hmm.  The testcases on this bug (and included in the patch) indicate that we *have* to check ConvertsToLength in all of these callpaths, though.  (Or, we have to fix the callers to be know ahead of time that they shouldn't be calling into this API. Or something else.)

Here's a Try run with unit-test results, anyway (not talos yet at this point)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be52eb2d9fce

(I dropped the bug number from the commit message, to make it slightly less obvious to hackers-watching-Try that these are patches for a bug that's marked as security-sensitive. The bug number is still in the unit tests inside of the patch, so it's still discoverable. Not a huge deal if a bit of this bug leaks, in any case, since this is Nightly-only and it's an uninitialized-memory-read, which might cause data leakage but probably isn't exploitable.)

Assignee

Comment 28

•

8 years ago

(In reply to Bobby Holley (busy) from comment #25)
> Well, this code was super-hot, to the extent that I regressed it on
> CART+TART when I implemented them in a slightly-less-than-optimal way.

(For my later reference, these regressions were tracked in bug 1270515)

Assignee

Comment 29

•

8 years ago

Below are two Try runs that should reveal whether this imposes a CART/TART regression, based on the try syntax provided in bug 1270515's current "user story" field.

* "Before" (without this bug's patches):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=de21bc040281
* "After" (with this bug's patches):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcc68b2dc84d

Assignee

Updated

•

8 years ago

status-firefox48: ? → unaffected

Al Billings [:abillings - ex-MoCo]

Assignee

Comment 30

•

8 years ago

If I'm reading the graphs right, I think the talos results from comment 29's try runs indicate that this is unlikely to impact CART/TART (delta of < 1%, within the standard deviation of one or the other endpoint, and "low" confidence rating):
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=de21bc040281&newProject=try&newRevision=dcc68b2dc84d&framework=1&showOnlyImportant=0

Updated

•

8 years ago

Flags: sec-bounty?

David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)

Assignee

Comment 31

•

8 years ago

Attached patch part 2, v2: Use more forgiving margin/padding getters in nsButtonFrameRenderer, for better behavior when percent or auto values are encountered — Details — Splinter Review

(Making a small tweak to reftests in "part 2", to fix a failure in my first try push that was happening due to font-size differences inside vs. outside the button.)

Attachment #8754072 - Attachment is obsolete: true

Attachment #8754072 - Flags: review?(dbaron)

Attachment #8754567 - Flags: review?(dbaron)

Updated

•

8 years ago

Attachment #8754062 - Flags: review?(dbaron) → review+

David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)

Updated

•

8 years ago

Attachment #8754567 - Flags: review?(dbaron) → review+

David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)

Updated

•

8 years ago

Attachment #8754094 - Flags: review?(dbaron) → review+

David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)

Updated

•

8 years ago

Attachment #8754102 - Flags: review?(dbaron) → review+

Assignee

Comment 32

•

8 years ago

Thanks for the reviews! I'll land once my updated Try run is complete:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=935e55a0a463

Assignee

Comment 34

•

8 years ago

Landed this on Friday (5/20), forgot to post csets. (Forgot that the bot doesn't post on closed bugs.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/66e13bb5f4c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c76ec44e0c23
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cab985e78c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e01296aaeac0

Looks like inbound hasn't merged to central yet, though, so this bug is still open.

Comment 35

•

8 years ago

Daniel: this was marked sec-critical early on based on the ASAN traces, but looking at the patch it looks like you're preventing the use of mLength from an uninitialized nsStyleCoord. If that's true won't this simply lead to wacky but harmless layout if it doesn't crash due to the access violation?

Flags: needinfo?(dholbert)

Keywords: sec-critical → csectype-uninitialized

Assignee

Comment 36

•

8 years ago

You're correct -- this bug allowed for layout to be influenced by uninitialized data inside of a tagged union (in nsStyleCoord).

As I understand it, that means the worst that could happen here (aside from DOS) is data leakage -- e.g. an attacker could perhaps set things up such that the stale data in this uninitialized nsStyleCoord might reveal something about previously visited sites or other tabs or something. (Not sure if that's actually possible, but I think that's the worst attack scenario here.)

Flags: needinfo?(dholbert)

Assignee

Comment 37

•

8 years ago

Specifically, when this bug is triggered:
 - We get a ToLength() call on an "auto" (or percent) typed nsStyleCoord.
 - This triggers this code (assertions removed for brevity), taking the "AsCalcValue" path at the end there:

> 187   static nscoord ToLength(nsStyleUnit aUnit, nsStyleUnion aValue) {
> 189     if (aUnit == eStyleUnit_Coord) {
> 190       return aValue.mInt;
> 191     }
> 193     return AsCalcValue(aValue)->ToLength();
> 194   }
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleCoord.h?rev=b17cd6fd7cd0#187

 - AsCalcValue() casts our tagged-union to a Calc* pointer.
 - Its ToLength() method (not virtual) reads the "nscoord mLength" value off of the Calc value:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleCoord.h?rev=b17cd6fd7cd0#94

So, comment 36 is still correct, but it goes one layer deeper than I was implying there. (An attacker can read a 32-bit value (nscoord) out of the address in memory which is pointed to by the uninitialized union value in a nsStyleCoord's tagged union.)

If the attacker sets the nsStyleCoord to a percent unit, then they can precisely control that pointer address (since, unlike "auto", a percent unit will store an actual numeric value in the tagged union.)

Assignee

Comment 38

•

8 years ago

So, in short, this bug probably probably allows an attacker to read the contents of memory, at attacker-controlled memory addresses.
 (1) Choosing the memory location by setting a percentage margin and triggering this bug.
 (2) Reading the memory by looking at the impact on layout.
 (3) [repeat, to continue reading as much memory as they like]

Assignee

Comment 39

•

8 years ago

(I filed bug 1275067 on making ToLength a bit less footgunny -- so that it only calls AsCalc() in cases where we actually know we have a calc unit.)

Carsten Book [:Tomcat]

Comment 40

•

8 years ago

https://hg.mozilla.org/mozilla-central/rev/e01296aaeac0
https://hg.mozilla.org/mozilla-central/rev/7cab985e78c8
https://hg.mozilla.org/mozilla-central/rev/c76ec44e0c23
https://hg.mozilla.org/mozilla-central/rev/66e13bb5f4c0

Status: NEW → RESOLVED

Closed: 8 years ago

status-firefox49: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla49

Updated

•

8 years ago

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

Updated

•

8 years ago

Group: core-security-release

status-firefox-esr45: --- → unaffected

Keywords: sec-moderate