Last Comment Bug 990096 - AddressSanitizer: global-buffer-overflow due to unhandled OOM [@ JSC::Yarr::digitsCreate()]
: AddressSanitizer: global-buffer-overflow due to unhandled OOM [@ JSC::Yarr::d...
Status: RESOLVED FIXED
[adv-main31+]
: csectype-bounds, sec-high
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
-- critical (vote)
: mozilla31
Assigned To: Jason Orendorff [:jorendorff]
: Matt Wobensmith [:mwobensmith][:matt:]
: Jason Orendorff [:jorendorff]
Mentors:
: 877459 877462 (view as bug list)
Depends on:
Blocks: langfuzz 912928 988953
  Show dependency treegraph
 
Reported: 2014-03-31 09:55 PDT by Christian Holler (:decoder)
Modified: 2016-06-04 15:33 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard: [qa-]
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
unaffected
unaffected
-
wontfix


Attachments
bug-990096-part-1-wtfbridge-v1.patch (2.38 KB, patch)
2014-04-01 16:30 PDT, Jason Orendorff [:jorendorff]
hv1989: review+
Details | Diff | Splinter Review
bug-990096-part-2-newOrCrash-v1.patch (14.57 KB, patch)
2014-04-01 16:32 PDT, Jason Orendorff [:jorendorff]
hv1989: review+
abillings: sec‑approval+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2014-03-31 09:55:54 PDT
I'm seeing the following ASan error in OOM fuzzing:


==59119==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000234bb60 at pc 0x11f1c14 bp 0x7fffffff57b0 sp 0x7fffffff57a8
WRITE of size 16 at 0x00000234bb60 thread T0
    #0 0x11f1c13 in VectorBase opt64asan-oom/js/src/../../dist/include/mozilla/Vector.h:588
    #1 0x11f1c13 in Vector opt64asan-oom/js/src/../../dist/include/js/Vector.h:57
    #2 0x11f1c13 in Vector yarr/wtfbridge.h:130
    #3 0x11f1c13 in js_malloc(unsigned long) yarr/YarrPattern.h:99
    #4 0x11f1c13 in _ZL6js_newIN3JSC4Yarr14CharacterClassEEPT_v opt64asan-oom/js/src/../../dist/include/js/Utility.h:325
    #5 0x11f1c13 in JSC::Yarr::digitsCreate() yarr/RegExpJitTables.h:2635
    #6 0x11ffea3 in JSC::Yarr::YarrPattern::digitsCharacterClass() yarr/YarrPattern.h:405
    #7 0x11ffea3 in JSC::Yarr::YarrPatternConstructor::atomBuiltInCharacterClass(JSC::Yarr::BuiltInCharacterClassID, bool) yarr/YarrPattern.cpp:350
    #8 0x120858a in bool JSC::Yarr::Parser<JSC::Yarr::YarrPatternConstructor, char16_t>::parseEscape<false, JSC::Yarr::YarrPatternConstructor>(JSC::Yarr::YarrPatternConstructor&) yarr/YarrParser.h:266
    #9 0x11fe31d in JSC::Yarr::Parser<JSC::Yarr::YarrPatternConstructor, char16_t>::parseAtomEscape() yarr/YarrParser.h:406
    #10 0x11fe31d in JSC::Yarr::Parser<JSC::Yarr::YarrPatternConstructor, char16_t>::parseTokens() yarr/YarrParser.h:586
    #11 0x11f5db6 in Parser yarr/YarrParser.h:674
    #12 0x11f5db6 in JSC::Yarr::ErrorCode JSC::Yarr::parse<JSC::Yarr::YarrPatternConstructor>(JSC::Yarr::YarrPatternConstructor&, JSLinearString const&, unsigned int) yarr/YarrParser.h:855
    #13 0x11f5db6 in JSC::Yarr::YarrPattern::compile(JSLinearString const&) yarr/YarrPattern.cpp:870


According to the OOM backtraces, the OOM is happening in JSC::Yarr::digitsCreate() but the debug information seems to be garbled and didn't point me to any location. However, I identified this as the possible problem:

> 2633 CharacterClass* digitsCreate()
> 2634 {
> 2635     CharacterClass* characterClass = js_new<CharacterClass>();
> 2636     characterClass->m_ranges.append(CharacterRange(0x30, 0x39));
> 2637     return characterClass;
> 2638 }

Line 2635 is allocating characterClass but not checking the return value of js_new. With the next line this *should* lead to a null crash, but it doesn't. Maybe some weird inlining+optimizing is happening here (this is all in a header file).

When I add a null check right after the js_new and return NULL, then the signature changes and YARR hits MOZ_CRASH instead, so that confirms my theory.

Should we just add the null check right after the js_new?
Comment 1 User image Jason Orendorff [:jorendorff] 2014-04-01 16:30:52 PDT
Created attachment 8400265 [details] [diff] [review]
bug-990096-part-1-wtfbridge-v1.patch

Crash on OOM in vector methods.
Comment 2 User image Jason Orendorff [:jorendorff] 2014-04-01 16:32:11 PDT
Created attachment 8400266 [details] [diff] [review]
bug-990096-part-2-newOrCrash-v1.patch

Crash on OOM in places that were using js_new.
Comment 3 User image Hannes Verschore [:h4writer] 2014-04-02 03:52:11 PDT
Comment on attachment 8400265 [details] [diff] [review]
bug-990096-part-1-wtfbridge-v1.patch

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

Really dislike the fact of crashing instead of handling OOM, but better to crash correctly. So r+
Comment 4 User image Hannes Verschore [:h4writer] 2014-04-02 04:38:20 PDT
Comment on attachment 8400266 [details] [diff] [review]
bug-990096-part-2-newOrCrash-v1.patch

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

Again would have been nice to this properly instead of crashing. Now since this is already present for so long and we will replace yarr, it is not worth it. So this indeed seems the best way forward.

::: js/src/yarr/YarrPattern.h
@@ +30,5 @@
>  #define yarr_YarrPattern_h
>  
>  #include "yarr/wtfbridge.h"
>  #include "yarr/ASCIICType.h"
> +#include "yarr/Yarr.h"

Is this needed? newOrCrash is defined in wtfbridge.h which already gets included...
Comment 6 User image Jan de Mooij [:jandem] 2014-04-07 11:01:50 PDT
*** Bug 877459 has been marked as a duplicate of this bug. ***
Comment 7 User image Jan de Mooij [:jandem] 2014-04-07 11:40:27 PDT
*** Bug 877462 has been marked as a duplicate of this bug. ***
Comment 8 User image Al Billings [:abillings] 2014-04-09 17:25:04 PDT
Was this trunk (31) only?
Comment 9 User image Christian Holler (:decoder) 2014-04-10 07:48:34 PDT
I'm pretty sure this is a long standing bug, but I'm not sure if it's a good idea to backport this. It's an OOM issue, so not easy to trigger, but the fix is making all of these conditions controlled crash conditions so this could possibly impact stability.
Comment 10 User image Al Billings [:abillings] 2014-04-10 11:15:55 PDT
I'm trying to figure out why it didn't get sec-approval as a sec-high before checkin if it isn't trunk only and how far back it goes. Does it affect ESR24, for example?

Any sec-high that isn't trunk only should go through sec-approval. See https://wiki.mozilla.org/Security/Bug_Approval_Process

Getting the questions answered that show up in the template when someone sets sec-approval? would still be a good idea. Jason?
Comment 11 User image Jason Orendorff [:jorendorff] 2014-04-10 19:28:24 PDT
Ugh. Confusion on my part --- I fixed a ton of these OOM bugs in the past week and though we've been doing it in sec bugs out of caution, most of them are not sensitive.

The good news is, the check-in really reveals nothing whatsoever about the bug.

Also, I'm not sure this is actually sec-high either. Or sec-anything. Christian looked at it, to try to figure out why ASan was calling this a "global-buffer-overrun"; I don't think he ever found anything though. Christian?

I'll answer the usual questions, in a sec...
Comment 12 User image Jason Orendorff [:jorendorff] 2014-04-10 19:40:26 PDT
Comment on attachment 8400266 [details] [diff] [review]
bug-990096-part-2-newOrCrash-v1.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug 625600 maybe? it's old.
  Might be hard to bisect since it's apparently due to an optimization.

User impact if declined:
  None, unless this turns out to be exploitable.

Testing completed (on m-c, etc.):
  Yes, it's on m-c, seems fine.

Risk to taking this patch (and alternatives if risky):
  Well, probably nothing... The patch that landed in
  m-c looks super harmless. Maybe it'll be ignored. If we push it
  to aurora, beta, release, and esr24, that won't be ignored.
  I don't think we usually worry about that kind of thing though.

String or IDL/UUID changes made by this patch:
  none
Comment 13 User image Jason Orendorff [:jorendorff] 2014-04-10 19:44:31 PDT
Comment on attachment 8400266 [details] [diff] [review]
bug-990096-part-2-newOrCrash-v1.patch

Wrong questions. Trying again.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
  Hard. The patch gives no hint of what the problem is; a determined
  adversary would have to recreate decoder's OOM-testing stuff and run
  everything under ASan to find the bug.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
  No.

Which older supported branches are affected by this flaw?
  All... maybe?
  All branches fail to crash on OOM here, but whether they're affected
  depends on compiler optimizations.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
  I don't. They should be fairly easy to create, and low risk.

How likely is this patch to cause regressions; how much testing does it need?
  Unlikely to cause regressions. Ordinary testing would be sufficient.
Comment 14 User image Christian Holler (:decoder) 2014-04-11 03:16:37 PDT
As far as I can remember, I figured out that ASan itself is required here for Clang to perform the optimizations that lead to the global-buffer-overflow here. That doesn't mean it can't happen without ASan, just that it influences Clang's will to inline something here. I think aggressive inlining and exploiting the fact that null dereferencing is undefined behavior (and doesn't need to crash) lead to this problem. The resulting code didn't actually call anything on the null pointer anymore but somehow instead directly messed with the address in the trace.

