1134667 - heap-buffer-overflow (read of size 4) at CanBreakLineBefore, inside of nsRubyBaseContainerFrame reflow

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Aki Helin]

Attachment: ff-bofr-canbreak.html

The attached page triggers a heap buffer overflow in recent tinderbox aurora builds. I updated the build on my test machine a while ago, so this could be a trivial regression, but I guess it's still useful to track these and make sure they're fixed early on.

READ of size 4 at 0x60c000055140 thread T0 (Web Content)
    #0 0x7f0da81f2930 in CanBreakLineBefore /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/layout/generic/../../dist/include/gfxFont.h:817
    #1 0x7f0da81eff82 in ReflowColumns /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsRubyBaseContainerFrame.cpp:512
    #2 0x7f0da81eeefa in Reflow /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsRubyBaseContainerFrame.cpp:422
    #3 0x7f0da801e28b in ReflowFrame /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsLineLayout.cpp:956
    #4 0x7f0da81f5760 in ReflowSegment /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsRubyFrame.cpp:219
    #5 0x7f0da81f4be9 in Reflow /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsRubyFrame.cpp:168
    #6 0x7f0da801e28b in ReflowFrame /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsLineLayout.cpp:956
    #7 0x7f0da8084781 in ReflowInlineFrame /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:3968
    #8 0x7f0da8082ef5 in DoReflowInlineFrames /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:3766
    #9 0x7f0da8079feb in ReflowInlineFrames /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:3632
    #10 0x7f0da806a8a3 in ReflowLine /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:2717

Comment 1

[Mats Palmgren (inactive)]

We're reading a value past the end of an array and using that value to determine
if we should do a line break or not.  I guess if you can control the index somehow,
then you could use it to probe some bits of the memory contents.  It doesn't look
very exploitable to me.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8566710 [details] [diff] [review]
patch

I haven't looked at this particular chunk of code or dealt with text runs recently, so I'm punting this review to jfkthame, since he knows text runs (& reviewed this code in mozilla-central changeset eada1a2a1b28).

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8566710 [details] [diff] [review]
patch

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

Looks OK; assuming asan is happy with it, r=me.

Comment 5

[Daniel Holbert [:dholbert]]

Yup, worth sanity-checking in ASAN before considering this fixed & landing. Info on building w/ ASAN here, in case you haven't built with it before:  https://developer.mozilla.org/en/Building_Firefox_with_Address_Sanitizer

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I guess I should add the testcase to crashtest and push to try server to test.

Comment 7

[Daniel Holbert [:dholbert]]

