1891506 - AddressSanitizer: heap-buffer-overflow [@ net_CoalesceDirs] with WRITE of size 1

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

Testcase found while fuzzing mozilla-central rev 1dc823d4e96d (built with: --enable-address-sanitizer --enable-fuzzing).

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch
$ python -m fuzzfetch --build 1dc823d4e96d -a --fuzzing --target firefox gtest -n firefox
$ FUZZER=URIParser ./firefox/firefox testcase.bin

AddressSanitizer: heap-buffer-overflow [@ net_CoalesceDirs] with WRITE of size 1

    ==216==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x50200008db98 at pc 0x7f1fa4795fb3 bp 0x7ffe96914c80 sp 0x7ffe96914c78
    WRITE of size 1 at 0x50200008db98 thread T0
    SCARINESS: 31 (1-byte-write-heap-buffer-overflow)
        #0 0x7f1fa4795fb2 in net_CoalesceDirs(netCoalesceFlags, char*) /netwerk/base/nsURLHelper.cpp:361:11
        #1 0x7f1fa49c0746 in mozilla::net::nsStandardURL::Resolve(nsTSubstring<char> const&, nsTSubstring<char>&) /netwerk/base/nsStandardURL.cpp
        #2 0x7f1fa087a137 in FuzzingRunURIParser(unsigned char const*, unsigned long) /netwerk/test/fuzz/TestURIFuzzing.cpp:181:18
        #3 0x55ea8051b91b in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /tools/fuzzing/libfuzzer/FuzzerLoop.cpp:570:11
        #4 0x55ea8051b3a1 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /tools/fuzzing/libfuzzer/FuzzerLoop.cpp:479:7
        #5 0x55ea8051c7d7 in fuzzer::Fuzzer::MutateAndTestOne() /tools/fuzzing/libfuzzer/FuzzerLoop.cpp:717:19
        #6 0x55ea8051d1e5 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /tools/fuzzing/libfuzzer/FuzzerLoop.cpp:861:9
        #7 0x55ea8050db6b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /tools/fuzzing/libfuzzer/FuzzerDriver.cpp:864:14
        #8 0x7f1fb49f64ee in mozilla::FuzzerRunner::Run(int*, char***) /tools/fuzzing/interface/harness/FuzzerRunner.cpp:75:13
        #9 0x7f1fb4904646 in XREMain::XRE_mainStartup(bool*) /toolkit/xre/nsAppRunner.cpp:4674:35
        #10 0x7f1fb4915bff in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /toolkit/xre/nsAppRunner.cpp:5946:12
        #11 0x7f1fb4916f41 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /toolkit/xre/nsAppRunner.cpp:6015:21
        #12 0x55ea803497a2 in do_main /browser/app/nsBrowserApp.cpp:227:22
        #13 0x55ea803497a2 in main /browser/app/nsBrowserApp.cpp:445:16
        #14 0x7f1fcce9c082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: eebe5d5f4b608b8a53ec446b63981bba373ca0ca)
        #15 0x55ea8026de68 in _start (/home/worker/firefox/firefox+0xdce68) (BuildId: 55084467f1eb4fefe25b8b5876b38dd05b797d2d)
    
    DEDUP_TOKEN: net_CoalesceDirs(netCoalesceFlags, char*)
    0x50200008db98 is located 0 bytes after 8-byte region [0x50200008db90,0x50200008db98)
    allocated by thread T0 here:
        #0 0x55ea80309cce in malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
        #1 0x55ea8034ee05 in moz_xmalloc /memory/mozalloc/mozalloc.cpp:52:15
        #2 0x7f1fa49adb05 in mozilla::net::nsStandardURL::AppendToSubstring(unsigned int, int, char const*) /netwerk/base/nsStandardURL.cpp:1368:25
        #3 0x7f1fa49c06e7 in mozilla::net::nsStandardURL::Resolve(nsTSubstring<char> const&, nsTSubstring<char>&) /netwerk/base/nsStandardURL.cpp:2785:14
        #4 0x7f1fa087a137 in FuzzingRunURIParser(unsigned char const*, unsigned long) /netwerk/test/fuzz/TestURIFuzzing.cpp:181:18
        #5 0x55ea8051b91b in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /tools/fuzzing/libfuzzer/FuzzerLoop.cpp:570:11
        #6 0x55ea8051b3a1 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /tools/fuzzing/libfuzzer/FuzzerLoop.cpp:479:7
        #7 0x55ea8051c7d7 in fuzzer::Fuzzer::MutateAndTestOne() /tools/fuzzing/libfuzzer/FuzzerLoop.cpp:717:19
        #8 0x55ea8051d1e5 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /tools/fuzzing/libfuzzer/FuzzerLoop.cpp:861:9
        #9 0x55ea8050db6b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /tools/fuzzing/libfuzzer/FuzzerDriver.cpp:864:14
        #10 0x7f1fb49f64ee in mozilla::FuzzerRunner::Run(int*, char***) /tools/fuzzing/interface/harness/FuzzerRunner.cpp:75:13
        #11 0x7f1fb4904646 in XREMain::XRE_mainStartup(bool*) /toolkit/xre/nsAppRunner.cpp:4674:35
        #12 0x7f1fb4915bff in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /toolkit/xre/nsAppRunner.cpp:5946:12
        #13 0x7f1fb4916f41 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /toolkit/xre/nsAppRunner.cpp:6015:21
        #14 0x55ea803497a2 in do_main /browser/app/nsBrowserApp.cpp:227:22
        #15 0x55ea803497a2 in main /browser/app/nsBrowserApp.cpp:445:16
        #16 0x7f1fcce9c082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: eebe5d5f4b608b8a53ec446b63981bba373ca0ca)
    
    DEDUP_TOKEN: __interceptor_malloc
    SUMMARY: AddressSanitizer: heap-buffer-overflow /netwerk/base/nsURLHelper.cpp:361:11 in net_CoalesceDirs(netCoalesceFlags, char*)
    Shadow bytes around the buggy address:
      0x50200008d900: fa fa 00 04 fa fa fd fd fa fa fd fd fa fa fa fa
      0x50200008d980: fa fa 00 02 fa fa 00 fa fa fa fd fd fa fa fd fa
      0x50200008da00: fa fa fd fd fa fa fd fd fa fa fd fa fa fa fd fd
      0x50200008da80: fa fa fd fd fa fa fd fa fa fa fa fa fa fa fd fd
      0x50200008db00: fa fa 00 02 fa fa 04 fa fa fa fd fa fa fa fa fa
    =>0x50200008db80: fa fa 00[fa]fa fa fa fa fa fa fa fa fa fa fa fa
      0x50200008dc00: fa fa fa fa fa fa fd fd fa fa fd fd fa fa fa fa
      0x50200008dc80: fa fa 00 02 fa fa 04 fa fa fa fd fd fa fa fa fa
      0x50200008dd00: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fa fa
      0x50200008dd80: fa fa 00 02 fa fa 00 fa fa fa fd fd fa fa fa fa
      0x50200008de00: fa fa fa fa fa fa 00 02 fa fa 04 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
    
    Command: /home/worker/firefox/firefox -rss_limit_mb=3500 -use_value_profile=1 -timeout=5 -entropic=1 -dict=./tokens.dict ./corpora/ -handle_segv=0 -handle_bus=0 -handle_abrt=0 -handle_ill=0 -handle_fpe=0 -print_pcs=1
    
    ==216==ABORTING

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: Detailed Crash Information

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Comment 3

