[5] Signed integer overflow (UB) in back/forward list index handling

LowWebKit back/forward listIntegerOverflow

a6cd3ca

Low. The pathological values are still rejected by the surrounding range checks on any conventional wrapping build, so there is no demonstrated primitive. What earns it a rating at all is location: a UIProcess bounds check on renderer-supplied input whose correctness rested on the toolchain declining to exploit the UB rather than on the language contract.

Session history is owned by the privileged parent process, and the renderer asks for entries by relative offset — "two back", "one forward". Two range checks over that signed offset did signed arithmetic on a value whose full int32_t domain is reachable from the untrusted side. The expectation those checks encoded is that ordinary signed arithmetic on a relative offset behaves like mathematical arithmetic, which holds for every input except the edges of the type.

The angle: a compromised renderer can hand the parent process an INT32_MIN or near-INT32_MAX delta and drive a bounds check whose behavior the source no longer determines — an abort on trap-instrumented builds, and on an optimizer that exploits the assumption, a check that admits an out-of-range index.

The commit message states the case directly: when index or distance is INT_MIN, the expression static_cast<unsigned>(-index) negates in signed arithmetic before casting, which is undefined behavior, and a compromised WebProcess can send INT32_MIN via the BackForwardItemAtIndex IPC message to trigger UB in the UIProcess. The fix adds a MESSAGE_CHECK rejecting INT_MIN at the boundary and, as defense in depth, casts to unsigned before negating in both WebBackForwardList::itemAtIndex() and BackForwardController::canGoBackOrForward().

Source/WebKit/UIProcess/WebBackForwardList.cpp

Source/WebCore/history/BackForwardController.cpp

Patch Details

In BackForwardController::canGoBackOrForward(int distance), the negative-distance branch changes from static_cast<unsigned>(-distance) <= backCount() to -static_cast<unsigned>(distance) <= backCount() — the negation now happens after the conversion to unsigned.

In WebBackForwardList::itemAtDeltaFromCurrentIndex(int delta, AllowSkippingBackForwardItems), the combined guard if (!m_currentIndex || (int)*m_currentIndex + delta < 0) splits into a null check on m_currentIndex followed by a range check that performs no arithmetic on delta in signed space: if (delta < 0 && -static_cast<unsigned>(delta) > *m_currentIndex) return nullptr;, with a comment stating the intent. m_currentIndex is std::optional<size_t>, so the old code narrowed a size_t to int and then added the renderer-supplied delta in signed space; the new code compares an unsigned magnitude against the size_t directly and, for non-negative delta, performs no arithmetic at all.

The IPC handler backForwardItemAtIndexForWebContent gains a leading IPC::Connection& parameter — in both the definition and the private declaration in WebBackForwardList.h — which is what lets it use the connection-aware validator macro. Its first statement is now MESSAGE_CHECK_COMPLETION_BASE(delta != std::numeric_limits<int32_t>::min(), connection, completionHandler(nullptr));, rejecting the single input value whose negation is not representable in int32_t before delta reaches itemAtDeltaFromCurrentIndex. There are no test files in the diff.

Negating a signed value before widening it to unsigned, so the absolute-value conversion inside a bounds check is undefined at exactly the type minimum.

Background

Signed integer overflow is undefined behavior in C++. For a two's-complement int, the range is asymmetric: INT_MIN is -2147483648 while INT_MAX is 2147483647, so -INT_MIN has no representable result, and an addition whose mathematical sum leaves the range is likewise undefined. Compilers are permitted to optimize on the assumption that UB never occurs, which means the surrounding code's behavior at those inputs is not determined by reading the source.

Unsigned arithmetic is defined and modular. Conversions to and arithmetic on unsigned wrap modulo 2^N by the standard, so -static_cast<unsigned>(x) is always well defined and, for negative x, equals the mathematical magnitude of x.

Operator binding. In static_cast<unsigned>(-x) the unary minus applies to x in its original signed type and the cast happens afterward; in -static_cast<unsigned>(x) the conversion happens first and the negation is unsigned. The two spellings differ only at the type minimum.

WebKit's multi-process model. The WebProcess renders untrusted web content and is treated as potentially compromised. The UIProcess is the privileged parent that owns real session history. WebBackForwardList in the UIProcess is an IPC::MessageReceiver; the WebProcess asks it for history entries by relative offset.

MESSAGE_CHECK family. These macros validate IPC-supplied arguments in the receiving process; when a check fails, the sending process is treated as misbehaving. MESSAGE_CHECK_COMPLETION_BASE is the variant used when the handler owns a CompletionHandler that must still be invoked — here with nullptr — before bailing out. Handlers opt into these by declaring an IPC::Connection& first parameter, which the generated message dispatcher supplies.

