New issue
Advanced search Search tips

Issue 818396 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug-Security



Sign in to add a comment

Use-of-uninitialized-value in blink::SubresourceIntegrity::ParseAlgorithmPrefix

Project Member Reported by ClusterFuzz, Mar 3 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5749762427715584

Fuzzer: libFuzzer_html_preload_scanner_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  blink::SubresourceIntegrity::ParseAlgorithmPrefix
  blink::HTMLPreloadScanner::Scan
  blink::LLVMFuzzerTestOneInput
  
Sanitizer: memory (MSAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=540520:540528

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5749762427715584

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.
 
Project Member

Comment 1 by ClusterFuzz, Mar 3 2018

Labels: Test-Predator-Auto-Owner
Owner: vogelheim@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/def6084e9b4a72781bbf47b7eb819084af96c5d2 (Implement Origin Trial for signature-based SRI.).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
Project Member

Comment 2 by ClusterFuzz, Mar 3 2018

Components: Blink>HTML>Parser
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 3 by sheriffbot@chromium.org, Mar 3 2018

Labels: M-66
Project Member

Comment 4 by sheriffbot@chromium.org, Mar 3 2018

Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 5 by sheriffbot@chromium.org, Mar 3 2018

Labels: Pri-1
Status: Started (was: Assigned)
This seems difficult to reproduce. It currently doesn't repro for me, even with the 'clusterfuzz reproduce ...' command; with neither current revision nor the one form the bug report; neither with nor without xvfb. WIll try some more, though.
Hrm. I can now consistently reproduce a use-of-uninitialized-value with the given test case, but at a very different stack (and one that has pretty much nothing to do with any code I touched).

Investigating further, but am tempted to drop this.

Comment 9 by kenrb@chromium.org, Mar 5 2018

Thanks for looking at this.

What do the other call stacks look like that you are seeing?
@kenrb:

Uninitialized bytes in __interceptor_strlen at offset 2 inside [0x701000003c70, 3)
==168296==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7fd409ee1141 in g_strconcat (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x6a141)
    #1 0x7fd409e9cb24  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x25b24)
    #2 0x7fd409e9d13f in g_get_language_names (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x2613f)
    #3 0xaef7702 in l10n_util::GetApplicationLocaleInternal(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) ui/base/l10n/l10n_util.cc:485:34
    #4 0xaef82e2 in GetApplicationLocale ui/base/l10n/l10n_util.cc:522:30
    #5 0xaef82e2 in l10n_util::GetApplicationLocale(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) ui/base/l10n/l10n_util.cc:529
    #6 0xaeeca74 in ui::ResourceBundle::LoadTestResources(base::FilePath const&, base::FilePath const&) ui/base/resource/resource_bundle.cc:383:3
    #7 0xaeec2c4 in ui::ResourceBundle::InitSharedInstanceWithPakPath(base::FilePath const&) ui/base/resource/resource_bundle.cc:210:23
    #8 0x119e90ad in content::TestContentClient::TestContentClient() content/test/test_content_client.cc:51:5
    #9 0x10673fb0 in content::TestContentClientInitializer::TestContentClientInitializer() content/public/test/test_content_client_initializer.cc:20:29
    #10 0x1066c065 in TestEnvironment content/public/test/blink_test_environment.cc:39:3
    #11 0x1066c065 in content::SetUpBlinkTestEnvironment() content/public/test/blink_test_environment.cc:85
    #12 0xf8cf1c in blink::BlinkFuzzerTestSupport::BlinkFuzzerTestSupport(int, char**) third_party/WebKit/Source/platform/testing/BlinkFuzzerTestSupport.cpp:28:3
    #13 0xf82105 in blink::LLVMFuzzerTestOneInput(unsigned char const*, unsigned long) third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerFuzzer.cpp:35:48
    #14 0xf82bb8 in LLVMFuzzerTestOneInput third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerFuzzer.cpp:81:10
    #15 0xf8595f in main testing/libfuzzer/unittest_main.cc:57:5
    #16 0x7fd404c812b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
    #17 0xf0e029 in _start (/mnt/vogelheim/chrome/chromium/src/out/x64.msan/html_preload_scanner_fuzzer+0xf0e029)

  Uninitialized value was created by a heap allocation
    #0 0xf33e8d in __interceptor_malloc /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/msan/msan_interceptors.cc:946:3
    #1 0x7fd409ec6e08 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4fe08)

SUMMARY: MemorySanitizer: use-of-uninitialized-value (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x6a141) in g_strconcat
Exiting

Cc: mmoroz@chromium.org
+mmoroz: Max, could this be a bug with the fuzzer, where it is passing uninitialized memory into the preload scanner?
Labels: -Pri-1 Pri-2
Hrmm. #11 was a good hint, because I was only looking at my code rather than what surrounds it. I think I have an explanation for the stack in the bug report now (but not for the one I'm actually seeing; nor can I verify this):

- My change introduced a new member CachedDocumentParameters::integrity_features
- This is being read very close to stack reported by MSAN.
- That class has two constructors, one that initializes it from Document*, and one default constructor.
- The Document* constructor initializes the field and is used by all "real" code.
- HTMLPreloadScannerFuzzer uses the default constructor (indirectly). I don't think that initializes the member. Thus use-of-uninitialized-value happens.


So the theory in #11 would be partially correct, in that this is triggered by the way the fuzzer sets up the test case. It still seems the actual bug would be outside the fuzzer.

The thing that irritates me is that I don't think the default constructor initializes the other fields either. So I could either change the default constructor. Or I could add one line to the fuzzer to initialize it from the fuzz data, which would improve fuzz coverage. Or both.

Not sure what to do. If anyone feels strongly here, I'll gladly follow your advice.


Given that my current best theory is that this is triggered only in fuzzing, I will generously downgrade the bug from Pri 1.
After some more looking:

- When I follow the MSAN build instructions more carefully - and particularly, add "use_prebuilt_instrumented_libraries = true" to args.gn - then the stack in #10 goes away. I'd say, that was my fault.

- However, then I'm back to not being able to reproduce. I tried some further variants, including downloading the pre-built binary to make sure it's nothing in my build environment, but... no repro.

- I have a tentative patch for setting CachedDocumentParameters::integrity_features from the fuzz data, but I'm not sure it's worthwhile if I cannot check whether it actually fixes anything.
Project Member

Comment 14 by sheriffbot@chromium.org, Mar 7 2018

Labels: -Security_Impact-Head Security_Impact-Beta
Denial, would you mind trying to reproduce this with our reproduce tool? Please see the following message on the testcase details page (https://clusterfuzz.com/v2/testcase-detail/5749762427715584?noredirect=1):


You can reproduce this crash painlessly with our reproduce tool. For Googlers, install the required libraries and run prodaccess && /google/data/ro/teams/clusterfuzz-tools/releases/clusterfuzz reproduce 5749762427715584. For non-Googlers, see the installation section. Report any issues at clusterfuzz-dev@chromium.org.
Cc: och...@chromium.org
Reproduce tool with MSAN on rodete does not work. Look at "Running on other distros using Docker" on https://www.chromium.org/developers/testing/memorysanitizer
Oh right, thanks Abhishek for reminding that!

Daniel, I apologize for swapping some letters in your name in c#15, that was not intentional :)
#15: Yeah, I had tried that (#7), and it still doesn't work.

#16: Ah. "Reproduce tool with MSAN on rodete does not work." Thanks, that's certainly important info! :)

#17: "Denial" is a cool hacker name, so all's fine. :)

---------------

I still couldn't get this to work with Docker, though:

1, Running the repro tool directly in Docker yields "command not found", presumably because the Docker container can't access the /google/data/ro/... directory.

2, What does work - as in, it runs - is to use the repro tool to build on my machine, and then running the resulting binary on Docker. That seems to run fine - no errors; and the various environment variables, etc. all seem to be there, but yet... no repro. Below is a log of the command, in case either of you can see anything I'm doing obviously wrong here.


Any further hints?
If not, I'll submit a patch based on my best guess about the stack in the bug report.


---------------------------------------------------------------------

$ third_party/instrumented_libraries/scripts/run_docker.sh  MSAN_OPTIONS="handle_sigfpe=1:handle_sigbus=1:handle_sigill=1:allocator_release_to_os_interval_ms=500:halt_on_error=1:print_summary=1:handle_abort=1:print_stats=1:coverage=0:symbolize=1:handle_segv=1:use_sigaltstack=1" DISPLAY=":0.0" MSAN_SYMBOLIZER_PATH="/usr/local/google/home/vogelheim/.pex/code/cd4ccd3a4765253140e654a372f37ec8274b6f7e/clusterfuzz/resources/llvm-symbolizer" /mnt/vogelheim/chrome/chromium/src/out/clusterfuzz_5749762427715584/html_preload_scanner_fuzzer -rss_limit_mb=2048 -runs=100 -timeout=25 ./fuzz-3
+ export USER=user
+ USER=user
+ useradd -mU -u 75773 user
+ sudo -u user bash -c 'Xvfb :1 -screen 0 1280x1024x24 -ac -nolisten tcp >/dev/null 2>&1 &'
+ sudo -u user DISPLAY=:1 HOME=/home/user MSAN_OPTIONS=handle_sigfpe=1:handle_sigbus=1:handle_sigill=1:allocator_release_to_os_interval_ms=500:halt_on_error=1:print_summary=1:handle_abort=1:print_stats=1:coverage=0:symbolize=1:handle_segv=1:use_sigaltstack=1 DISPLAY=:0.0 MSAN_SYMBOLIZER_PATH=/usr/local/google/home/vogelheim/.pex/code/cd4ccd3a4765253140e654a372f37ec8274b6f7e/clusterfuzz/resources/llvm-symbolizer /mnt/vogelheim/chrome/chromium/src/out/clusterfuzz_5749762427715584/html_preload_scanner_fuzzer -rss_limit_mb=2048 -runs=100 -timeout=25 ./fuzz-3
WARNING: The binary has too many instrumented PCs.
         You may want to reduce the size of the binary
         for more efficient fuzzing and precise coverage data
INFO: Seed: 3055104709
INFO: Loaded 2 modules   (2453784 guards): 15008 [0x7fac3f89c318, 0x7fac3f8aad98), 2438776 [0x16331710, 0x16c7f0f0),
/mnt/vogelheim/chrome/chromium/src/out/clusterfuzz_5749762427715584/html_preload_scanner_fuzzer: Running 1 inputs 100 time(s) each.
Running: ./fuzz-3
Executed ./fuzz-3 in 184 ms
***
*** NOTE: fuzzing was not performed, you have only
***       executed the target code on a fixed set of inputs.
***




Project Member

Comment 19 by bugdroid1@chromium.org, Mar 19 2018

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

commit 1912996b9488dcfd7996c254e75b081830f9f404
Author: Daniel Vogelheim <vogelheim@chromium.org>
Date: Mon Mar 19 15:05:58 2018

Fuzz also over IntegrityFeatures flag.

Increases fuzzer coverage to match changes in cl 924187. Hopefully also
fixes the referenced bug, because this now explicitly initializes
CachedDocumentParameters::integrity_features when called from the fuzzer.

Bug:  818396 
Change-Id: Ica18606543bd5cb36df567a132a6fe2b1da9620f
Reviewed-on: https://chromium-review.googlesource.com/968508
Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544030}
[modify] https://crrev.com/1912996b9488dcfd7996c254e75b081830f9f404/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerFuzzer.cpp

Daniel, I'm sorry that none of the suggestions above helps to reproduce the issue. I filed a separate bug 823304 for us, so we'll take a closer look later (next two weeks busy with bug bash).

It seems that your CL should fix this, CF will verify that in a day or so. Thank you!
Project Member

Comment 21 by ClusterFuzz, Mar 20 2018

ClusterFuzz has detected this issue as fixed in range 544024:544037.

Detailed report: https://clusterfuzz.com/testcase?key=5749762427715584

Fuzzer: libFuzzer_html_preload_scanner_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  blink::SubresourceIntegrity::ParseAlgorithmPrefix
  blink::HTMLPreloadScanner::Scan
  blink::LLVMFuzzerTestOneInput
  
Sanitizer: memory (MSAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=540520:540528
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=544024:544037

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5749762427715584

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 22 by ClusterFuzz, Mar 20 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
ClusterFuzz testcase 5749762427715584 is verified as fixed, so closing issue as verified.

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

Comment 23 by sheriffbot@chromium.org, Mar 20 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Woohoo, well done!
Labels: -ReleaseBlock-Stable -M-66 M-67
Project Member

Comment 26 by sheriffbot@chromium.org, Jun 26 2018

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment