clfs.sys ClfsValidateBlock sector count OOB

CLFS stores a log in a .blf made of fixed-size 512-byte sectors. Every block opens with a CLFS_LOG_BLOCK_HEADER whose first count-bearing fields are a 16-bit TotalSectorCount at +0x04 — the number of sectors the header claims this block contains — and a sibling 16-bit ValidSectorCount at +0x06 that the validator requires be at least TotalSectorCount. Inside each sector the last two bytes carry a stamp: a signature byte at +0x1FE within the sector whose high bit marks the sector BAAD (torn write or unused), and an own-tag byte at +0x1FF that every live sector of a block must share. After reading a block off disk the kernel walks those stamps to decide which sectors are live and which it must recover against the shadow copy.

ClfsValidateBlock is the gatekeeper. Every block read from a .blf passes through it before CLFS acts on the contents; once it returns success, the restart-area, container-table, and record-offset fields inside the block get trusted. And the .blf is attacker-writable: a low-privilege user can open a log file, rewrite the header to anything it likes, and force the kernel to re-read it. So the on-disk TotalSectorCount at +0x04 is not a count the kernel produced. It is whatever the attacker typed, and the validator’s job is to confirm that what the header claims matches what the buffer contains before the rest of the code trusts either.

The vulnerable build does perform a bound check — that is not where the bug is. The bug is that the bound check and the loop body read TotalSectorCount from the buffer at two different points in time. The bound check loads the field once into a register for the comparison against cbBuffer, then discards it. The loop body, on every iteration, reloads the field from the buffer to compute its exit condition. So the count the kernel measured against cbBuffer and the count the kernel actually honours when it decides whether to keep walking are two independent snapshots of attacker-controlled memory:

/* ClfsValidateBlock — vulnerable build */
NTSTATUS ClfsValidateBlock(
    PCLFS_LOG_BLOCK_HEADER pHdr,
    ULONGLONG              cbBuffer,
    UCHAR                  chOwnTag,
    ULONG                  fFlags,
    PULONG                 pcValidated)
{
    *pcValidated = 0;

    if (fFlags > 0x10 || (fFlags & ~0x10111) != 0)
        return STATUS_INVALID_PARAMETER;

    /* pHdr->BlockLsn (+0x18) must match the caller's expected LSN */

    if (pHdr->ValidSectorCount < pHdr->TotalSectorCount)   /* +0x06 sibling >= +0x04 */
        return STATUS_LOG_CORRUPT;

    /* sector-0 stamp via ClfsValidateSector, then record-/restart-area geometry
       at pHdr->RestartAreaOffset (+0x28), SymbolOffset (+0x2C), SymbolSize (+0x30) */

    if (cbBuffer < (ULONGLONG)(pHdr->TotalSectorCount << 9))   /* +0x04 bound — fresh read */
        return STATUS_LOG_CORRUPT;

    *pcValidated = 1;
    ULONG idx = 1;
    USHORT cSectorNow;
    if (pHdr->TotalSectorCount > 2) {                  /* +0x04 only walk multi-sector blocks */
        do {
            PUCHAR pbStamp = (PUCHAR)pHdr + (idx << 9) + 0x1FE;   /* sector tail */
            if ((CHAR)pbStamp[0] < 0)                              /* signature high bit = BAAD */
                return STATUS_LOG_CORRUPT;
            if (pbStamp[1] != chOwnTag)                            /* own-tag mismatch */
                return STATUS_LOG_CORRUPT;
            /* ... signature flag-bit checks against fFlags ... */

            idx += 1;
            *pcValidated = idx;
            cSectorNow = pHdr->TotalSectorCount;        /* +0x04 RE-READ every iteration */
        } while (idx < (ULONG)(cSectorNow - 1));        /* exit bound is the re-read */
    }
    /* last-sector stamp at pHdr + (TotalSectorCount-1)*512 + 0x1FE, then ClfsValidateSector */
    return STATUS_SUCCESS;
}

Two reads of +0x04 are the whole bug. In a single-threaded world the bound check would be sufficient — the field does not change, so the re-read returns the same value. But the compiler emits a fresh load of TotalSectorCount on every iteration because it cannot prove that the *pcValidated = idx write does not alias pHdr->TotalSectorCount, and from the kernel’s perspective each iteration is a fresh trust decision: the validator re-asks the buffer “how many sectors did you say this block had?” and acts on the new answer without re-checking it against cbBuffer. If the pool that backs pHdr changes underneath the validator — another thread sharing the block, a re-read of the same buffer from a concurrent flush, any path that lets the count grow between the bound check and the loop — the read at pHdr + idx*512 + 0x1FE walks past the byte length the caller handed in, and the next stamp fetch is off the block buffer into whatever pool lives next to it.