Back/forward list shape. WebBackForwardList holds BackForwardListItemVector m_entries and std::optional<size_t> m_currentIndex; a delta of -1 means the previous entry, +1 the next, and DefaultCapacity caps the list at 100 entries. On the WebCore side, BackForwardController::backCount() / forwardCount() return unsigned counts.

Analysis

Two sites did signed arithmetic on an offset whose full domain is reachable, in two different ways.

In BackForwardController::canGoBackOrForward (WebCore, WebContent process), the unary minus in static_cast<unsigned>(-distance) binds to the int operand, so for distance == INT_MIN the negation is evaluated as int arithmetic and -INT_MIN is not representable.

In WebBackForwardList::itemAtDeltaFromCurrentIndex (UIProcess), the overflow reachable is not INT32_MIN — adding a small non-negative index (the list caps near 100) to INT_MIN stays representable and yields a negative sum, so the pre-fix guard correctly rejected it. The undefined case here is a large positive delta: INT32_MAX + *m_currentIndex overflows int. On a wrapping build the sum turns negative and the guard rejects — fail-closed — but the operation is UB and the compiler is entitled to assume it cannot happen.

  Before (UIProcess):                  After (UIProcess):
  (int)*m_currentIndex + delta < 0     !m_currentIndex ──► nullptr
        │ signed add, UB at            delta < 0 ?
        │ large positive delta           ├─ yes ──► -(unsigned)delta > *m_currentIndex ──► nullptr
        └──► reject / accept                └─ no  ──► no arithmetic on delta at all

Because these are UB rather than merely wrong, the concrete behavior at those inputs is not fixed by the source — an optimizer may propagate the "cannot happen" assumption backwards into the surrounding comparison, e.g. treating -distance as provably positive and folding the <= against an unsigned count. On a conventional two's-complement build with wraparound and no such folding, -INT_MIN wraps back to INT_MIN and the conversion to unsigned produces 0x80000000, far larger than any plausible backCount(), so the check would still reject. What the patch restores is that both range checks have defined behavior across the entire int32_t domain.

Reachability is established from the diff and header: backForwardItemAtIndexForWebContent is declared under the // IPC messages block in WebBackForwardList.h on a class deriving from IPC::MessageReceiver, so a compromised WebProcess can invoke it with any int32_t. The WebCore-side canGoBackOrForward and goBackOrForward run inside the WebContent process; script-driven relative navigation is the natural producer of their distance argument, though the supplied BackForwardController.cpp context does not include the binding layer that connects them to a web-facing API.

Escalation is bounded and build-dependent. If the toolchain lowers the addition with two's-complement wraparound and performs no UB-derived folding, the sum turns negative and the guard still returns nullptr — a rejected lookup with no state change. If the UIProcess build is UBSan- or -ftrapv-instrumented, the addition would trap and the parent process would abort, giving a compromised renderer a browser-wide denial of service. If instead an optimizer exploited the distance < 0 implies -distance > 0 assumption in canGoBackOrForward and folded the comparison against backCount() to true, that function could report a navigation as permissible when it is not, and the downstream itemAtIndex(distance) in goBackOrForward would then be reached with an out-of-domain index; whether that could reach an out-of-range list access depends on BackForwardClient::itemAtIndex implementations the supplied context does not include. That path is confined to the WebContent process, where the attacker already has execution in this threat model.

This vulnerability weakens the WebContent-to-UIProcess IPC boundary, specifically the assumption that integers arriving from a compromised renderer stay inside the domain where the UIProcess's index arithmetic is well defined. The validity of a UIProcess bounds check on renderer-controlled input rested on compiler behavior rather than on the language contract. The separate WebCore-side negation UB sits inside the WebContent process, so its trapping outcome would abort only the renderer the attacker already controls and crosses no boundary. No corruption primitive is demonstrated, and both sites still reject the pathological values on conventional wrapping builds, so this reads as hardening of a privileged-process input path rather than a live escape path.

The static_cast<unsigned>(-x) idiom is a near-universal way of writing "take the magnitude of this negative index," and it is wrong at exactly one input. Codebases that mix signed relative offsets — deltas, distances, scroll amounts, seek positions — with unsigned or size_t container sizes accumulate the idiom densely, because the cast is what silences the sign-compare warning, and the cast placement that silences the warning is also the placement that keeps the UB. The correct spelling looks stranger and is therefore rarer. The second lesson is in the shape of the UIProcess rewrite: rather than fixing the arithmetic, the patch removes the arithmetic. "Do range checks without doing math on the untrusted value" is a stronger discipline than "do the math carefully," because it does not need re-verification when the surrounding types change.

