1911747 - Assertion failure: false, at netwerk/protocol/data/DataChannelParent.cpp:105 with IPC fuzzing

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Christian Holler (:decoder)]

In experimental IPC fuzzing, we found the following crash on mozilla-central revision 20240803-3455f458595d (fuzzing-asan-nyx-opt build):

Assertion failure: false, at /netwerk/protocol/data/DataChannelParent.cpp:105
=================================================================
==1639==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x7ffff7fb4983 bp 0x000000000069 sp 0x7fffffffbe10 T0)
==1639==The signal is caused by a WRITE memory access.
==1639==Hint: address points to the zero page.
    #0 0x7ffff7fb4983  (ld_preload_fuzz.so+0x3983)
    #1 0x7ffff7fb677a  (ld_preload_fuzz.so+0x577a)
    #2 0x7fffdeae7508 in mozilla::net::DataChannelParent::RecvNotifyListeners(mozilla::net::DataChannelInfo const&) /netwerk/protocol/data/DataChannelParent.cpp:104:3
    #3 0x7fffdf222952 in mozilla::net::PDataChannelParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PDataChannelParent.cpp:177:85
    #4 0x7fffe89e5ddf in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PContentParent.cpp:6378:32
    #5 0x7fffdf637b9b in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /ipc/glue/MessageChannel.cpp:1820:25
    [...]

The attached testcase can be reproduced using a special build to inject IPC messages.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

Apparently, this is this code failing:

https://searchfox.org/mozilla-central/rev/8b0666aff1197e1dd8017de366343de9c21ee437/netwerk/protocol/data/DataChannelParent.cpp#103-105

kershaw, do you know what the security implications are if LoadInfoArgsToLoadInfo fails here?

Comment 4

[Julian Descottes [:jdescottes]]

I don't know much about crashes, but functionally speaking, crashing here should only mean that the observer notification for the data channel will not be emitted. This observer notification is only used by WebDriver BiDi to raise network events that can be monitored by automated tests & tooling. Soon it should also be used by DevTools in order to show the request in the netmonitor.

Comment 5

[Christian Holler (:decoder)]

I think the issue is rather what happens if we don't crash here. This only crashes in Nightly (it's a diagnostic assert), since the MOZ_ALWAYS_SUCCEEDS expands to a diagnostic assert only. So in release, we will continue without the proper loadInfo (uninitialized?).

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Christian Holler (:decoder) from comment #5)

I think the issue is rather what happens if we don't crash here. This only crashes in Nightly (it's a diagnostic assert), since the MOZ_ALWAYS_SUCCEEDS expands to a diagnostic assert only. So in release, we will continue without the proper loadInfo (uninitialized?).

I see, in that case I assume we will crash in SetLoadInfo? I assume MOZ_RELEASE_ASSERT also works in release?

https://searchfox.org/mozilla-central/rev/8b0666aff1197e1dd8017de366343de9c21ee437/netwerk/base/nsBaseChannel.cpp#470-477

nsBaseChannel::SetLoadInfo(nsILoadInfo* aLoadInfo) {
  MOZ_RELEASE_ASSERT(aLoadInfo, "loadinfo can't be null");
  mLoadInfo = aLoadInfo;

  // Need to update |mNeckoTarget| when load info has changed.
  SetupNeckoTarget();
  return NS_OK;
}

Comment 7

[Kershaw Chang [:kershaw]]

(In reply to Christian Holler (:decoder) from comment #3)

Apparently, this is this code failing:

https://searchfox.org/mozilla-central/rev/8b0666aff1197e1dd8017de366343de9c21ee437/netwerk/protocol/data/DataChannelParent.cpp#103-105

kershaw, do you know what the security implications are if LoadInfoArgsToLoadInfo fails here?

Looks like the errors are all related to principals, but I am not sure what's the security implications.
Maybe Nika knows?

Comment 8

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

The loadInfo object which would normally be deserialized from the IPC connection and put into the channel would be nullptr instead of a nsILoadInfo pointer (https://searchfox.org/mozilla-central/rev/891d104826fb0cfd5cbdd6128e2372ce62810028/netwerk/protocol/data/DataChannelParent.cpp#103). Since bug 1528677, we now assume everywhere that we have a LoadInfo on all channels (so never null-check the read), this will probably lead to a null deref at some point in some code which is expecting a non-null LoadInfo.

This code should perhaps be changed to check for an error, and return an IPC_FAIL(...) if it failed.

That being said, this function does have an unrelated problem, which is that the security-sensitive aOriginProcess second argument was set to NOT_REMOTE_TYPE when de-serializing a LoadInfo being sent by a content process (thus treating the LoadInfo as-if it was sent by the parent process). This is used to track which remote type is responsible for a load being initialized for the triggeringRemoteType member, so we should make sure to get it right even if this particular caller isn't super likely to use it.

nsAutoCString remoteType;
nsresult rv = GetRemoteType(remoteType);
if (NS_FAILED(rv)) {
  return IPC_FAIL(this, "Failed to get remote type");
}

rv = mozilla::ipc::LoadInfoArgsToLoadInfo(
    aDataChannelInfo.loadInfo(), remoteType, getter_AddRefs(loadInfo));
if (NS_FAILED(rv)) {
  return IPC_FAIL(this, "Failed to deserialize LoadInfo");
}

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Bug 1911747 - Return IPC_FAIL when failed to convert loadinfo, r=#necko

Comment 10

[Andrew McCreight [:mccr8]]

I'll mark this as sec-other. Comment 8 says this is a null deref but maybe this relates to some other more serious security issue?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/860fc0861d12
Return IPC_FAIL when failed to convert loadinfo, r=necko-reviewers,valentin

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/860fc0861d12

Comment 13

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

The patch landed in nightly and beta is affected.
:kershaw, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox131 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Bug 1911747 - Return IPC_FAIL when failed to convert loadinfo, r=#necko

Original Revision: https://phabricator.services.mozilla.com/D218814

Comment 15

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Could crash. Since loadInfo is null, we might crash at somewhere.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: Straightforward change.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/54439231b182