Hello @elawre.., Google Product Security, 

Good Morning.

I would like to confirm that the crash consistently occurs in the latest Chrome "UBSAN" "Beta and Stable" builds as well.

The latest UBSAN "Stable" build can be downloaded from https://www.googleapis.com/download/storage/v1/b/chromium-browser-ubsan/o/linux-release-vptr%2Fubsan-vptr-linux-stable-61.0.3163.79.zip?generation=1504878162013216&alt=media

The latest UBSAN "Beta" build can be downloaded from https://www.googleapis.com/download/storage/v1/b/chromium-browser-ubsan/o/linux-release-vptr%2Fubsan-vptr-linux-beta-61.0.3163.79.zip?generation=1504685367727667&alt=media

Thanks,
~ Kushal.

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5615999520079872.

kushal89.shah: Thanks for your report.

herb: Can you please take a look and reassign? Thanks.

ClusterFuzz testcase 5615999520079872 is flaky and no longer crashes, so closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Hello Google Product Security Team [skia],

Good Afternoon.

I think Clusterfuzz has gotten it all wrong. 

The testcase is still reproducing the same crash consistently on latest Linux UBSAN build 501638 available at https://www.googleapis.com/download/storage/v1/b/chromium-browser-ubsan/o/linux-release-vptr%2Fubsan-vptr-linux-release-501638.zip?generation=1505324124513376&alt=media

Thanking You,

Yours Sincerely,
Kushal Arvind Shah.
Security Researcher | Fortinet's FortiGuard Labs.

Reopening based on c#6.

reed, herb: Can you please confirm this potentially high severity bug? It's been open for over a week now. Thanks.

+bungeman

Lines 69 and 70 are from https://skia-review.googlesource.com/3541, which might be related to this bug. The OOB access could happen if `length + sizeof(SkData)` overflows.

I don't know if UBSan builds have SkASSERT_RELEASE enabled? If not, that could explain why UBSan hits this. And, this bug might not happen in release builds, since the assert *would* fire in such builds.

If that's the case, then this shouldn't be exploitable in the wild, right? We can test this by confirming that the test case hits the SkASSERT_RELEASE and crashes in a release build.

 61 sk_sp<SkData> SkData::PrivateNewWithCopy(const void* srcOrNull, size_t length) {
 62     if (0 == length) {
 63         return SkData::MakeEmpty();
 64     }
 65 
 66     const size_t actualLength = length + sizeof(SkData);
 67     SkASSERT_RELEASE(length < actualLength);  // Check for overflow.
 68 
 69     void* storage = ::operator new (actualLength);
 70     sk_sp<SkData> data(new (storage) SkData(length));
 71     if (srcOrNull) {
 72         memcpy(data->writable_data(), srcOrNull, length);
 73     }
 74     return data;
 75 }

Hello @palmer, @meacer, @reed, @inferno, @elawre..., @bunge..., Google Product Security Team,

Good Evening.

I would like to confirm that the same crash is consistently reproducible in the latest Chrome UBSAN "Stable" AND "Beta" Builds available at https://www.googleapis.com/download/storage/v1/b/chromium-browser-ubsan/o/linux-release-vptr%2Fubsan-vptr-linux-stable-61.0.3163.91.zip?generation=1505809317117541&alt=media AND https://www.googleapis.com/download/storage/v1/b/chromium-browser-ubsan/o/linux-release-vptr%2Fubsan-vptr-linux-beta-62.0.3202.18.zip?generation=1505774912963123&alt=media respectively.

Thanks,
~ Kushal.