Better not to push testcases for security-sensitive bugs to Try (though maybe this bug isn't too security-sensitive).

Getting an ASAN build takes a little time, but it's worth it for re-use in the future, so it's worth doing at some point.

Having said that, I'm happy to test this for you, if you don't have time to get ASAN off the ground right now. (e.g. IIRC you use Windows, whereas I think ASAN requires a linux or Mac environment)  I'm pretty sure I have an ASAN-configged build somewhere that I can update & test.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

That would be very nice if you can test this for me at present.

Comment 9

[Daniel Holbert [:dholbert]]

Hmm, I'm actually having trouble getting a local build with ASAN, and I'm going to stop wrestling with it for now.

(spent a while trying to determine the right compiler to use, to compile the snapshot of clang that that we use for ASAN, -- turns out, that clang source code only compiles with specific clang/gcc versions.  clang 3.4 happens to compile it correctly.  And then once I had the ASAN-enhanced clang built, it gave me a weird compile error when building Firefox, "/usr/include/c++/4.9/cstddef:51:11: error: no member named 'max_align_t' in the global namespace" Not sure why I'm getting GCC 4.9 includes, but whatever; not gonna dig deeper right now.)

At this point, I'd suggest using Try server to generate a linux64-ASAN with the fix, and verify that you can load the testcase locally without any trouble, using that build.

Comment 10

[Mats Palmgren (inactive)]

To be clear - we shouldn't push tests to any branch, including Try, before the bug
has been made public.  In this case though, I guess we can make it public earlier
than usual since Ruby is disabled by default on beta/release.

Comment 11

[Mats Palmgren (inactive)]

I have a local ASAN build on Linux64, I can try it there...

Comment 12

[Mats Palmgren (inactive)]

Applying the patch fixes the problem in my local ASAN build.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Thanks mats. That's great. I'll land it as soon as m-i opened.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch for test

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8566966 [details] [diff] [review]
patch for test

Looks fine, assuming of course that the crashtest itself does indeed trigger a fatal issue in ASAN-instrumented builds.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

I tested this file in the nightly asam build, and it does crash in the same way.

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

test landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/495753e0d44f

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/495753e0d44f

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

I guess we can make this bug public now?

Comment 22

[Mats Palmgren (inactive)]

Standard procedure is that we land testcases *after* the bug has been made public.
(This is because we want to reveal as little as possible about the bug to our
adversaries who we can assume is watching everything we do in Bugzilla and our
repositories.)

You don't need to track that yourself, just mark the bug in-testsuite? and
attach a patch with the test(s) and someone will land that for you when the
time comes.

No worries though, this bug is likely not exploitable in any way, and it only
affected Nightly (IIUC).  Still, it's good to have some margin also for minor
issues like this, in case we missed something in the analysis/fix, or which
versions are affected, or a minor tweak to the test might make it more potent,
etc.

Comment 23

[Mats Palmgren (inactive)]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #21)
> I guess we can make this bug public now?

Normally I would wait a couple of weeks also for a bug like this to allow Nightly
users to update to a fixed version.  We shouldn't assume that users do that daily.

But given that the test is already public, perhaps there's no point in keeping
this bug closed.  I'll open it in a few days unless someone sees a reason not to.

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #22)
> Standard procedure is that we land testcases *after* the bug has been made
> public.

FWIW: before landing, xidorn had asked in #layout what to do about the testcase, when landing.

I sanity-checked w/ dveditz, and he said it'd be fine to go ahead & land the testcase with the fix, as long as it's a nightly-only issue & not obviously-easily-exploitable. (This surprised me a bit, but doesn't seem too outrageous, given that nightly has a pretty small target-population and users should generally be updating frequently.)

khuey & I suggested that xidorn err on the side of caution & give it a few days before landing the testcase. Hence his few-day wait.

I agree that it makes sense to tie the testcase-landing w/ the bug opening, generally. We should probably come up with a documented process for nightly-only sec bugs somewhere... (maybe that already exists?)

Comment 25

[Al Billings [:abillings - ex-MoCo]]

This is a sec-low rated issue too, which is part of the calculus. If this was a sec-critical UAF, maybe not. :-)

Comment 26

[Mats Palmgren (inactive)]

IMHO, we shouldn't even try to make judgement calls on a bug-by-bug case.  That will
inevitably lead to someone making a mistake.  There's no rush in opening bugs just
for the sake of it.  We should be conservative and allow for a margin of error (as
explained at the end of comment 22).

I don't think we should have a different process for "Nightly-only" bugs.
We should *always* hold the tests (and revealing code comments) until after the
bug is public.  It's easy to remember, it's unambiguous, and it saves time too!

We should leave the call when to open bugs to a few people experienced with that
procedure.  There's more to it than just "have all affected branches been fixed?".

As far as I know, this is standard procedure for the security bugs in Layout,
DOM, Editor, GFX etc for a long time already.  I agree we should write it down
somewhere, probably here:
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 27

[Mats Palmgren (inactive)]

A slightly unrelated note: I also think it's time to re-evaluate the rule about
landing sec-moderate fixes without approval.  That seems way too risky to me.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

(In reply to Mats Palmgren (:mats) from comment #27)
> A slightly unrelated note: I also think it's time to re-evaluate the rule
> about
> landing sec-moderate fixes without approval.  That seems way too risky to me.

If you're on security-group (and you should be), can you start a discussion there on it?

Comment 29

[Mats Palmgren (inactive)]

I am, and I will propose some changes there.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Does this affect Beta?

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

No, because we enabled CSS Ruby by default since Firefox 38.

Comment 32

[Benjamin Kerensa [:bkerensa]]

Does this impact ESR31?

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Is Firefox 37 is unaffected, why do you think it impacts ESR31?

In any case, it is sec-low. We don't normally take sec-low or sec-moderate fixes in ESR.

Comment 34

[Daniel Holbert [:dholbert]]

Definitely doesn't impact ESR 31.

The frame class in question (nsRubyBaseContainerFrame) didn't exist until Firefox 34 (added in bug 1021952, and at that point, it was just a stub & unlikely to be affected by this bug anyway).

CSS Ruby support wasn't preffed on by default until Firefox 38, which is why we're only considering this affecting Firefox 38 & newer.