1753136 - Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Failed to parse precursor from nullprincipal query), at src/caps/NullPrincipal.cpp:311

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 20220131-788ab1920ef8 (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -d --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html

Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Failed to parse precursor from nullprincipal query), at src/caps/NullPrincipal.cpp:311

#0 0x7f3542c773e8 in mozilla::NullPrincipal::GetPrecursorPrincipal(nsIPrincipal**) src/caps/NullPrincipal.cpp:310:5
#1 0x7f3545f8b6bc in GetPrecursorPrincipal /builds/worker/workspace/obj-build/dist/include/nsIPrincipal.h:408:39
#2 0x7f3545f8b6bc in mozilla::dom::IsolationOptionsForNavigation(mozilla::dom::CanonicalBrowsingContext*, mozilla::dom::WindowGlobalParent*, nsIURI*, nsIChannel*, nsTSubstring<char> const&, bool, unsigned int, mozilla::Maybe<unsigned long> const&, mozilla::Maybe<nsTString<char> > const&) src/dom/ipc/ProcessIsolation.cpp:532:30
#3 0x7f3542090766 in mozilla::net::DocumentLoadListener::MaybeTriggerProcessSwitch(bool*) src/netwerk/ipc/DocumentLoadListener.cpp:1636:24
#4 0x7f35420975c3 in mozilla::net::DocumentLoadListener::OnStartRequest(nsIRequest*) src/netwerk/ipc/DocumentLoadListener.cpp:2247:8
#5 0x7f3541eee322 in mozilla::net::ParentChannelListener::OnStartRequest(nsIRequest*) src/netwerk/protocol/http/ParentChannelListener.cpp:88:25
#6 0x7f35420b3588 in mozilla::net::ParentProcessDocumentOpenInfo::OnDocumentStartRequest(nsIRequest*) src/netwerk/ipc/DocumentLoadListener.cpp:298:38
#7 0x7f3541e66d07 in mozilla::net::HttpBaseChannel::DoNotifyListener() src/netwerk/protocol/http/HttpBaseChannel.cpp:3933:15
#8 0x7f3541f6471a in mozilla::net::HttpAsyncAborter<mozilla::net::nsHttpChannel>::HandleAsyncAbort() /builds/worker/workspace/obj-build/dist/include/mozilla/net/HttpBaseChannel.h:1041:10
#9 0x7f3541facaab in applyImpl<mozilla::net::nsHttpChannel, void (mozilla::net::nsHttpChannel::*)()> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1147:12
#10 0x7f3541facaab in apply<mozilla::net::nsHttpChannel, void (mozilla::net::nsHttpChannel::*)()> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1153:12
#11 0x7f3541facaab in mozilla::detail::RunnableMethodImpl<mozilla::net::nsHttpChannel*, void (mozilla::net::nsHttpChannel::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1200:13
#12 0x7f35417bb1ce in mozilla::RunnableTask::Run() src/xpcom/threads/TaskController.cpp:467:16
#13 0x7f3541795056 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) src/xpcom/threads/TaskController.cpp:770:26
#14 0x7f3541793d18 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) src/xpcom/threads/TaskController.cpp:606:15
#15 0x7f3541793f93 in mozilla::TaskController::ProcessPendingMTTask(bool) src/xpcom/threads/TaskController.cpp:390:36
#16 0x7f35417be206 in operator() src/xpcom/threads/TaskController.cpp:124:37
#17 0x7f35417be206 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:531:5
#18 0x7f35417a9923 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1195:16
#19 0x7f35417b0a0a in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:467:10
#20 0x7f3542250ff6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:85:21
#21 0x7f3542175d57 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:331:10
#22 0x7f3542175c62 in RunHandler src/ipc/chromium/src/base/message_loop.cc:324:3
#23 0x7f3542175c62 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:306:3
#24 0x7f3546442068 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
#25 0x7f3548372d56 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:295:30
#26 0x7f354849aeac in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:5367:22
#27 0x7f354849c770 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5552:8
#28 0x7f354849cfe9 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5611:21
#29 0x556c0a253f45 in do_main src/browser/app/nsBrowserApp.cpp:225:22
#30 0x556c0a253f45 in main src/browser/app/nsBrowserApp.cpp:395:16
#31 0x7f35565da0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
#32 0x556c0a22f7bc in _start (/home/worker/builds/m-c-20220131212843-fuzzing-debug/firefox-bin+0x157bc)

