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?

Crash on OOM in vector methods.

Crash on OOM in places that were using js_new.

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 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...

https://hg.mozilla.org/mozilla-central/rev/96d5b6816785 https://hg.mozilla.org/mozilla-central/rev/9a491dcb9c7f

Was this trunk (31) only?

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.

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?

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 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 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.

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 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.

I'm going to mark this esr24- based on comment 13 and comment 15.

matt, is this something you might verify for 31? thanks!

Yes, thanks.

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.