2020481 - Heap OOB write in Key::MaybeUpdateAutoIncrementKey via missing bounds check on mAutoIncrementKeyOffsets (sandbox escape)

Status: RESOLVED DUPLICATE of bug 2014101

(Core :: Storage: IndexedDB, defect)

Description

[TheKng]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/145.0.0.0 Safari/537.36

Steps to reproduce:

Heap OOB write in Key::MaybeUpdateAutoIncrementKey — sandbox escape via IPC

Found via source code audit. Three related bugs that together allow a compromised content process to achieve arbitrary heap writes in the parent process.

Bug 1: Missing bounds check in Key::MaybeUpdateAutoIncrementKey
File: dom/indexedDB/Key.cpp, lines 588-601

void Key::MaybeUpdateAutoIncrementKey(int64_t aKey) {
  if (mAutoIncrementKeyOffsets.IsEmpty()) {
    return;
  }
  for (uint32_t offset : mAutoIncrementKeyOffsets) {
    char* buffer;
    MOZ_ALWAYS_TRUE(mBuffer.GetMutableData(&buffer));
    buffer += offset;                          // NO BOUNDS CHECK
    WriteDoubleToUint64(buffer, double(aKey)); // 8-byte write at arbitrary offset
  }
  TrimBuffer();
}

The function iterates mAutoIncrementKeyOffsets and writes 8 bytes at each offset into mBuffer WITHOUT verifying that offset + 8 <= mBuffer.Length().

Bug 2: Zero validation in IPC deserialization
File: dom/indexedDB/SerializationHelpers.h, lines 37-40

static bool Read(MessageReader* aReader, paramType* aResult) {
    return ReadParam(aReader, &aResult->mBuffer) &&
           ReadParam(aReader, &aResult->mAutoIncrementKeyOffsets);
    // NO validation that offsets are within mBuffer bounds
}

ParamTraits<Key>::Read deserializes both mBuffer and mAutoIncrementKeyOffsets from IPC with zero cross-validation.

Bug 3: Inverted validation logic in AllocCursor
File: dom/indexedDB/ActorsParent.cpp, line 10630

// INCORRECT - validates only when we TRUST (inverted logic)
if (aTrustParams && NS_AUUF_OR_WARN_IF(!VerifyRequestParams(
                        commonParams.optionalKeyRange()))) {

Compare with the correct pattern at line 10486:

// CORRECT - validates when we do NOT trust
if (NS_AUUF_OR_WARN_IF(!aTrustParams && !VerifyRequestParams(aParams))) {

In e10s/Fission, aTrustParams is false for out-of-process content, so the inverted condition means optionalKeyRange is NEVER validated for content process requests.

Attack chain:

1. Compromised content process creates IndexedDB database with autoIncrement object store
2. Crafts ObjectStoreAddParams IPC message with:
   • indexUpdateInfos[0].value.mBuffer = short string (10 bytes)
   • indexUpdateInfos[0].value.mAutoIncrementKeyOffsets = [0x10000] (beyond buffer)
3. Parent process receives via PBackgroundIDBObjectStore::AddRequest
4. MaybeUpdateAutoIncrementKey writes 8 bytes at offset 0x10000 past heap buffer
5. HEAP OOB WRITE in parent process = sandbox escape

Suggested fix:

1. Add bounds check in MaybeUpdateAutoIncrementKey:

if (offset + sizeof(uint64_t) > mBuffer.Length()) {
    MOZ_ASSERT_UNREACHABLE("Invalid auto-increment key offset");
    return;
}

2. Add validation in ParamTraits<Key>::Read:

for (uint32_t offset : aResult->mAutoIncrementKeyOffsets) {
    if (offset + sizeof(uint64_t) > aResult->mBuffer.Length()) {
        return false;
    }
}

3. Fix inverted condition at line 10630: change aTrustParams && to !aTrustParams &&

Actual results:

A compromised content process can write 8 bytes of partially-controlled data at attacker-chosen offsets in the parent process heap, bypassing the content process sandbox. Multiple offsets can be specified in a single IPC message for multiple writes.

Expected results:

MaybeUpdateAutoIncrementKey should validate that each offset in mAutoIncrementKeyOffsets is within mBuffer bounds before writing. The IPC deserializer should validate offset/buffer relationships. AllocCursor should validate when aTrustParams is false (matching the pattern in AllocRequest).

Comment 1

[Andrew McCreight [:mccr8]]

This has been at least partially fixed already by bug 2014101, which is fixed in 148, 149, and the forthcoming ESR140 release.

Comment 2

[Andrew McCreight [:mccr8]]

Jari, can I dupe this to bug 2014101? This bug (and the other bug) talk about adding validation to ParamTraits<Key>::Read but maybe that's redudant with the other checks you added there? Thanks.

Comment 3

[TheKng]

hey, thanks for checking. yeah i saw bug 2014101 but i'm not sure it covers everything here.

the main thing i'm worried about is the inverted condition in AllocCursor at ActorsParent.cpp line 10630 - it says aTrustParams && but if you look at line 10486 the same pattern uses !aTrustParams &&. so right now it only validates when we trust the params, which is backwards. that one seems like a separate issue from what 2014101 fixed.

also even if the IPC deserialization now checks the offsets, MaybeUpdateAutoIncrementKey itself still doesn't do any bounds checking before writing. would be good to have both as defense in depth.

let me know if you need me to check anything else.

Comment 4

[Andrew McCreight [:mccr8]]

The patch does fix the inverted condition in TransactionBase::AllocCursor. You can see it at the top of the patch here: https://github.com/mozilla-firefox/firefox/commit/18e346fda127

Comment 5

[Jari Jalkanen [:jari]]

The findings are identical to what was in the bug 2014101, this can be duped to it. Checking right after IPC would have been the more conventional way to do this, agreed, but it was not done in the context of that bug fix because it's a part of larger story.