1663642 - nsStandardURL::SetSpec accepts schemeless `/` as a valid URL

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.bin

Testcase found while fuzzing mozilla-central rev fb9c01b719fa.

Steps to reproduce:

1. pip install fuzzfetch
2. python -m fuzzfetch -a --fuzzing --gtest -n ff-asan
3. FUZZER=URIParser ./ff-asan/firefox testcase.bin

Assertion failure: aSeg.mLen < 0 || (aSeg.mPos + aSeg.mLen <= aSpec.Length() && aSeg.mPos + aSeg.mLen >= aSeg.mPos), at /builds/worker/checkouts/gecko/netwerk/base/nsStandardURL.cpp:233

==16025==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x7f45f9a19bb8 bp 0x7ffeabf0a950 sp 0x7ffeabf0a950 T0)
==16025==The signal is caused by a WRITE memory access.
==16025==Hint: address points to the zero page.
    #0 0x7f45f9a19bb8 in mozilla::net::nsStandardURL::SanityCheck(mozilla::net::nsStandardURL::URLSegment const&, nsTString<char> const&) /builds/worker/checkouts/gecko/netwerk/base/nsStandardURL.cpp:231:3
    #1 0x7f45f9a19d1a in mozilla::net::nsStandardURL::~nsStandardURL() /builds/worker/checkouts/gecko/netwerk/base/nsStandardURL.cpp:249:3
    #2 0x7f45f9a19f1d in mozilla::net::nsStandardURL::~nsStandardURL() /builds/worker/checkouts/gecko/netwerk/base/nsStandardURL.cpp:236:33
    #3 0x7f45f9a24dcd in mozilla::net::nsStandardURL::Release() /builds/worker/checkouts/gecko/netwerk/base/nsStandardURL.cpp:1261:1
    #4 0x7f45f7fc96f8 in ~nsCOMPtr_base /builds/worker/workspace/obj-build/dist/include/nsCOMPtr.h:329:7
    #5 0x7f45f7fc96f8 in FuzzingRunURIParser(unsigned char const*, unsigned long) /builds/worker/checkouts/gecko/netwerk/test/fuzz/TestURIFuzzing.cpp:238:1
    #6 0x555c7b058ffe in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /builds/worker/checkouts/gecko/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:562:11
    #7 0x555c7b047a66 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /builds/worker/checkouts/gecko/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:301:6
    #8 0x555c7b04c2df in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /builds/worker/checkouts/gecko/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:802:9
    #9 0x7f46051eac9f in mozilla::FuzzerRunner::Run(int*, char***) /builds/worker/checkouts/gecko/tools/fuzzing/interface/harness/FuzzerRunner.cpp:75:13
    #10 0x7f4605121053 in XREMain::XRE_mainStartup(bool*) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:3861:35
    #11 0x7f4605133597 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:4933:12
    #12 0x7f4605134333 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5003:21
    #13 0x555c7ae99375 in do_main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:217:22
    #14 0x555c7ae99375 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:331:16
    #15 0x7f461d52eb96 in __libc_start_main /build/glibc-2ORdQG/glibc-2.27/csu/../csu/libc-start.c:310

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Given that this is a bug found in the fuzz harness and not in the browser, I suppose it will need some analysis to figure out if the browser is also affected.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

This looks like the same assert as bug 1648493, but it was supposed to have been fixed on m-c by way of bug 1660975 a week prior to this being hit by the fuzzer.

Comment 3

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Same assert, different cause.

Comment 4

[Daniel Veditz [:dveditz]]

sec-high if a URL from web content can trigger this case.

Comment 5

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Attachment: Bug 1663642 - fix r=#necko

Comment 6

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Attachment: Bug 1663642 - Unit test r=#necko

Depends on D90070

Comment 7

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

(In reply to Daniel Veditz [:dveditz] from comment #4)

sec-high if a URL from web content can trigger this case.

From what I can tell this can't be triggered from web content - it may be possible but I've found no instances where we simply call SetSpec on a StandardURL without checking the scheme first.
The bug here was that nsStandardURL::SetSpec didn't treat / as an invalid URL. It just parsed an empty scheme. Most instances where we create a new URL go throught NS_NewURI which matches schemes with the proper implementation.

Comment 8

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 9175475 [details]
Bug 1663642 - fix r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. It requires chaining with a different exploit to be able to call SetSpec on an existing or newly constructed standardURL.
• 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 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?: Yes
• If not, how different, hard to create, and risky will they be?: Easy
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions. This state should not be allowed.
  No testing is necessary.

Comment 9

[Daniel Veditz [:dveditz]]

Lowering to sec-moderate based on comment 7

Comment 10

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

https://hg.mozilla.org/integration/autoland/rev/5311a3edbc43bca9cf721c0fb4e9f510895160ef

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 9175475 [details]
Bug 1663642 - fix r=#necko

Clearing sec-approval because it was moved to moderate. I wondered if this could be used as a sandbox escape; but from the comment#7 I suspect not.

Comment 12

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

https://hg.mozilla.org/integration/autoland/rev/5311a3edbc43bca9cf721c0fb4e9f510895160ef
https://hg.mozilla.org/mozilla-central/rev/5311a3edbc43

Comment 13

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 9175475 [details]
Bug 1663642 - fix r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Security in depth. This could be chained with other bugs to read arbitrary memory locations.
• 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): Blocks a codepath that leads to invalid URL state.
  Assuming we don't have any code that expects that to work (it would be crazy if we did, considering this codepath is not normally exercised at all) we should not have any regressions.
• String changes made/needed:

Comment 14

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 9175475 [details]
Bug 1663642 - fix r=#necko

approved for 82.0b2

Comment 15

[Julien Cristau [:jcristau] (back August 18)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5c8f3a6f5713

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9175475 [details]
Bug 1663642 - fix r=#necko

Approved for 78.4esr.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/be9479262e16

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad2368da8d64
Unit test r=necko-reviewers,kershaw

Comment 19

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Attachment: Bug 1663642 - Fix eslint error in test_standardurl.js a=test-only

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd4a2da695bd
Fix eslint error in test_standardurl.js a=test-only CLOSED TREE

Comment 21

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/ad2368da8d64
https://hg.mozilla.org/mozilla-central/rev/bd4a2da695bd

Comment 22

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #29916 (as of Fri, 30 Apr 2021 07:56:44 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
5%	perf_reftest_singletons (docs)	window-named-property-get.html	linux1804-64-shippable	e10s stylo	615.43 -> 582.90

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=29916