Comment 1

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220202040916-f66aeabcf86c.
The bug appears to have been introduced in the following build range:

Start: d1c894f81d2a11efc998f4294fe137cb371c1d2b (20211213201156)
End: 69575514d07e77820a6200835383fcad637bba57 (20211213231848)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d1c894f81d2a11efc998f4294fe137cb371c1d2b&tochange=69575514d07e77820a6200835383fcad637bba57

Comment 2

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Testcase crashes using the initial build (mozilla-central 20220131212843-788ab1920ef8) but not with tip (mozilla-central 20220205014840-e8991d00a1d1.)
The bug appears to have been fixed in the following build range:

Start: c4339dd5c34f8b56fa479d303ce54062b43851a0 (20220203115837)
End: f5b2f691d5d59f0c5e0bbc77d6bd086b2f35f3a5 (20220203143708)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c4339dd5c34f8b56fa479d303ce54062b43851a0&tochange=f5b2f691d5d59f0c5e0bbc77d6bd086b2f35f3a5
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Hey Nika, we introduced precursor URIs on NullPrincipals within Bug 1715167 - is this assertion worrisome?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

I think this assertion is revealing a few different potential bugs/oversights.

The first interesting bug is that it seems like our URL parser isn't fully spec compliant, as it allows DEL characters inside of http hostnames, whereas based on my reading of the URL spec (https://url.spec.whatwg.org/#concept-host-parser, https://url.spec.whatwg.org/#special-scheme, https://url.spec.whatwg.org/#forbidden-domain-code-point), it appears as though for special schemes like http the DEL charcter shouldn't be allowed in the host.

The other issue is that of nsSimpleURI's behaviour when setting the query parameter. When doing so, it invokes NS_EscapeURI with esc_OnlyNonASCII (https://searchfox.org/mozilla-central/rev/4615b544a0f7166233c409c619b426c4025467a7/netwerk/base/nsSimpleURI.cpp#705), which will only %-escape non-ascii characters (which should never appear in an origin, as origins are ASCII-only), C0 control characters (codepoints 0x00 through 0x1f), and the DEL character (0x7f). None of those characters should be legal in the hostname for "special" schemes like http (though we don't effectively enforce that), but according to my reading of the spec may be allowed for other schemes. The precursor principal parsing logic makes no attempt to %-decode the query, so will fail if the origin contains any of these escaped characters.

We can fix the specific issue here by fixing either of these issues, but we should probably fix both to be safe.

ni? :valentin who might know why we don't appear to check for DEL in TestForInvalidHostCharacters https://searchfox.org/mozilla-central/rev/fe3fa7f53037d4e869858fef4ff9310dfa795c41/netwerk/base/nsStandardURL.cpp#70-78 (and/or whether there are other issues around special vs. non-special schemes in our URL parser)

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1753136 - Explicitly escape and unescape precursor origins in null principal queries, r=valentin

Comment 6

[Valentin Gosu [:valentin] (he/him)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #4)

ni? :valentin who might know why we don't appear to check for DEL in TestForInvalidHostCharacters https://searchfox.org/mozilla-central/rev/fe3fa7f53037d4e869858fef4ff9310dfa795c41/netwerk/base/nsStandardURL.cpp#70-78 (and/or whether there are other issues around special vs. non-special schemes in our URL parser)

The DEL character recently added to the invalid character list in bug 1752320.
Regarding nsSimpleURI, I think esc_OnlyNonASCII should do the right thing in escaping 0-0x1f, and 0x7f. Not sure if there's anything else I'm missing.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

:nika, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #6)

The DEL character recently added to the invalid character list in bug 1752320.

Oh neat, that should presumably also fix this assertion failure.

Regarding nsSimpleURI, I think esc_OnlyNonASCII should do the right thing in escaping 0-0x1f, and 0x7f. Not sure if there's anything else I'm missing.

Yup, the only reason it caused problems was because there was an origin which contained those characters due to nsStandardURL not preventing DEL.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ee3c7d0ca2f
Explicitly escape and unescape precursor origins in null principal queries, r=valentin

Comment 10

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/9ee3c7d0ca2f

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:nika, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.