[Tyson Smith [:tsmith]]

Attachment: testcase.bin

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

How can I run a local build to test this? I can repro, but that's not enough to debug the problem. What exact input to Resolve() causes the problem?

Comment 5

[Tyson Smith [:tsmith]]

The instructions are here: https://firefox-source-docs.mozilla.org/tools/fuzzing/fuzzing_interface.html#local-build-requirements-and-flags

tldr:

• Add ac_add_options --enable-fuzzing to your mozconfig
• run mach build then mach gtest dontruntests
• run FUZZER=URIParser <local-build>/firefox testcase.bin

I can also get a Pernosco session if you'd like just ni? me.

Comment 6

[Valentin Gosu [:valentin]]

This is most likely a regression from bug 1890346 (or less likely by bug 1887614).
You should be able to see the input by adding MOZ_LOG=nsStandardURL:5 to the command line.

Comment 7

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1890346

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1891506: Fix issue with net_CoalesceDirs r=valentin!

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1891506: Update tests for net_CoalesceDirs r=valentin!

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

FYI, setting FUZZING = URLParser causes invalid URIs to be sent which trigger asserts (they must begin with '/'). I'm not sure if this is intentional or not, and if this is something which should be possible to cause to be passed to Coalesce by web content.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

I've made it resilient to input without a leading /, in case it can be caused by web content somehow

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