The same shape hands the attacker the accept path. The exit condition idx >= cSectorNow - 1 is satisfied by the first iteration that sees a TotalSectorCount small enough to make it true, regardless of what the bound check measured at entry. An under-inflated TotalSectorCount lets the loop exit early and the function return success having verified only the prefix of the block — the tail sectors, where the attacker stages forged record offsets and container counts for the downstream walks, sail through unverified. That accept path is the worse outcome: once ClfsValidateBlock returns success on a block whose tail is fabricated, the same table walks that earlier CLFS validator bugs have turned into pool-corruption primitives run on attacker-chosen record offsets and container counts, and the next append or flush turns into a pool write at a partially-attacker-controlled offset.

The patch

The fix is one line of caching: read TotalSectorCount once at the top of the valid region into a local, and drive both the bound check and the loop exit off that single local. The loop body no longer reaches into the header for its bound, so a buffer that changes underneath it cannot inflate or shorten the walk. The first-sector stamp check is also moved into a ClfsValidateSector helper, but that is refactoring — the security-relevant change is the local in place of the per-iteration load:

/* ClfsValidateBlock — patched */
NTSTATUS ClfsValidateBlock(
    PCLFS_LOG_BLOCK_HEADER pHdr,
    ULONGLONG              cbBuffer,
    UCHAR                  chOwnTag,
    ULONG                  fFlags,
    PULONG                 pcValidated)
{
    *pcValidated = 0;

    if (fFlags > 0x10 || (fFlags & ~0x10111) != 0)
        return STATUS_INVALID_PARAMETER;

    /* pHdr->BlockLsn (+0x18) must match the caller's expected LSN */

    USHORT cSector = pHdr->TotalSectorCount;                   /* +0x04 read ONCE, cached */

    if (pHdr->ValidSectorCount < cSector)                      /* +0x06 sibling >= cSector */
        return STATUS_LOG_CORRUPT;

    /* ClfsValidateSector(pHdr, chOwnTag, fFlags | 0x40) — sector-0 stamp,
       then record-/restart-area geometry at +0x28 / +0x2C / +0x30 */

    if (cbBuffer < (ULONGLONG)(cSector << 9))                  /* bound uses cached value */
        return STATUS_LOG_CORRUPT;

    *pcValidated = 1;
    ULONG idx = 1;
    ULONG cExit = (ULONG)(cSector - 1);                        /* exit bound frozen once */
    if (cExit > 1) {
        do {
            PUCHAR pbStamp = (PUCHAR)pHdr + (idx << 9) + 0x1FE;
            if ((CHAR)pbStamp[0] < 0)
                return STATUS_LOG_CORRUPT;
            if (pbStamp[1] != chOwnTag)
                return STATUS_LOG_CORRUPT;
            /* ... signature flag-bit checks against fFlags ... */

            idx += 1;
            *pcValidated = idx;
        } while (idx < cExit);                                 /* exit uses cached value */
    }
    /* last-sector stamp at pHdr + (cSector-1)*512 + 0x1FE, then ClfsValidateSector */
    return STATUS_SUCCESS;
}

The principle is “if you check an attacker-controlled value, do not then re-read it.” A loop bound that is re-fetched from the buffer it is supposed to be policing is not a bound; it is a suggestion the attacker is allowed to revise mid-walk. The fix is unglamorous — a local variable in place of a memory operand — but it converts “the kernel re-asks the attacker every iteration” back into “the kernel decided once, at the boundary.”

Attack path

flowchart TD
    A["attacker crafts .blf whose CLFS_LOG_BLOCK_HEADER TotalSectorCount can change while the block is being validated"]
    B["kernel reads the block into a pool buffer and calls ClfsValidateBlock"]
    C["bound check reads TotalSectorCount from the buffer once and compares cbBuffer against TotalSectorCount times 512"]
    D["loop body re-reads TotalSectorCount from the buffer every iteration to compute its exit"]
    E["a change to TotalSectorCount between the bound check and the loop walks past the validated bound, or exits early and accepts a forged tail"]
    A --> B --> C --> D --> E

Reachability is the same as every other CLFS validator bug: opening a .blf and having CLFS read its block back is what every log open, append, and flush does, and there is no privilege requirement to create or open a CLFS log. The only gate that matters is whether the validator can be fooled into trusting a value it should have frozen, and in the vulnerable build the bound on the inner loop was a value the buffer was allowed to change.

A count that lives inside an attacker-controlled buffer must size a walk over that buffer exactly once. Two reads of the same field — one for the check, one for the loop — are not a check at all; they are two independent trust decisions about a value the attacker chose, and the second one is the one that actually moves the pointer. That applies to every field in CLFS_LOG_BLOCK_HEADER: sector count, sector-size hint, record offsets, LSN are all values the attacker chose, and the validator is the only thing standing between “the disk says so” and “the kernel trusts it.”