Closed Bug 1134667 Opened 5 years ago Closed 5 years ago
heap-buffer-overflow (read of size 4) at Can
Break Line Before, inside of ns Ruby Base Container Frame reflow
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
Summary: heap-buffer-overflow (read of size 4) at CanBreakLineBefore → heap-buffer-overflow (read of size 4) at CanBreakLineBefore, inside of nsRubyBaseContainerFrame reflow
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.
Assignee: nobody → quanxunzhen
Attachment #8566710 - Flags: review?(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).
Attachment #8566710 - Flags: review?(dholbert) → review?(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.
Attachment #8566710 - Flags: review?(jfkthame) → review+
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
I guess I should add the testcase to crashtest and push to try server to test.
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.
That would be very nice if you can test this for me at present.
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.
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.
I have a local ASAN build on Linux64, I can try it there...
Applying the patch fixes the problem in my local ASAN build.
Thanks mats. That's great. I'll land it as soon as m-i opened.
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.
Attachment #8566966 - Flags: review?(dholbert) → review+
I tested this file in the nightly asam build, and it does crash in the same way.
I guess we can make this bug public now?
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.
(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.
(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?)
This is a sec-low rated issue too, which is part of the calculus. If this was a sec-critical UAF, maybe not. :-)
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
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.
(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?
I am, and I will propose some changes there.
No, because we enabled CSS Ruby by default since Firefox 38.
Does this impact ESR31?
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.
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.
You need to log in before you can comment on or make changes to this bug.