Audit directions

• Absolute-value conversion with the negation inside the cast. The invariant is converting a signed magnitude to unsigned must widen first and negate second, because the signed domain is asymmetric. Narrow: grep WebCore and WebKit for static_cast<unsigned>(-, static_cast<size_t>(-, and static_cast<uint64_t>(- — the code-review tell is a unary minus inside the cast parentheses on a signed variable; every hit where the operand can reach the type minimum is a candidate. Wider: the same class hides behind other magnitude idioms — abs() / labs() on an int (explicitly undefined at INT_MIN), std::abs on the difference of two signed indices, and hand-rolled x < 0 ? -x : x ternaries; search for those adjacent to any <= / < comparison against a .size(), a count(), or a capacity, since that pairing is what turns a numeric wart into a bounds-check wart. Widest: any language with fixed-width two's-complement integers — Rust's i32::abs panics at i32::MIN in debug builds (is checked_abs / unsigned_abs used?), Java's Math.abs(Integer.MIN_VALUE) silently returns a negative number. Carry the tell: any expression computing a magnitude from a signed value and immediately comparing it to a length — ask whether the type minimum is reachable from untrusted input.

• IPC handlers accepting raw signed integers as list offsets. The class is handlers that treat a wire-format integer as a value in a narrower semantic domain than its declared type. Narrow: enumerate the handlers declared under the // IPC messages comment blocks in UIProcess headers — start with WebBackForwardList.h, which lists backForwardGoToItem, backForwardAllItems, backForwardListContainsItem, backForwardListCounts — and check which take int32_t / int64_t parameters without a corresponding MESSAGE_CHECK; the tell is a signed parameter used in index or size arithmetic with no validator macro as the first statement. Wider: the same class covers any deserialization boundary where a type's full range is accepted but only a sub-range is meaningful — IPC::Decoder custom decode implementations, SessionState / FrameState restore paths, sandbox-message argument parsing; the shape to notice is a decoded scalar flowing into arithmetic before any range predicate. Widest: wire-type-wider-than-semantic-type applies to Chromium Mojo interfaces, protobuf int32 fields, and any RPC layer where the IDL scalar range exceeds the handler's assumed domain. For each decoded integer, name the sub-range the handler actually accepts, then check whether anything enforces it before first use.

• Rewritten validity predicates that narrow their own scope. The class is a fixed predicate that silently admits inputs the sloppier original rejected as a side effect. Narrow: in WebBackForwardList::itemAtDeltaFromCurrentIndex, the old (int)*m_currentIndex + delta < 0 also rejected very large positive deltas under wrapping, while the new guard runs only when delta < 0; trace the remainder of that function and its AllowSkippingBackForwardItems paths to confirm the forward direction is bounded against m_entries.size() on its own — the tell is any use of *m_currentIndex + delta as an index without a prior comparison to the vector length. Wider: the same drift appears wherever a canX() predicate and a doX() action are separately reachable from script or IPC — history navigation, editing command isEnabled/execute pairs, media canPlayType/load pairs; look for an action method that re-derives its own bounds instead of consulting the predicate. Widest: guard-and-guarded-operation-separately-reachable applies to POSIX access() before open(), permission checks in web APIs, validator/executor splits in bytecode VMs — if the predicate can be skipped by calling the action directly, the action must repeat every bound the predicate asserted.

• UB whose observable severity is set by build flags rather than by the source. Narrow: check the sanitizer and -fno-strict-overflow / -ftrapv settings applied to the WebKit UIProcess targets, then re-rank the outstanding static_cast<unsigned>(-x) and signed-addition hits from the first direction by which process they live in — a UIProcess hit under a trapping build is a renderer-triggerable parent-process abort, while the same hit in WebCore aborts only the already-compromised renderer. Wider: the same reasoning applies to every UB class whose runtime effect is flag-dependent — unaligned loads, strict-aliasing violations, INT_MIN / -1 division; the shape to notice is any audit finding whose severity write-up contains "depends on the compiler." Widest: for a UB finding in a privileged process, the severity ceiling is set by the hardening configuration, so establish the build configuration before ranking — holds for any codebase shipping sanitizer-hardened or panic=abort builds, including Rust services where an arithmetic overflow that wraps in release aborts in debug.