@jkratzer - is the URLParser fuzzing feeding things that can't ever be fed to it by web content? I'm going to keep this fix in anyways, but wanted to check.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9396959 [details]
Bug 1891506: Fix issue with net_CoalesceDirs r=valentin!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very hard - I don't think it's possible to feed a bad string in. The patch makes sure if there is a way, it'll be safe
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: nightly and beta
• 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?: Trivial
• How likely is this patch to cause regressions; how much testing does it need?: Exceedingly unlikely
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9396959 [details]
Bug 1891506: Fix issue with net_CoalesceDirs r=valentin!

Security Approval Request

• How easily could an exploit be constructed based on the patch?:
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?:
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?:
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Beta/Release Uplift Approval Request

• User impact if declined: Low; I don't believe it's possible for web content to trigger this. If it is, then still pretty low (max 1-byte overrun with a 0).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): Belt-and-suspenders patch; I don't think web content can trigger it
• String changes made/needed: none
• Is Android affected?: Yes

Comment 15

[Jason Kratzer [:jkratzer]]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #12)

@jkratzer - is the URLParser fuzzing feeding things that can't ever be fed to it by web content? I'm going to keep this fix in anyways, but wanted to check.

I think this might actually be a question for Valentin. I'm not sure if there's anything done to the URI prior to passing it NS_MutateURI.

Comment 16

[Daniel Veditz [:dveditz]]

If you have a nsIURI to call NS_MutateURI it should already have all the sanitization and normalization that any user-input URL would get

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 9396959 [details]
Bug 1891506: Fix issue with net_CoalesceDirs r=valentin!

sec-approval+ = dveditz

Comment 18

[Valentin Gosu [:valentin]]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #10)

FYI, setting FUZZING = URLParser causes invalid URIs to be sent which trigger asserts (they must begin with '/'). I'm not sure if this is intentional or not, and if this is something which should be possible to cause to be passed to Coalesce by web content.

If the URIParser fuzzer is hitting the assert I think that means web content can also hit it, since all the fuzzer is doing is call NS_NewURI.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Interesting, since I thought my updated test (which tried passing "") didn't trigger the assertion. But I can re-check that before landing the tests.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

dveditz - when should I land tests?

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ce748640a32
Fix issue with net_CoalesceDirs r=valentin,necko-reviewers

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8ce748640a32

Comment 23

[Donal Meehan [:dmeehan]]

Comment on attachment 9396959 [details]
Bug 1891506: Fix issue with net_CoalesceDirs r=valentin!

Approved for 126.0b5

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/32f7ae0d5316

Comment 25

[Daniel Veditz [:dveditz]]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #20)

dveditz - when should I land tests?

It's OK to land tests now.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/157920102b96
Update tests for net_CoalesceDirs r=valentin,necko-reviewers

Comment 27

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/mozilla-central/rev/157920102b96