Grandiras/ATProto.NET
1
1
Fork 0
Code Issues Pull requests Projects Releases 9 Packages 8 Wiki Activity Actions

fix(repo): spec-correct MST and hardened DAG-CBOR / CAR parsing #111

New issue
Closed
opened 2026-09-24 12:28:55 +00:00 by Grandiras · 0 comments
Grandiras commented 2026-09-24 12:28:55 +00:00
Owner
Copy link

Part of the v0.7.0 update cycle. The MST does not follow the atproto spec, and the DAG-CBOR/CAR readers can be crashed by untrusted input. Both run on relay data.

Correctness: MST

Today every MST root CID differs from the network's (0/5 reference vectors match), so repos written by the SDK (#40: MerkleSearchTree + RepoCommit + CarWriter) are rejected by other implementations.

  • Encode the nullable l and t as explicit CBOR null, not omitted. The empty tree must hash to bafyreie5737gdxlw5i64vzichcalba3z2v5n6icifvx5xytvske7mr3hpm.
  • Decode null l/t. MerkleSearchTree.Deserialize currently throws on every real getRepo node.
  • Bulk build: an empty layer-0 range must yield null, not an empty node.
  • Incremental Add/Delete currently disagree with the bulk build in 99 of 100 random trials. Rebuild from the sorted entry set, or fix the mutation code, so that any sequence of edits yields the bulk-build root.
  • Pin the behaviour to the upstream reference vectors (bluesky-social/atproto packages/repo/tests/mst.test.ts, plus atproto-interop-tests MST fixtures where they exist), and add a randomized add/delete ≡ bulk-build property test.

