Closed Bug 1492524 Opened 11 months ago Closed 11 months ago

[libfuzzer] heap-buffer-overflow in [@DoLiteralInternal]

Categories

(Core :: Networking: HTTP, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ fixed
firefox62 --- wontfix
firefox63 + fixed
firefox64 + fixed

People

(Reporter: rforbes, Assigned: u408661)

Details

(Keywords: sec-high, Whiteboard: [necko-triaged][fuzzblock][post-critsmash-triage][adv-main63+][adv-esr60.3+])

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-phabricator-request
abillings
: sec-approval+
Details | Review
sending 0xa to the HTTP2 header decompressor caused the following crash.

==46812==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000042af1 at pc 0x7f3a84b5d5e1 bp 0x7ffe8472c760 sp 0x7ffe8472c758
READ of size 1 at 0x602000042af1 thread T0
    #0 0x7f3a84b5d5e0 in mozilla::net::Http2Decompressor::DoLiteralInternal(nsTSubstring<char>&, nsTSubstring<char>&, unsigned int) /home/rforbes/src/firefox/netwerk/protocol/http/Http2Compression.cpp:943:22
    #1 0x7f3a84b5864e in mozilla::net::Http2Decompressor::DoLiteralWithoutIndex() /home/rforbes/src/firefox/netwerk/protocol/http/Http2Compression.cpp:979:17
    #2 0x7f3a84b5652f in mozilla::net::Http2Decompressor::DecodeHeaderBlock(unsigned char const*, unsigned int, nsTSubstring<char>&, bool) /home/rforbes/src/firefox/netwerk/protocol/http/Http2Compression.cpp:441:12
    #3 0x7f3a93448798 in RunHttp2DecompressorFuzzing(unsigned char const*, unsigned long) /home/rforbes/src/firefox/netwerk/protocol/http/fuzztest/http2_decompressor_libfuzzer.cpp:22:23
    #4 0x55d94a1c0ff4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/rforbes/src/firefox/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #5 0x55d94a1be088 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /home/rforbes/src/firefox/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:442:3
    #6 0x55d94a1c3fc9 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /home/rforbes/src/firefox/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:708:5
    #7 0x55d94a1c44f8 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /home/rforbes/src/firefox/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:741:3
    #8 0x55d94a1a6e67 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/rforbes/src/firefox/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:754:6
    #9 0x7f3a929f0b21 in mozilla::FuzzerRunner::Run(int*, char***) /home/rforbes/src/firefox/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #10 0x7f3a92903939 in XREMain::XRE_mainStartup(bool*) /home/rforbes/src/firefox/toolkit/xre/nsAppRunner.cpp:3986:35
    #11 0x7f3a92917913 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/rforbes/src/firefox/toolkit/xre/nsAppRunner.cpp:4937:12
    #12 0x7f3a9291948e in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/rforbes/src/firefox/toolkit/xre/nsAppRunner.cpp:5044:21
    #13 0x55d94a106dec in do_main /home/rforbes/src/firefox/browser/app/nsBrowserApp.cpp:233:22
    #14 0x55d94a106dec in main /home/rforbes/src/firefox/browser/app/nsBrowserApp.cpp:315
    #15 0x7f3aaa57482f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #16 0x55d94a006748 in _start (/home/rforbes/src/firefox/obj/ff-asan-release/dist/bin/firefox+0x36748)

0x602000042af1 is located 0 bytes to the right of 1-byte region [0x602000042af0,0x602000042af1)
allocated by thread T0 here:
    #0 0x55d94a0ca998 in __interceptor_malloc (/home/rforbes/src/firefox/obj/ff-asan-release/dist/bin/firefox+0xfa998)
    #1 0x55d94a107f3d in moz_xmalloc /home/rforbes/src/firefox/memory/mozalloc/mozalloc.cpp:70:17
    #2 0x55d94a1c0db9 in operator new[] /home/rforbes/src/firefox/obj/ff-asan-release/dist/include/mozilla/mozalloc.h:151:12
    #3 0x55d94a1c0db9 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/rforbes/src/firefox/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:508
    #4 0x55d94a1be088 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /home/rforbes/src/firefox/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:442:3
    #5 0x55d94a1c3fc9 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /home/rforbes/src/firefox/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:708:5
    #6 0x55d94a1c44f8 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /home/rforbes/src/firefox/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:741:3
    #7 0x55d94a1a6e67 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/rforbes/src/firefox/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:754:6
    #8 0x7f3a929f0b21 in mozilla::FuzzerRunner::Run(int*, char***) /home/rforbes/src/firefox/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #9 0x7f3a92903939 in XREMain::XRE_mainStartup(bool*) /home/rforbes/src/firefox/toolkit/xre/nsAppRunner.cpp:3986:35
    #10 0x7f3a92917913 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/rforbes/src/firefox/toolkit/xre/nsAppRunner.cpp:4937:12
    #11 0x7f3a9291948e in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/rforbes/src/firefox/toolkit/xre/nsAppRunner.cpp:5044:21
    #12 0x55d94a106dec in do_main /home/rforbes/src/firefox/browser/app/nsBrowserApp.cpp:233:22
    #13 0x55d94a106dec in main /home/rforbes/src/firefox/browser/app/nsBrowserApp.cpp:315
    #14 0x7f3aaa57482f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/rforbes/src/firefox/netwerk/protocol/http/Http2Compression.cpp:943:22 in mozilla::net::Http2Decompressor::DoLiteralInternal(nsTSubstring<char>&, nsTSubstring<char>&, unsigned int)
Shadow bytes around the buggy address:
  0x0c0480000500: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa
  0x0c0480000510: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa
  0x0c0480000520: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa
  0x0c0480000530: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa
  0x0c0480000540: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa
=>0x0c0480000550: fa fa fd fa fa fa 00 00 fa fa 01 fa fa fa[01]fa
  0x0c0480000560: fa fa 00 00 fa fa 07 fa fa fa 07 fa fa fa fa fa
  0x0c0480000570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480000580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480000590: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c04800005a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==46812==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0xa,
\x0a
I'm fairly certain I know why this is happening. I need to go through Http2Compression.cpp and audit all the uses of mOffset - we have guarantees in some places that we won't overrun the buffer we're indexing into with mOffset, but there were some obvious places when I looked at this stack and the crashing line where we aren't guaranteeing that mOffset lies within the bounds of mData.
Assignee: nobody → hurley
Priority: -- → P2
Whiteboard: [necko-triaged]
Keywords: sec-high
Whiteboard: [necko-triaged] → [necko-triaged][fuzzblock]
Attached patch fuzzer-fix.patch (obsolete) — Splinter Review
Here's the patch I came up with that should (I hope) cover all the cases where we might end up with mOffset beyond the allowable bounds of mData. :rforbes, you said you were willing to run the fuzzer against a patched version. Let me know how it goes :)
Attachment #9010706 - Flags: feedback?(rforbes)
absolutely. I will work on it monday.
so far the patch looks good. We are definitely past that crash. I am going to keep running it for a while and see what come up with.
Attached file Bug 1492524
Attachment #9010706 - Attachment is obsolete: true
Attachment #9010706 - Flags: feedback?(rforbes)
Comment on attachment 9012270 [details]
Bug 1492524

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Probably medium easily - it's fairly obvious that all you have to do is lie about the length of an entry in a compressed hpack stream to trigger this bug. How to turn that into something other than a crash, though, is left as an exercise to the exploiter.

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

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: Not particularly difficult to create, riskiness is slightly more complex - it's certainly possible I have an off-by-one in there somewhere (which would cause not crashes or exploits, but failed http/2 sessions), but not super-likely.

How likely is this patch to cause regressions; how much testing does it need?: Not very likely, but manual testing on a lot of websites that support http/2 (google + friends, facebook, others) would be good to ensure that I didn't inadvertently break http/2 connections in some corner case.
Attachment #9012270 - Flags: sec-approval?
We only have two betas left. This bug will need release management approval, so we can backport to affected branches, before I can give sec-approval.

Ritu?
This one sounds a bit scary because it sounds like bake time would be beneficial for giving us confidence that it hasn't caused any regressions with major sites. But given the medium-to-easy difficulty of exploiting, it doesn't sound like we're going to want to leave this in-tree for very long before shipping the fix?

I guess I'd prefer we took this as early as possible in the 64 Beta cycle to let it bake, but will defer to the judgement of Nicholas and the sec folks about whether that's a feasible choice or not. Otherwise, might as well take it now so we can get it into next week's b13 builds.
Flags: needinfo?(hurley)
Flags: needinfo?(abillings)
I think we can take this fix, the change doesn't look so complicated. Also, given that we are ~3 weeks away from launch, there is still room to bake, find and fix regressions. Pascal, will let you make the final decision for beta63.
Flags: needinfo?(rkothari) → needinfo?(pascalc)
I'm pretty comfortable landing this in whatever release y'all see fit - yes, there is a (very slight) chance of an off-by-one, but our automated tests do exercise the compression code pretty well in the well-formed case, so I think it would have to be a pretty corner-ish case. Like I said in my approval request, the failure mode in this potential new case is way better from a security perspective, and at least no worse from a user experience perspective (crash vs. page that maybe doesn't load quite right), IMHO.
Flags: needinfo?(hurley)
Comment on attachment 9012270 [details]
Bug 1492524

sec-approval+ for trunk. Can we get branch patches made and nominated?
Flags: needinfo?(abillings)
Attachment #9012270 - Flags: sec-approval? → sec-approval+
Nicholas, can you make the beta uplift approval request? Thanks
Flags: needinfo?(pascalc) → needinfo?(hurley)
https://hg.mozilla.org/integration/autoland/rev/430f18b70e6f2def589d9b304abad3bc8d10d58d

This grafts cleanly as-landed, so Beta/ESR60 approval requests are all that's needed for uplift.
Comment on attachment 9012270 [details]
Bug 1492524

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: http/2

User impact if declined: Potential UAF crashes

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): See comment 6

String changes made/needed: None
Flags: needinfo?(hurley)
Attachment #9012270 - Flags: approval-mozilla-beta?
Comment on attachment 9012270 [details]
Bug 1492524

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high

User impact if declined: Potential UAF crashes

Fix Landed on Version: 64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): See comment 6

String or UUID changes made by this patch: None
Attachment #9012270 - Flags: approval-mozilla-esr60?
https://hg.mozilla.org/mozilla-central/rev/430f18b70e6f
Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9012270 [details]
Bug 1492524

Uplift approved for 63 beta 14, thanks.
Attachment #9012270 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9012270 [details]
Bug 1492524

UAF fix, approved for ESR 60.3.
Attachment #9012270 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify-
Whiteboard: [necko-triaged][fuzzblock] → [necko-triaged][fuzzblock][post-critsmash-triage]
Whiteboard: [necko-triaged][fuzzblock][post-critsmash-triage] → [necko-triaged][fuzzblock][post-critsmash-triage][adv-main63+][adv-esr60.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.