Closed Bug 1272983 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- unaffected
firefox49 + fixed
firefox-esr45 --- unaffected

People

(Reporter: inferno, Assigned: dholbert)

References

Details

(6 keywords)

Attachments

(7 files, 4 obsolete files)

240 bytes, text/html
Details
256 bytes, text/html
Details
142 bytes, text/html
Details
3.32 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.27 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.30 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
12.16 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
==8392==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60d000000000 at pc 0x7f2dddd1ccf6 bp 0x7ffef05c2420 sp 0x7ffef05c2418
READ of size 4 at 0x60d000000000 thread T0 (Web Content)
    #0 0x7f2dddd1ccf5 in ToLength objdir-ff-asan/dist/include/nsStyleCoord.h:96:14
    #1 0x7f2dddd1ccf5 in ToLength objdir-ff-asan/dist/include/nsStyleCoord.h:193
    #2 0x7f2dddd1ccf5 in ToLength objdir-ff-asan/dist/include/nsStyleCoord.h:317
    #3 0x7f2dddd1ccf5 in GetMarginNoPercentage layout/style/nsStyleStruct.h:841
    #4 0x7f2dddd1ccf5 in nsIFrame::GetUsedMargin() const layout/generic/nsFrame.cpp:969
    #5 0x7f2dddc3f72a in nsIFrame::GetLogicalUsedMargin(mozilla::WritingMode) const layout/generic/nsIFrame.h:967:49
    #6 0x7f2ddd9b2bf5 in nsBidiPresUtils::RepositionFrame(nsIFrame*, bool, int, nsTHashtable<nsFrameContinuationState> const*, mozilla::WritingMode, bool, nsSize const&) layout/base/nsBidiPresUtils.cpp:1568:31
    #7 0x7f2ddd9ae5bf in nsBidiPresUtils::RepositionInlineFrames(BidiLineData*, mozilla::WritingMode, nsSize const&, int) layout/base/nsBidiPresUtils.cpp:1691:14
    #8 0x7f2ddd9ad7b3 in nsBidiPresUtils::ReorderFrames(nsIFrame*, int, mozilla::WritingMode, nsSize const&, int) layout/base/nsBidiPresUtils.cpp:1276:10
    #9 0x7f2dddc2d085 in nsLineLayout::TextAlignLine(nsLineBox*, bool) layout/generic/nsLineLayout.cpp:3239:5
    #10 0x7f2dddc93a17 in nsBlockFrame::PlaceLine(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFloatManager::SavedState*, mozilla::LogicalRect&, int&, bool*) layout/generic/nsBlockFrame.cpp:4507:3
    #11 0x7f2dddc91a3b in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) layout/generic/nsBlockFrame.cpp:4016:12
    #12 0x7f2dddc875ae in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3755:9
    #13 0x7f2dddc77547 in ReflowLine layout/generic/nsBlockFrame.cpp:2753:5
    #14 0x7f2dddc77547 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2289
    #15 0x7f2dddc701c3 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1189:3
    #16 0x7f2dddc8e164 in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:306:3
    #17 0x7f2dddc83c6a in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3391:7
    #18 0x7f2dddc77560 in ReflowLine layout/generic/nsBlockFrame.cpp:2750:5
    #19 0x7f2dddc77560 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2289
    #20 0x7f2dddc701c3 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1189:3
    #21 0x7f2dddcd0cac in ReflowChild layout/generic/nsContainerFrame.cpp:1022:3
    #22 0x7f2dddcd0cac in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsCanvasFrame.cpp:644
    #23 0x7f2dddd7ea7d in ReflowChild layout/generic/nsContainerFrame.cpp:1022:3
    #24 0x7f2dddd7ea7d in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) layout/generic/nsGfxScrollFrame.cpp:541
    #25 0x7f2dddd8031a in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) layout/generic/nsGfxScrollFrame.cpp:653:3
    #26 0x7f2dddd8277b in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsGfxScrollFrame.cpp:887:3
    #27 0x7f2dddce96ad in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) layout/generic/nsContainerFrame.cpp:1065:3
    #28 0x7f2dddf40377 in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsViewportFrame.cpp:313:7
    #29 0x7f2dddbaf31d in PresShell::DoReflow(nsIFrame*, bool) layout/base/nsPresShell.cpp:9514:3
    #30 0x7f2dddbc335d in PresShell::ProcessReflowCommands(bool) layout/base/nsPresShell.cpp:9687:24
    #31 0x7f2dddbc224e in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) layout/base/nsPresShell.cpp:4103:11
    #32 0x7f2dddafed8c in nsDocumentViewer::LoadComplete(nsresult) layout/base/nsDocumentViewer.cpp:928:5
    #33 0x7f2dde8f9bfc in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) docshell/base/nsDocShell.cpp:7517:5
    #34 0x7f2dde8f5edf in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) docshell/base/nsDocShell.cpp:7331:7
    #35 0x7f2dde8fce6f in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) docshell/base/nsDocShell.cpp:7228:13
    #36 0x7f2dd895e71a in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) uriloader/base/nsDocLoader.cpp:1250:3
    #37 0x7f2dd895d355 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) uriloader/base/nsDocLoader.cpp:834:5
    #38 0x7f2dd8959eac in nsDocLoader::DocLoaderIsEmpty(bool) uriloader/base/nsDocLoader.cpp:724:9
    #39 0x7f2dd895bfa5 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) uriloader/base/nsDocLoader.cpp:608:5
    #40 0x7f2dd895cf5c in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) uriloader/base/nsDocLoader.cpp:464:14
    #41 0x7f2dd6be7069 in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) netwerk/base/nsLoadGroup.cpp:633:18
    #42 0x7f2dd986a073 in nsDocument::DoUnblockOnload() dom/base/nsDocument.cpp:9192:7
    #43 0x7f2dd9869a21 in nsDocument::UnblockOnload(bool) dom/base/nsDocument.cpp:9120:9
    #44 0x7f2dd983943c in nsDocument::DispatchContentLoadedEvents() dom/base/nsDocument.cpp:5270:3
    #45 0x7f2dd990fd42 in applyImpl<nsDocument, void (nsDocument::*)()> objdir-ff-asan/dist/include/nsThreadUtils.h:707:12
    #46 0x7f2dd990fd42 in apply<nsDocument, void (nsDocument::*)()> objdir-ff-asan/dist/include/nsThreadUtils.h:713
    #47 0x7f2dd990fd42 in nsRunnableMethodImpl<void (nsDocument::*)(), true, false>::Run() objdir-ff-asan/dist/include/nsThreadUtils.h:741
    #48 0x7f2dd6a02554 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1073:7
    #49 0x7f2dd6a84478 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:290:10
    #50 0x7f2dd78343a1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:98:21
    #51 0x7f2dd77a4ea8 in RunInternal ipc/chromium/src/base/message_loop.cc:233:3
    #52 0x7f2dd77a4ea8 in RunHandler ipc/chromium/src/base/message_loop.cc:226
    #53 0x7f2dd77a4ea8 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:206
    #54 0x7f2ddd1c9f9f in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:156:3
    #55 0x7f2ddf372077 in XRE_RunAppShell toolkit/xre/nsEmbedFunctions.cpp:802:12
    #56 0x7f2dd77a4ea8 in RunInternal ipc/chromium/src/base/message_loop.cc:233:3
    #57 0x7f2dd77a4ea8 in RunHandler ipc/chromium/src/base/message_loop.cc:226
    #58 0x7f2dd77a4ea8 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:206
    #59 0x7f2ddf3716e0 in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:637:7
    #60 0x4fcfdb in content_process_main(int, char**) ipc/contentproc/plugin-container.cpp:237:19
    #61 0x7f2dd3b71ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287
AddressSanitizer can not describe address in more detail (wild memory access suspected).
SUMMARY: AddressSanitizer: heap-buffer-overflow (/mnt/scratch0/clusterfuzz/slave-bot/builds/linux_asan_firefox/custom/firefox/libxul.so+0x91d4cf5)
Shadow bytes around the buggy address:
  0x0c1a7fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1a7fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1a7fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1a7fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1a7fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c1a7fff8000:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1a7fff8010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1a7fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1a7fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1a7fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1a7fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==8392==ABORTING
Daniel, can you take a look at this?
Flags: needinfo?(dholbert)
Keywords: sec-critical
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
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")
Attachment #8752600 - Attachment description: margin.html → testcase 1 (margin.html)
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;
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
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.
Ok - maybe we should just do GetMargin at that callsite instead?
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.
Oh, I guess that's kinda what GetMargin() would let us do. Yeah, that seems reasonable.
Attached patch fix v1 (obsolete) — 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)
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.
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")
Tracking for 49 since this is sec-critical. Does this affect earlier versions?
Flags: needinfo?(dholbert)
No, this only affects Firefox 49 (regressed by bug 1272983).
Flags: needinfo?(dholbert)
Keywords: regression, testcase
Note that this was also reported in bug 1273360.
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+
Group: core-security → layout-core-security
(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.
This testcase crashes due to the other GetMarginNoPercentage callsite.  (And if I use GetPaddingNoPercentage instead, it produces a broken layout instead of crashing.)
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)
(Sorry, forgot to qref; reposting.)
Attachment #8754061 - Attachment is obsolete: true
Attachment #8754061 - Flags: review?(dbaron)
Attachment #8754062 - Flags: review?(dbaron)
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)
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)
Thanks a ton for digging into this dholbert! Sorry for the mess :-(
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.
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)
(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)
(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...
But I guess it's possible that my optimization was enough. Talos will tell - might be worth giving jmaher a heads-up about it?
(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.)
(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)
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
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
Flags: sec-bounty?
(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)
Attachment #8754062 - Flags: review?(dbaron) → review+
Attachment #8754567 - Flags: review?(dbaron) → review+
Attachment #8754094 - Flags: review?(dbaron) → review+
Attachment #8754102 - Flags: review?(dbaron) → review+
Thanks for the reviews! I'll land once my updated Try run is complete:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=935e55a0a463
Duplicate of this bug: 1273360
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.
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)
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)
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.)
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]
(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.)
Group: layout-core-security → core-security-release
Duplicate of this bug: 1276926
Group: core-security-release
Keywords: sec-moderate
meant sec-high (based on comment 38)
Flags: sec-bounty? → sec-bounty+
Keywords: sec-moderatesec-high
You need to log in before you can comment on or make changes to this bug.