[2] NetworkBroadcastChannelRegistry crashes on a BroadcastChannel message with a null name

MediumWebKit NetworkProcessMemoryCorruption

e85a1ea

Medium, and capped there by the shape of the fault: a fixed-offset load from an unconditionally null base, with no write and no attacker-chosen address. What keeps it above Low is the target — the network process is shared by the entire session, and one message from any WebProcess takes it down.

WebKit runs network traffic in a separate, more privileged process that holds cookies, credentials, and cache state, and every message it accepts from a content process is attacker-influenced once that content process is compromised. BroadcastChannel is the web API that lets same-origin browsing contexts exchange messages over an author-named channel; because the peers may live in different content processes, the network process brokers the traffic through a registry keyed on (origin, channel name) pairs. The contract that registry depends on is that a channel name arriving over IPC is a usable hash-table key.

The angle: a compromised WebProcess sends one RegisterChannel message with a null name and deterministically terminates the network process for every tab in the session.

NetworkBroadcastChannelRegistry uses the IPC-supplied channel name as a HashMap<String, ...> key in registerChannel() (via ensure()) and in unregisterChannel() / postMessage() (via find()). A compromised or malformed WebProcess could send a null String for the name. Looking up a null String key dereferences a null StringImpl while hashing the key (StringHash::hash() calls key.impl()->hash()), crashing the network process before HashTable::validateKey() ever runs. Reject a null name with a MESSAGE_CHECK in all three endpoints, matching the existing origin validation.

Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.cpp

LayoutTests/ipc/register-broadcast-channel-malformed-client-origin-crash.html

LayoutTests/ipc/coreipc.js

Patch Details

Each of the three IPC entry points gains exactly one line of input validation on the name argument: MESSAGE_CHECK(!name.isNull(), connection) for the two synchronous handlers, and MESSAGE_CHECK_COMPLETION(!name.isNull(), connection, completionHandler()) for postMessage, which must still discharge its reply. The new checks sit immediately after the pre-existing isValidClientOrigin(origin) checks. No other production logic changed — the ensure(name, ...) insert and the find(name) lookups are untouched. The coreipc.js ArgumentSerializer is extended so a JS null passed where a String is expected serializes as a null String rather than throwing SerializationError, which is what lets the regression test send name: null at all.

Missing boundary validation of a cross-process string argument that is representable-but-illegal as a hash-table key, whose hash function assumes the sentinel can never reach it.

  Before:                                   After:
  RegisterChannel(origin, name)             RegisterChannel(origin, name)
    |- MESSAGE_CHECK(validOrigin)             |- MESSAGE_CHECK(validOrigin)
    |                                         |- MESSAGE_CHECK(!name.isNull())
    `- ensure(name, ...)                      |     `-> terminate sender
         `- StringHash::hash(name)            `- ensure(name, ...)
              `- name.impl()->hash()               `- impl() guaranteed non-null
                   ^ impl() == nullptr
                     -> fault, network process dies

Background

Where this lives. NetworkBroadcastChannelRegistry is the broker that lets same-origin BroadcastChannel instances in different WebProcesses find each other. It keeps a HashMap<ClientOrigin, HashMap<String, Vector<IPC::Connection::UniqueID>>> mapping (origin, channel name) pairs to subscribed connections, and relays postMessage payloads to every other subscriber. It receives RegisterChannel, UnregisterChannel, and PostMessage, each carrying (ClientOrigin origin, String name, ...).

Null String vs. empty String. A WTF String holds a RefPtr<StringImpl>. A null String has impl() == nullptr; an empty String points at a valid zero-length StringImpl. String::isNull() distinguishes them, and the IPC decoder can legitimately produce either — null is a representable value on the wire.

StringHash and hash-table traits. StringHash::hash(const String&) computes key.impl()->hash(). WTF's HashTraits<String> uses the null String as the table's empty value, so by contract a null String is never a legal key and the hash function carries no null check. Both HashMap::ensure() and HashMap::find() hash the key before touching any bucket. HashTable::validateKey() is the debug-build check that flags use of the empty or deleted key value.

MESSAGE_CHECK / MESSAGE_CHECK_COMPLETION. WebKit's IPC validation macros: on a false condition they log and terminate the sending connection's WebProcess, treating the malformed message as evidence of compromise. The _COMPLETION variant additionally invokes the reply handler so an async message does not leave a dangling completion.

IPC testing API. A test-only facility (IPCTestingAPIEnabled=true) exposing window.IPC so a layout test can hand-construct raw IPC messages, standing in for a compromised WebProcess. LayoutTests/ipc/coreipc.js is its JS-side argument serializer.

Analysis