SkASSERT_RELEASE is an always assert, and will always call SK_ABORT. If you're getting to line 70 and having problems with a write near address 0, it's because your 'new' implementation is non-conforming (you're running with "allocator_may_return_null=1"). This report is invalid, wasting our time, and unproductive. Stop running in this configuration or at least fully triage any such reports to a specific real issue before reporting them to us.

 66     const size_t actualLength = length + sizeof(SkData);
 67     SkASSERT_RELEASE(length < actualLength);  // Check for overflow.
 68 
 69     void* storage = ::operator new (actualLength);
 70     sk_sp<SkData> data(new (storage) SkData(length));

Description: Access violation near NULL on destination operand

Given the code above, I am forced to believe that length must be < actualLength by the time we get to line 69. Therefore I deduce that we did not under-allocate (i.e. we have allocated sufficient space).

Given the "near NULL" annotation with the crash, I can only theorize that 'new' returned null.

Are there any other theories that can explain the crash?
Would an over-committing allocator also produce this sort of crash?

- 

I apologize for the incorrect assignment of blame. This is not due to asan returning nullptr from 'new'. This is base/allocator returning nullptr from operator new (which is also invalid). When std::new_handler is not set (as it is here), ShimCppNew will just return nullptr. The following is the stack trace from where the std::new_handler was observed as nullptr and the allocator was allowed to return nullptr.


#0  0x00007ffff41d0105 in (anonymous namespace)::CallNewHandler(size_t) (size=57732953834024) at ../../base/allocator/allocator_shim.cc:75
#1  0x00007ffff41d0510 in ShimCppNew(size_t) (size=57732953834024) at ../../base/allocator/allocator_shim.cc:177
#2  0x00007ffff41cf7e4 in operator new(size_t) (size=57732953834024) at ../../base/allocator/allocator_shim_override_cpp_symbols.h:19
#3  0x00007ffff54ea95e in SkData::PrivateNewWithCopy(void const*, size_t) (srcOrNull=0x0, length=57732953833984) at ../../third_party/skia/src/core/SkData.cpp:69
#4  0x00007ffff54eb6f7 in SkData::MakeUninitialized(size_t) (length=57732953833984) at ../../third_party/skia/src/core/SkData.cpp:104
#5  0x00007ffff53899a4 in SkBitmap::ReadRawPixels(SkReadBuffer*, SkBitmap*) (buffer=0x7fffffffc090, bitmap=0x7fffffffbd48)
    at ../../third_party/skia/src/core/SkBitmap.cpp:687
#6  0x00007ffff5754b15 in SkReadBuffer::readImage() (this=0x7fffffffc090) at ../../third_party/skia/src/core/SkReadBuffer.cpp:294
#7  0x00007ffff5bb2fa3 in SkImageSource::CreateProc(SkReadBuffer&) (buffer=...) at ../../third_party/skia/src/effects/SkImageSource.cpp:66
#8  0x00007ffff591031a in SkValidatingReadBuffer::readFlattenable(SkFlattenable::Type) (this=0x7fffffffc090, type=SkFlattenable::kSkImageFilter_Type)
    at ../../third_party/skia/src/core/SkValidatingReadBuffer.cpp:301
#9  0x00007ffff556092c in SkValidatingDeserializeFlattenable(void const*, size_t, SkFlattenable::Type) (data=0x3031844d2a70, size=172, type=SkFlattenable::kSkImageFilter_Type) at ../../third_party/skia/src/core/SkFlattenableSerialization.cpp:26
#10 0x00007ffff55609d6 in SkValidatingDeserializeImageFilter(void const*, size_t) (data=0x3031844d2a70, size=172)
    at ../../third_party/skia/src/core/SkFlattenableSerialization.cpp:30
#11 0x000000000044f504 in (anonymous namespace)::RunTestCase((anonymous namespace)::(anonymous namespace)::string&, SkBitmap&, SkCanvas*) (ipc_filter_message=..., bitmap=..., canvas=0x7fffffffc9c0) at ../../skia/tools/filter_fuzz_stub/filter_fuzz_stub.cc:32
#12 0x000000000044eb9c in (anonymous namespace)::ReadAndRunTestCase(char const*, SkBitmap&, SkCanvas*) (filename=0x7fffffffe009 "/usr/local/google/home/bungeman/Downloads/New_Ubsan_PoC.fil", bitmap=..., canvas=0x7fffffffc9c0) at ../../skia/tools/filter_fuzz_stub/filter_fuzz_stub.cc:66
#13 0x000000000044e7a3 in main(int, char**) (argc=2, argv=0x7fffffffdc28) at ../../skia/tools/filter_fuzz_stub/filter_fuzz_stub.cc:85

In particular, this line should abort: https://cs.chromium.org/chromium/src/base/allocator/allocator_shim.cc?sq=package:chromium&l=76 . C++14 18.6.1.1 (4.2) specifically states that "if the current new_handler (18.6.2.5) is a null pointer value, throws bad_alloc." In other words, instead of returning false (as if everything were ok) this must not return normally.

If this is correct, that the crash came from null returned by base/allocator, then we just have to ask: is this a shipping configuration or not?
- If this is, then we need to fix the "bug" that the allocator is returning null
- If this is not, then this test configuration is not exercising shipping behavior

@primiano: I just took a look at the review where this was added at https://codereview.chromium.org/1675143004/diff/500001/base/allocator/allocator_shim.cc#newcode114 , and I'm not sure why this specifically calls out that if std::new_handler isn't set then nullptr can be returned. It seems this should just abort? It appears that I have the wrong line above (in comment 16). It seems fine for CallNewHandler to return false, but it does not seem ok for ShimCppNew to be returning nullptr.

#13: Please keep the discussion respectful and professional. I'm deleting your comment.

`allocator_may_return_null` is a build-time flag that the sanitizers use as a way of configuring their checking behavior. It's not a run-time flag that anyone was saying you had turned on. Our concern is that it may be inducing behavior that breaks Skia code's (likely valid) assumptions; and therefore, the sanitizer's behavior might not really shed light on the behavior of production builds.

As discussed in #14, under likely-valid assumptions, the code seems safe, and as discussed in #18, we'll get to the bottom of the returning-null behavior.

In the meantime, I'll ask that you dig into your crash reports a bit more to make sure the root cause likely to really be a bug in Skia (or whatever code). It'd be ideal if you can show that the test cases really trigger buggy behavior in production builds. The sanitizers like UBSan are very valuable, but not every issue they uncover is a real issue. (Most, but not all.)

Thank you.

Hello @palmer, 

Good Afternoon.

#12: Please request your colleague (in this case @bungeman) to refrain from using disrespectful and demeaning language in the first instance, so that no retaliatory responses are heard from the other end. 

Personally, I too dislike this kind of communication and am very respectful and professional in my behavior. 

BUT, I am no Gandhi or Mandela to turn my face and offer other cheek for someone to insult the second time. I give it as good as I get it. If other person is nice, I will be much much much nicer, but if other person initiates an insult, then it's only fair that they get the same in return. What goes around comes around (ref. J. Timberlake).

As for comment #15, and its continuing comments #16 & #18, the stack trace is not "completely" identical to the one I shared in the initial report.

And Yes, from my stack trace, I did follow the code in SkData.cpp and SkBitmap.cpp and I too noticed the chances of it either being an Out-of-bounds Write or maybe a Null-Deref, But it is difficult to know for sure. Also, I did not notice any Base Allocator Shim or any other such code in the original report.

Also I am not sure what you meant by Production code. Do you mean the Current Stable Shipping Release of Chrome UBSAN? If so, then I have previously stated in #2, that the crash reproduces there too. If you meant, that I test the code via an Html page containing Skia code on Non-Sanitizer builds, then I have not done that yet, since I have seen (Most, but not all) reports to Google PSIRT have always been accepted via Sanitizer Builds ASAN/MSAN/UBSAN/TSAN/LSAN.

Thanks,
Kushal.

#21: A production build is one we would ship to users, although you could also check with a normal (without UBSan or other sanitizers) Debug build.

The builds available from https://www.googleapis.com/download/storage/v1/b/chromium-browser-ubsan/o/linux-release-vptr%2Fubsan-vptr-linux-release-500415.zip?generation=1504832740040269&alt=media and the like are not official, supported production products. They might, or might not, be useful for finding bugs and debugging, but we don't call them "Current Stable Shipping Release".

#22: Could you provide the Web Link for a normal (without UBSan or other Sanitizers) debug build "with a filter_fuzz_stub binary" to test the Vulnerability?

Though I am not sure if the crash would be reproducible on that build, BUT I can surely try it.

TL;DR I think this is simply the test harness missing a call to base::EnableTerminationOnOutOfMemory() 

reed's comment #c17 is spot on:
> is this a shipping configuration or not?
In chrome: No. chrome calls EnableTerminationOnOutOfMemory() very early in its initialization, which sets a suicidal new handler.

Re #15:
"if the current new_handler (18.6.2.5) is a null pointer value, throws bad_alloc."
But we have exceptions disabled (-fno-exceptions) in the chrome codebase.

I am not a C++ lawyercat, but I think the issue here is that C++ standards don't say what is supposed to happen if:
- The underlying allocator fails
- AND no new_handler is set
- AND exceptions are disabled.

I took a look at the canonical lib c++ implementations and they seem to be in wild disagreement on this, specifically:

- GNU's libstdc++ abort()s in the aforementioned case:
  https://code.woboq.org/gcc/libstdc++-v3/libsupc++/new_op.cc.html#54

- LLVM's libcxx just returns nullptr, behaving like the chromium shim here:
  https://github.com/llvm-mirror/libcxx/blob/bc4474e3baad95ed84582e8a9af7c50ae439458d/src/new.cpp#L82

So, unless I am misreading the code, we (chrome base allocator) seems to be conforming to llvm cxx behavior here.
If you want the suicide-on-failure just cal  base::EnableTerminationOnOutOfMemory() in your test harness like chrome does.

Reassigning to bungeman@ to deal with fixing the test harness, if necessary.
I don't think there is any bug or vulnerability in shipped code here, just in the test harness

#26: OK, thanks. Taking it out of the security triage queue and making it public.

Who is the author/maintainer of the "test harness"?

Of course C++ doesn't define anything when exceptions are disabled, that is a different language (and obviously a not very well specified one in some cases). However, it seems that in general if the spec would throw an exception then the no-exception thing to do is to either write a different api or abort (as if there was an exception but it wasn't handled). The libcxx behavior seems particularly terrible, and Skia will not work properly with it if no new_handler is installed which will abort. If Skia is used with a new handler which can cause new to return nullptr that would imply a security issue (as it would with just about any C++ library). Fortunately this is not the case with Chromium (as it installs an appropriate new_hander), but this is a rather unfortunate situation.

