1764731 - Assertion failure: IsValid(), at dom/serviceworkers/ServiceWorkerRegistrationDescriptor.cpp:59

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Christian Holler (:decoder)]

In experimental IPC fuzzing, we found the following diagnostic assert on mozilla-central revision 4d80f4e1809a+:

Assertion failure: IsValid(), at dom/serviceworkers/ServiceWorkerRegistrationDescriptor.cpp:59

==31730==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fba9385f9f2 bp 0x7fb9feeb2c50 sp 0x7fb9feeb2c30 T6)
    #0 0x7fba9385f9f2 in mozilla::dom::ServiceWorkerRegistrationDescriptor::ServiceWorkerRegistrationDescriptor(mozilla::dom::IPCServiceWorkerRegistrationDescriptor const&) dom/serviceworkers/ServiceWorkerRegistrationDescriptor.cpp:59:3
    #1 0x7fba938703bb in mozilla::dom::ServiceWorkerRegistrationParent::Init(mozilla::dom::IPCServiceWorkerRegistrationDescriptor const&) dom/serviceworkers/ServiceWorkerRegistrationParent.cpp:141:7
    #2 0x7fba8b36f18e in mozilla::ipc::BackgroundParentImpl::RecvPServiceWorkerRegistrationConstructor(mozilla::dom::PServiceWorkerRegistrationParent*, mozilla::dom::IPCServiceWorkerRegistrationDescriptor const&) ipc/glue/BackgroundParentImpl.cpp:1363:3
    #3 0x7fba8b50e3cf in mozilla::ipc::PBackgroundParent::OnMessageReceived(IPC::Message const&) objdir-ff-asan-vanilla/ipc/ipdl/PBackgroundParent.cpp:6357:28
    [...]
    #16 0x7fbaa9c7571e in __clone /build/glibc-S9d2JN/glibc-2.27/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

We can reproduce the issue locally but can't provide a testcase yet. The attached log has some raw data about the packets being sent.

I am concerned about this because these are diagnostic asserts that seem to check something about principalInfo which could indicate a security-relevant check that might not be present anymore in release builds. Marking s-s until investigated. If this turns out to be harmless, please keep it hidden anyway until the parent bug is opened up.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Jens Stutte [:jstutte]]

Looking at ServiceWorkerRegistrationDescriptor::IsValid there seem to be several conditions that can lead to this. :decoder, if you can reproduce this, a pernosco session would be very helpful, I think. Thank you!

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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

I am concerned about this because these are diagnostic asserts that seem to check something about principalInfo which could indicate a security-relevant check that might not be present anymore in release builds. Marking s-s until investigated. If this turns out to be harmless, please keep it hidden anyway until the parent bug is opened up.

We haven't systemically addressed bug 1491018 so it's definitely the case that a compromised content process can tell the parent process lies. That said, there are some attempts at checks in the ServiceWorker vicinity, but this IsValid check is more about ensuring there are no logic bugs as opposed to an adversarial check.

I don't see a parent bug right now?

Comment 4

[Daniel Veditz [:dveditz]]

I don't see a parent bug right now?

It's a fuzzing meta bug.

Guessing this is more an "audit" situation if we haven't addressed some known issues

Comment 5

[Christian Holler (:decoder)]

I managed to reproduce this once more and now I can replay it and managed to replay in rr:

https://pernos.co/debug/WTMEKTNUY9TKFzjgPIlbhA/index.html

Comment 6

[Jens Stutte [:jstutte]]

Thanks! From a short look at the session it seems, BackgroundParentImpl::RecvPServiceWorkerRegistrationConstructor should better check the IPCServiceWorkerRegistrationDescriptor content for plausible/accepted values and return IPC_FAIL in case. Here we come in with mType=TSystemPrincipalInfo which seems to lead to the assertion later.

Comment 7

[Christian Holler (:decoder)]

I did a quick search and I could only find the isValid check being called from within diagnostic asserts. Does that mean that we never actually check that the service worker has a TContentPrincipalInfo? Does this lead to privilege escalation given that we have a service worker then with system principal?

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

System Principal Risk Analysis

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

Does this lead to privilege escalation given that we have a service worker then with system principal?

No, the PServiceWorkerRegistration protocol is a subscription mechanism only used so that ServiceWorkerRegistration binding instances can be live-updating. It cannot create ServiceWorkers. And the subscription attempt will fail because ServiceWorkerManager::PrincipalInfoToScopeKey checks the PrincipalInfo type in non-debug code and returns an error code. This will propagate back through the layers of SWM::GetRegistration, ServiceWorkerRegistrationProxy::InitOnMainThread, ServiceWorkerRegistrationParent::Init, and InitServiceWorkerRegistrationParent.

Practically speaking, I think it would be quite difficult to get the browser to launch a system principaled ServiceWorker because the assumption that we are only dealing with content principals is baked in so deeply and we do have checks along the critical path. Specifically:

• When loading from disk, we explicitly create a ContentPrincipalInfo.
• When converting the scope to a principal, we explicitly call CreateContentPrincipal.
• When given a Principal and converting it to a scope key, we explicitly check that it's a content principal.

In particular, the way ServiceWorkers are created is via a call to ServiceWorkerContainer.register which then sends the request to the parent process. The parent process then exclusively decides what to do. The flow is checked in the following steps (and pretty diagram for the calls into SWM::register):

• ServiceWorkerContainerProxy::Register calls into SWM::Register without checking anything itself.
• In SWM::Register:
  • We call mozilla::dom::ServiceWorkerScopeAndScriptAreValid which checks the scope URI and script URI are appropriate schemes and that the principal may load the URIs. This probably wouldn't do much since the system principal can obviously load everything, but it does mean that any attempt has to at least be using an http(s) or webext URI (if enabled) for the payload script.
  • We call mozilla::dom::ServiceWorkerManager::PrincipalToScopeKey mentioned above which explicitly errors out because the system principal is not a content principal.

This specific asssertion trip and pernosco trace

The pernosco trace seems to indicate the IPC fuzz is explicitly on a PServiceWorkerRegistration message and I don't see any prior SendPServiceWorkerRegistration* calls, so I don't think there's anything particularly actionable there.

Suggested action

I filed bug 1853706 to codify what we've discussed for a while in terms of having a chain of trust for principals, with bug 1853726 specifically covering the PServiceWorkerRegistration protocol being fuzzed here. I'm making this bug depend on the specific SW bug 1853726 in order to not lose track of the analysis I've done here, but in practice I don't think there's really anything to be done here. That said, if it would help the fuzzer for us to call IPC_FAIL earlier like :jstutte proposes in comment 6, I'm fine to r+ such a patch.

Comment 9

[Christian Holler (:decoder)]

It would also be suitable for the fuzzer to just disable the diagnostic assert under IPC fuzzing. Or are there other situations where this particular assert would be useful to keep around?

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

As noted in comment 3 I think the assert is mainly a data-structure invariant check to guard against human error, so if there's a way for the fuzzer infrastructure to easily quiet that assert only for fuzzing, that seems fine. I would like to retain the assertion in general.

Obviously, there still is the meta issue where, until we address bug 1853726, the IPC fuzzer may just find other things that will be mooted by or non-actionable until that bug is addressed.

Comment 11

[Christian Holler (:decoder)]

Attachment: Bug 1764731 - Disable service worker assert for IPC fuzzing. r?asuth

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9d1c7897b94
Disable service worker assert for IPC fuzzing. r=asuth

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/f9d1c7897b94