I don't think this is easy to exploit (if possible at all), but if the uplift is trivial like Jason says, then we should of course just uplift it and be safe.
Comment 15 User image Al Billings [:abillings] 2014-04-11 10:11:06 PDT
Comment on attachment 8400266 [details] [diff] [review]
bug-990096-part-2-newOrCrash-v1.patch

Ok. Well, sec-approval+ for trunk. Let's get it onto Aurora and Beta if we have time (make and nom patches for those). I'm not sure we need to take this on ESR24.
Comment 16 User image Lawrence Mandel [:lmandel] (use needinfo) 2014-04-11 13:11:19 PDT
I'm going to mark this esr24- based on comment 13 and comment 15.
Comment 17 User image Liz Henry (:lizzard) (needinfo? me) 2014-06-19 15:44:25 PDT
matt, is this something you might verify for 31? thanks!
Comment 18 User image Matt Wobensmith [:mwobensmith][:matt:] 2014-06-19 15:48:40 PDT
Yes, thanks.
Comment 19 User image Matt Wobensmith [:mwobensmith][:matt:] 2014-07-11 14:18:37 PDT
I take back what I said in comment 17. :)

Based on comments, it does not appear easy to reproduce and appears harder to exploit, so I am marking [qa-] as a result. As usual, a test case or STR would help us verify - feel free to provide help in that direction if a verification is needed.

Note You need to log in before you can comment on or make changes to this bug.