As to the test harness, I'm not very familiar with that either. Does this mean that every fuzzer needs to call base::EnableTerminationOnOutOfMemory() early in its main in order to be valid? I don't see what is driving the filter_fuzz_stub executable (I assume there's some external fuzzing harness that picks this up on some builder). Should the fuzzing harness be making the call so that it's driving the code in an expected way? The stubs can run separately as well (they have their own main) so they would need to contain this call as well in order to behave correctly.

@kcc: it appears we need to ensure base::EnableTerminationOnOutOfMemory() is called in all the Chromium code fuzzers (or the Chromium and libcxx operator new needs to be changed to never return nullptr). Is there a straight forward way to handle any of this? I can just add the call to base::EnableTerminationOnOutOfMemory() in filter_fuzz_stub's main, which should fix this particular issue, but this seems like a wider problem.

+mmoroz +inferno

I would just add base::EnableTerminationOnOutOfMemory() to this target for now. 
(But then we will have crash reports from fuzzing anyway)

IIRC there are cases when it's interesting to allow new to return NULL, 
even though the real Chrome binary doesn't allow that. (right?

I would be interested to hear if there are *any* interesting case for new --> null. AFAIK all of chrome's code (as well as Skia's) never check for null.

I agree that adding "base::EnableTerminationOnOutOfMemory()" to filter_fuzz_stub would be ok for now.