Security: untrusted input

  • MST PrefixLen: validate 0 ≤ p ≤ previousKey.Length while decoding and throw FormatException (same bug class as indigo's SEC-4 fix, 2026-09-02). Today a crafted node allocates new byte[p + …] with p ≈ 2^31 before any check.
  • CarReader stack overflow: its hand-written recursive CBOR skipper for the CAR header crashes the process on a ~100 KB header of nested arrays (reproduced). Replace it with System.Formats.Cbor.CborReader.
  • Recursion depth: add a depth limit to the DAG-CBOR→JSON walkers (DagCborDecoder, FirehoseEventParser's transcoder), which overflow at the same depth.
  • CarReader must reject CIDv0 blocks, which currently bypass its unknown-codec rule.

Consolidation (while touching these files)

  • One internal null-aware DagCborLink read/write helper replacing the 4 write and 4 read copies (CarWriter, MstNodeData, RepoCommit, DagCborEncoder / DagCborDecoder, FirehoseEventParser, CarReader).
  • One one-pass DAG-CBOR→JSON transcoder (with depth limit) shared by DagCborDecoder.Decode and FirehoseEventParser, replacing the JsonNode tree plus re-serialize path (4.3→3.5 µs, 11.4→7.9 KB per record).
  • DagCborEncoder: stop sorting keys twice (own sort plus CborConformanceMode.Canonical); assert byte-identical output. SpaceRepoCar.Serialize writes the index map directly with CborWriter instead of JsonObject→JsonElement→CBOR.
  • Remove the duplicates and dead code: ComputeNodeCid vs SerializeNode, Create(entries) vs CreateFromEntries, ValidateNode (make it rebuild-and-compare), DagCborDecoder.DecodeToNode, DagCborEncoder.ComputeCid, unused locals, and CarBlock.DataLength.
  • Make MstNodeData, MstTreeEntry and MstKeyDepth internal. Keep the documented CidComputation.TryDecodeCidString and Tid.FromInt64/ToInt64.
  • Add varint tests: overflow, truncation, and values of 3+ bytes.

Acceptance

  • MST reference vectors pass.
  • Crafted-input tests (deep nesting, huge p, negative p, CIDv0) fail with FormatException, never crash.
  • The full suite passes.
  • CHANGELOG: Fixed for the MST, Security for the parsers, Breaking changes for anything made internal or removed.
Part of the v0.7.0 update cycle. The MST does not follow the atproto spec, and the DAG-CBOR/CAR readers can be crashed by untrusted input. Both run on relay data. ## Correctness: MST Today every MST root CID differs from the network's (0/5 reference vectors match), so repos written by the SDK (#40: `MerkleSearchTree` + `RepoCommit` + `CarWriter`) are rejected by other implementations. - [ ] Encode the nullable `l` and `t` as explicit CBOR `null`, not omitted. The empty tree must hash to `bafyreie5737gdxlw5i64vzichcalba3z2v5n6icifvx5xytvske7mr3hpm`. - [ ] Decode `null` `l`/`t`. `MerkleSearchTree.Deserialize` currently throws on every real `getRepo` node. - [ ] Bulk build: an empty layer-0 range must yield `null`, not an empty node. - [ ] Incremental `Add`/`Delete` currently disagree with the bulk build in 99 of 100 random trials. Rebuild from the sorted entry set, or fix the mutation code, so that any sequence of edits yields the bulk-build root. - [ ] Pin the behaviour to the upstream reference vectors (`bluesky-social/atproto` `packages/repo/tests/mst.test.ts`, plus `atproto-interop-tests` MST fixtures where they exist), and add a randomized add/delete ≡ bulk-build property test. ## Security: untrusted input - [ ] **MST `PrefixLen`:** validate `0 ≤ p ≤ previousKey.Length` while decoding and throw `FormatException` (same bug class as indigo's SEC-4 fix, 2026-09-02). Today a crafted node allocates `new byte[p + …]` with `p ≈ 2^31` before any check. - [ ] **`CarReader` stack overflow:** its hand-written recursive CBOR skipper for the CAR header crashes the process on a ~100 KB header of nested arrays (reproduced). Replace it with `System.Formats.Cbor.CborReader`. - [ ] **Recursion depth:** add a depth limit to the DAG-CBOR→JSON walkers (`DagCborDecoder`, `FirehoseEventParser`'s transcoder), which overflow at the same depth. - [ ] `CarReader` must reject CIDv0 blocks, which currently bypass its unknown-codec rule. ## Consolidation (while touching these files) - [ ] One internal null-aware `DagCborLink` read/write helper replacing the 4 write and 4 read copies (`CarWriter`, `MstNodeData`, `RepoCommit`, `DagCborEncoder` / `DagCborDecoder`, `FirehoseEventParser`, `CarReader`). - [ ] One one-pass DAG-CBOR→JSON transcoder (with depth limit) shared by `DagCborDecoder.Decode` and `FirehoseEventParser`, replacing the `JsonNode` tree plus re-serialize path (4.3→3.5 µs, 11.4→7.9 KB per record). - [ ] `DagCborEncoder`: stop sorting keys twice (own sort plus `CborConformanceMode.Canonical`); assert byte-identical output. `SpaceRepoCar.Serialize` writes the index map directly with `CborWriter` instead of `JsonObject`→`JsonElement`→CBOR. - [ ] Remove the duplicates and dead code: `ComputeNodeCid` vs `SerializeNode`, `Create(entries)` vs `CreateFromEntries`, `ValidateNode` (make it rebuild-and-compare), `DagCborDecoder.DecodeToNode`, `DagCborEncoder.ComputeCid`, unused locals, and `CarBlock.DataLength`. - [ ] Make `MstNodeData`, `MstTreeEntry` and `MstKeyDepth` `internal`. Keep the documented `CidComputation.TryDecodeCidString` and `Tid.FromInt64`/`ToInt64`. - [ ] Add varint tests: overflow, truncation, and values of 3+ bytes. ## Acceptance - MST reference vectors pass. - Crafted-input tests (deep nesting, huge `p`, negative `p`, CIDv0) fail with `FormatException`, never crash. - The full suite passes. - CHANGELOG: `Fixed` for the MST, `Security` for the parsers, `Breaking changes` for anything made internal or removed.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
v0.8.0
v0.7.0
v0.6.0
v0.5.0
v0.4.0
v0.3.0
v0.2.0
v0.1.1
v0.1.0
Labels
Clear labels   
breaking-change
Introduces a breaking API change

  
bug
Something is not working correctly

  
documentation
Documentation improvements

  
duplicate
This issue or PR already exists

  
enhancement
New feature or improvement

  
good first issue
Good for newcomers to the project

  
help wanted
Extra attention is needed

  
performance
Performance-related issue or improvement

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Grandiras/ATProto.NET#111
No description provided.