The root cause is an unenforced type invariant at an IPC boundary: a String used as a hash key must be non-null, and nothing enforced that. The handlers validated the ClientOrigin argument but placed no constraint on name, then used it directly as a key. With name null, impl() returns nullptr and the load of the hash field off that null base faults, taking down the process. This happens before validateKey() — the debug-only illegal-key assertion — would report anything, so on release builds the observable behaviour is a hard crash rather than an assertion. The lookup would be illegal even if hashing were null-safe, since inserting the empty value as a key breaks the table's empty-bucket bookkeeping; the fix therefore rejects the value at the boundary rather than making the lookup tolerant.

Reachability is not from ordinary web content: the BroadcastChannel bindings coerce the channel name to a DOMString, so a null String cannot be produced through the normal WebCore path. The attacker must already be able to emit arbitrary IPC on the WebProcess→NetworkProcess connection — WebProcess code execution, or the IPC testing API as in the regression test. Given that position, the trigger is a single message: send RegisterChannel with a syntactically valid ClientOrigin (the test uses an opaque topOrigin plus a SecurityOriginData::Tuple clientOrigin with a garbage host, which passes isValidClientOrigin because neither origin is null) and name: null. unregisterChannel and postMessage reach the same fault through find(name).

Escalation beyond process termination would require the null page to be mappable in the network process and the faulting offset to be attacker-influenced; neither holds, since the offset is a compile-time member offset in StringImpl and the base is unconditionally nullptr. No read, write, or type-confusion primitive is available, and the fault aborts before any bucket state is mutated.

This vulnerability weakens the WebContent→Network process trust boundary at its input-validation layer. WebKit's IPC security model assumes every handler in a higher-privilege process fully validates arguments from a potentially compromised WebProcess before feeding them into data structures with narrower invariants than the wire format admits. The plausible attacker gain is availability: reliable termination of a process shared across the whole browser session, disrupting every tab and forcing teardown and restart of network state.

Insight: this is a boundary instance of a broader WTF contract mismatch. The IPC String decoder's value domain (null | empty | non-empty) is strictly wider than the domain HashMap<String, ...> accepts as a key (non-null only), and StringHash::hash() intentionally omits the null check because the hash-table contract declares null to be the empty value. The class is systematically invisible to release-build testing and to ASan — a null-base load is a plain segfault, not a heap error — and the debug-only validateKey() never fires because the hash happens first. The pre-existing isValidClientOrigin() check in this very file is the same discipline applied correctly to a different argument; it simply was not extended to every hash-key argument.

Audit directions

• An IPC-decoded value whose wire domain is wider than the domain the receiving container accepts as a key, where the key-hashing path assumes the sentinel can never arrive. Narrow: grep Source/WebKit/NetworkProcess and Source/WebKit/GPUProcess for message handlers taking a const String& that reaches .ensure(, .find(, .add(, .set(, .get(, .contains(, or .remove( on a HashMap/HashSet member without a preceding MESSAGE_CHECK(!x.isNull()) — start with the registries that mirror this one's shape (NetworkStorageManager, WebSharedWorkerServer, NetworkMDNSRegister, ServiceWorker name/scope maps). Wider: any sentinel-valued key type — AtomString, RefPtr/WeakPtr (null is the empty value), ObjectIdentifier (zero is reserved), URL. Widest: the deserializer's value domain must be intersected with the consumer's legal-value domain at the boundary, not assumed to match — the same question applies to Mojo handlers feeding base::flat_map or any RPC layer whose codegen permits nullable strings for a required field. Code-review tell: a handler that validates some argument but lets the String key argument reach the hash call unguarded.
• Release-build-invisible key validation. WTF's HashTable::validateKey()/checkKey() empty-and-deleted-key assertions are debug-only, and for String keys the hash is computed before them, so illegal-key bugs neither assert nor trip sanitizers in shipping configurations. Narrow: enumerate HashTraits specializations in Source/WTF/wtf whose emptyValue()/constructDeletedValue() are values an IPC decoder can produce, and grep for handlers using those types as keys. Wider: any container whose precondition is enforced only under ASSERT_ENABLED. Widest: a precondition enforced only in debug builds is not enforced on the attack surface — ask, in any codebase, which invariants on untrusted-input paths are backed solely by assert/debug_assert/NDEBUG-gated checks.
• Completion-handler discipline on newly added async validation. postMessage uses MESSAGE_CHECK_COMPLETION precisely because an early return must still run completionHandler(). Narrow: grep Source/WebKit for MESSAGE_CHECK( inside functions whose signature includes a CompletionHandler<...>&& parameter — each is a candidate dropped reply. Wider: any early-exit path that skips a mandatory one-shot obligation, including CallbackAggregator scopes exited before creation and sendWithAsyncReply paths that return without invoking the reply. Widest: every exit path from a function holding a one-shot obligation must discharge it exactly once — the same audit applies to Promise executors that return without settling and Rust oneshot::Sender values dropped on error paths. Code-review tell: a CompletionHandler parameter in the signature with a plain MESSAGE_CHECK above the first use of the handler.