As for other targets (not every one, but those which depend on //base), it's hard to say for me. It sounds reasonable though, as it follows the production behavior.

CC'ing more folks who might have a stronger opinion.

FTR:
  asan's default is allocator_may_return_null=0 (i.e. asan crashes and never returns NULL on new)
  IIRC ClusterFuzz sets allocator_may_return_null=1 to get more interesting stuff. 
  This is unrelated to this bug as the crash happens w/o asan (with the system allocator) 

> IIRC there are cases when it's interesting to allow new to return NULL, 
  even though the real Chrome binary doesn't allow that. right?

Correct, but you have to ask that explicitly via base::UncheckedMalloc(). UncheckedMalloc / UncheckedCalloc will bypass the new_handler, even if it's set.

> AFAIK all of chrome's code (as well as Skia's) never check for null.
Well, actually there are only 3 users in chrome of base::UncheckedMalloc, and one of them is... skia :)


The skia/ext layer uses that to implement sk_malloc_nothrow / sk_malloc_flags(without SK_MALLOC_THROW)
There seem to be quite a number of class that end up using that, some examples:
 - SkBmpBaseCodec initializes fSrcBuffer without passing SK_MALLOC_THROW
 - dirEntryBuffer in SkIcoCodec::MakeFromStream
 - SkAutoPixmapStorage::tryAlloc
 - SkBitmapCache::Alloc
 - SkRgnBuilder::init
 - DiscardableMemoryPool::make
 - SkMallocPixelRef::MakeAllocate.

In all honesty, the fact that the default behavior of sk_malloc_flags is nothrow (i.e. UncheckedAlloc -> allow to return nullptr) by default seems a bit error-prone to me (as in, people have to remember to pass the SK_MALLOC_THROW flag to get the behavior that is the default in the rest of chrome). But I know nothing about skia's codebase and architecture, so there might be some good reason for that.

