[Security] Object-storage list() returns malformed keys not validated

[pathosDev]

Severity / Size

• Severity: MEDIUM — sweep operation trusts every key returned by backend.list() without shape-validation. A buggy backend (or adversarial response that bypasses S3's normal key constraints) could feed malformed keys into the pid-extractor, leading to wrong HKDF subkey derivation, decrypt failures, or — in a worst-case backend bug — accidental key/pid confusion that produces a wrong-but-valid subkey.
• Size: S (~1d).
• Threat model: backend bug (custom ObjectStorageBackend impl with a bug; or a real S3 with consistency edge cases returning incomplete keys); or an attacker who controls the backend (admin-level S3 access). Companion to the filesystem path-traversal fix (a290b06) but on the read side of the sweep.

Affected files

• src/persistence/object-storage/reEncryptionSweep.ts:142-220 — the sweep loop trusts items[i].key straight from backend.list().
• src/persistence/object-storage/reEncryptionSweep.ts:180-181 — pidFromKey(item.key, opts.keyPrefix) uses the key shape directly; a malformed key produces a malformed pid.
• src/persistence/object-storage/reEncryptionSweep.ts:223-240 — defaultPidFromKey extracts the pid by splitting on /. If item.key doesn't have the expected layout (e.g. starts with .., contains NUL, or has zero /-separators), the extracted pid is garbage.

Background

The sweep walks the corpus by listing keys under a prefix:

const items = await backend.list({ prefix: opts.keyPrefix });
for (const item of items) {
  const pid = pidFromKey(item.key, opts.keyPrefix);
  // derive subkey from pid; decrypt with retired key; re-encrypt with active
}

The hardening landed in a290b06 covers the write path (FS backend rejects malformed keys on put/get/delete/list). But the sweep is on the read path: it accepts whatever list() returns. If a backend's response includes:

• Keys with .. segments (path-traversal-like; should never happen on S3 but might on a custom backend).
• Keys with embedded NUL bytes.
• Keys outside the expected <keyPrefix><pid>/<rest> shape.
• Keys longer than reasonable (memory pressure).

…then defaultPidFromKey produces a garbage pid string. Downstream:

• deriveSubkey(masterKey, garbage-pid, info) produces a subkey.
• aesGcmDecrypt(subkey, iv, ciphertext) fails — wrong subkey, auth-tag mismatch.

The sweep aborts on that error (per the design: "Per-object failures … bubble up immediately and stop the sweep"). Operator sees a confusing error.

The worse case: if the backend returns a key whose pid-extractor output happens to collide with a legitimate pid (e.g. via NUL truncation), the sweep re-encrypts the legitimate pid's data with a key derived from the wrong pid → encrypted under a key that nobody else will use → data effectively lost.

Exploit walkthrough

Setup: deployment uses a custom ObjectStorageBackend (e.g. an HTTP-backed minio-like service). Backend has a bug that occasionally returns keys with trailing NUL bytes: 'users/42/snap.json\0\0\0'.

Step 1 — sweep starts: backend.list({ prefix: 'users/' }) returns a list including the NUL-poisoned key.

Step 2 — pid extraction: defaultPidFromKey('users/42/snap.json\0\0\0', 'users/') returns '42' (the slash-separator is at the right position; NUL bytes are kept).

Wait — that's actually fine for this specific case. Let me redo with a worse shape:

Step 1 — backend returns '/etc/passwd' (a S3-impl bug returning a malformed key that S3 itself wouldn't produce).

Step 2 — defaultPidFromKey('/etc/passwd', 'users/'): the key doesn't start with the prefix, so the helper's logic falls through to key.indexOf('/', 0) → returns '' (empty string before the first slash).

Step 3 — deriveSubkey(masterKey, '', info): HKDF with an empty salt. Different from the legitimate pid's subkey.

Step 4 — decrypt: aesGcmDecrypt(wrongSubkey, iv, ciphertext) → auth-tag mismatch → throws.

Step 5 — sweep aborts mid-corpus. Operator sees decrypt failure. Reruns; same failure on the same offending key.

The operator's workaround: configure opts.skip to filter the bad key. But before they figure out which key is the issue, they may run the sweep many times, slowing down rotation.

The actually-worse scenario: a custom backend returns the same legitimate key twice but the second time with a trailing space. pidFromKey('users/42/snap.json ', ...) extracts '42' correctly (trailing space is in the rest, not the pid). But: deriveSubkey salt is '42' — wait, that's the same. OK actually this case is fine.

The genuine worry is NUL-truncation behavior in the underlying crypto: WebCrypto / Bun's HKDF treat the salt as bytes, not as a C-string, so NUL-terminator confusion doesn't happen at the crypto layer. Safe.

So the realistic damage is: sweep aborts, operator confused. Not catastrophic — but the fix (validate at the read boundary) costs nothing.

How the 8 already-landed security fixes inform this

• Path-traversal guard (a290b06): closed the write-side key-traversal door. This issue is the read-side complement: don't trust keys we read from the backend either.
• Memcached CRLF block (65a2bc5): used a assertSafe... validator at the API boundary. Same pattern here: validate every key the sweep iterates over.

Fix design

Two complementary checks.

Track 1 — validate keys from list().

Reuse assertSafeKey() from FilesystemObjectStorageBackend.ts (or extract to a shared helper). In the sweep loop:

for (const item of items) {
  try {
    assertSafeKey(item.key);
  } catch (e) {
    log.warn(`reEncryptObjectStorage: skipping malformed key ${JSON.stringify(item.key)}: ${e.message}`);
    result.skippedMalformed += 1;
    opts.onProgress?.({ key: item.key, idx, total, action: 'skipped-malformed' });
    continue;
  }
  // ... existing handling
}

assertSafeKey rejects: empty strings, NUL bytes, absolute paths (POSIX + Windows), .. segments.

Track 2 — validate pid-extraction output.

After pidFromKey(...), verify the result is non-empty and shape-sane:

const pid = pidFromKey(item.key, opts.keyPrefix);
if (!pid || pid.length === 0 || pid.includes('\0') || pid.includes('/')) {
  log.warn(`reEncryptObjectStorage: skipping key ${item.key} — pid-extractor returned ${JSON.stringify(pid)}`);
  result.skippedMalformed += 1;
  continue;
}

Defends against pid-extractor edge cases not caught by assertSafeKey.

Track 3 — extract to shared helper.

assertSafeKey currently lives in FilesystemObjectStorageBackend.ts. Extract to a shared module so both the FS backend and the sweep use the same validator:

src/persistence/object-storage/KeyValidator.ts  (new file)
  export function assertSafeKey(key: string): void;
  export function assertWithinRoot(pathMod, root, fullPath): void;

Updates FilesystemObjectStorageBackend to import from there.

Track 4 — counter + result field.

ReEncryptResult.skippedMalformed: number field; obj_storage_sweep_malformed_keys_total metric.

API surface

// src/persistence/object-storage/reEncryptionSweep.ts
export interface ReEncryptResult {
  // ... existing
  readonly skippedMalformed: number;  // new
}

// src/persistence/object-storage/KeyValidator.ts (new file)
export function assertSafeKey(key: string): void;

Backward compatibility

The sweep's behaviour changes from "abort on first malformed key" to "skip + log + count, continue with the rest". This is a behaviour change: previously, a malformed key was a hard failure (forcing the operator to investigate); now it's a counted skip. Operators relying on the abort-on-malformed signal will need to inspect result.skippedMalformed after the run.

Trade-off: aborting on malformed keys gives faster feedback but blocks completion for what may be a non-fatal backend hiccup. Skip+log lets the sweep complete and surface the issue in the result. Tunable via opts.malformedKeyMode: 'skip' | 'abort' (default: skip).

Test plan

1. Exploit test: mock backend that returns a key with .. segment in its list; sweep skips it + counter increments; sweep completes for all other keys.

2. NUL-key test: key containing \0; rejected by assertSafeKey; skipped.

3. Empty-pid test: key that doesn't match the prefix; defaultPidFromKey returns empty; skipped via Track 2.

4. Mode test: with malformedKeyMode: 'abort', sweep throws on first malformed key (preserves old behaviour for users who want it).

5. Regression: existing reEncryptionSweep.test.ts tests pass.

Acceptance criteria

• KeyValidator.ts extracted as a shared module; both FS backend and sweep use it.
• Sweep validates item.key (via assertSafeKey) + pid (post-extraction shape check).
• ReEncryptResult.skippedMalformed field added; onProgress emits 'skipped-malformed'.
• opts.malformedKeyMode: 'skip' | 'abort' (default 'skip').
• obj_storage_sweep_malformed_keys_total metric exposed.
• Five new tests pass; existing sweep tests green.
• Plan-doc + README "Known security caveats" updated on land.