I'm unsure why anything with malloc in the name is relevant to this discussion at all. Malloc is allowed to return nullptr, and will never call new_handler. This is about 'new'. Skia is well aware that malloc can return nullptr. Chromium's malloc will crash by default instead of returning nullptr, which is why Skia ends up needing the unchecked ones so that it can get the correct behavior for malloc.

Re #36, ok, very good point. 
As far as "new" is concerned, the only way I am aware one can do a nullptr new in Chrome is by doing an UncheckedMalloc + placement new (but this is a different story).
In any other case (including using the nothrow version of new) Chrome will murder the process if the new fails (because chrome has a new_handler, and it doesn't care about new (std::nothrow)). 

So re-replying to reed's comment #32, right, I am not aware of any case where we check the return value of a c++ new.
Also doing that would pointless and dangerous.
Most modern compilers will optimize away the branch of a if(return_of_new == nullptr), because they assume new can never return nullptr (unless called with std::nothrow).

I just chcked and this is the case with clang.
See https://ghostbin.com/paste/55wao, and then compare it with the new (std::nothrow) version https://ghostbin.com/paste/yexeq .

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98b92d997e00fb6e5798fe3a320604536263d543

commit 98b92d997e00fb6e5798fe3a320604536263d543
Author: Ben Wagner <bungeman@chromium.org>
Date: Mon Sep 25 14:26:28 2017

filter_fuzz_stub to crash on unsatisfiable new.

By default Chromium's global operator new will return nullptr for
unsatisfiable requests. Chromium changes this behavior by calling
base::EnableTerminationOnOutOfMemory() to set a new_hanlder which will
properly abort. All fuzzers should also do this to match this behavior.
This updates filter_fuzz_stub to make this call, but other fuzzers may
also need to be updated.

BUG= chromium:763213 

Change-Id: Ic112ba15f9ed96e4c9412a7c52737d8489d6c615
Reviewed-on: https://chromium-review.googlesource.com/677546
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Reviewed-by: Ben Wagner <bungeman@chromium.org>
Commit-Queue: Ben Wagner <bungeman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504046}
[modify] https://crrev.com/98b92d997e00fb6e5798fe3a320604536263d543/skia/tools/filter_fuzz_stub/filter_fuzz_stub.cc

I believe this particular issue is now fixed with the fuzzer driver now handling 'new' as expected. The output from running this case is now as expected (and matches what will happen in Chromium).

$ out/ubsan/filter_fuzz_stub New_Ubsan_PoC.fil 
[1009/174033.834512:INFO:filter_fuzz_stub.cc(61)] Test case: New_Ubsan_PoC.fil
[1009/174033.835556:FATAL:memory_linux.cc(35)] Out of memory.
#0 0x7efdffc5e40d base::debug::StackTrace::StackTrace()
#1 0x7efdffc59d5b base::debug::StackTrace::StackTrace()
#2 0x7efdffda28d0 logging::LogMessage::~LogMessage()
#3 0x7efdfffb46a0 base::(anonymous namespace)::OnNoMemorySize()
#4 0x7efdfffb36c1 base::(anonymous namespace)::OnNoMemory()
#5 0x7efe00432e77 (anonymous namespace)::CallNewHandler()
#6 0x7efe004331c0 ShimCppNew
#7 0x7efe00432494 operator new()
#8 0x7efe01a5805a SkData::PrivateNewWithCopy()
#9 0x7efe01a58ec7 SkData::MakeUninitialized()
#10 0x7efe018ee9a3 SkBitmap::ReadRawPixels()
#11 0x7efe01cd6645 SkReadBuffer::readImage()
#12 0x7efe0215a353 SkImageSource::CreateProc()
#13 0x7efe01ea15f9 SkValidatingReadBuffer::readFlattenable()
#14 0x7efe01ad147c SkValidatingDeserializeFlattenable()
#15 0x7efe01ad1526 SkValidatingDeserializeImageFilter()
#16 0x0000002a9824 (anonymous namespace)::RunTestCase()
#17 0x0000002a8e3c (anonymous namespace)::ReadAndRunTestCase()
#18 0x0000002a8a48 main
#19 0x7efdfc0372b1 __libc_start_main
#20 0x00000028a029 <unknown>