From 62e99128acc8a209d3ba68dcc07cf83489b7a136 Mon Sep 17 00:00:00 2001 From: jcalabro Date: Fri, 3 Apr 2026 18:19:54 -0400 Subject: [PATCH] fix 6 issues from code review: overflow, empty CID, varint, CAR roots Fixes from thorough code review: - skipValue: arg.val * 2 overflow on crafted map headers (use std.math.mul) - peekTypeAt: @intCast panic on huge map count (use std.math.cast) - readUvarint: 10th byte silently truncated (reject byte > 1 at shift 63) - readUvarint: use u7 shift to avoid saturation arithmetic - Reject empty CIDs (just 0x00 prefix, no version/codec bytes) in both the high-level decoder and the low-level readCidLink - CAR reader: reject non-CID values in roots array (was silently skipping) - MST loadFromBlocks: remove unnecessary 512-byte buffer copy for root key Add 4 tests: varint 10th byte overflow/acceptance, empty CID rejection, too-short CID rejection. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/internal/repo/car.zig | 2 +- src/internal/repo/cbor.zig | 26 ++++++++++++-------- src/internal/repo/cbor_test.zig | 43 +++++++++++++++++++++++++++++++++ src/internal/repo/mst.zig | 8 +++--- 4 files changed, 63 insertions(+), 16 deletions(-) diff --git a/src/internal/repo/car.zig b/src/internal/repo/car.zig index c387f56..e7f1d2b 100644 --- a/src/internal/repo/car.zig +++ b/src/internal/repo/car.zig @@ -87,7 +87,7 @@ pub fn readWithOptions(allocator: Allocator, data: []const u8, options: ReadOpti for (root_values) |root_val| { switch (root_val) { .cid => |c| try roots.append(allocator, c), - else => {}, + else => return error.InvalidHeader, // roots must all be CID links } } if (roots.items.len == 0) return error.InvalidHeader; diff --git a/src/internal/repo/cbor.zig b/src/internal/repo/cbor.zig index 67f1044..a8d3f8c 100644 --- a/src/internal/repo/cbor.zig +++ b/src/internal/repo/cbor.zig @@ -366,7 +366,9 @@ fn decodeAt(allocator: Allocator, data: []const u8, pos: *usize, depth: usize) D .bytes => |b| b, else => return error.InvalidCid, }; - if (cid_bytes.len < 1 or cid_bytes[0] != 0x00) return error.InvalidCid; + // CID byte string must have 0x00 identity multibase prefix + at least + // version byte + codec byte (minimum 3 bytes total) + if (cid_bytes.len < 3 or cid_bytes[0] != 0x00) return error.InvalidCid; break :blk .{ .cid = .{ .raw = cid_bytes[1..] } }; // zero-cost: just reference the bytes }, .simple => unreachable, // handled above @@ -379,17 +381,20 @@ pub fn parseCid(raw: []const u8) Cid { return .{ .raw = raw }; } -/// read an unsigned varint (LEB128). rejects varints longer than 10 bytes. +/// read an unsigned varint (LEB128). rejects varints longer than 10 bytes +/// and rejects overflow (10th byte must have value <= 1). pub fn readUvarint(data: []const u8, pos: *usize) ?u64 { var result: u64 = 0; - var shift: u6 = 0; - for (0..10) |_| { + var shift: u7 = 0; + for (0..10) |i| { if (pos.* >= data.len) return null; const byte = data[pos.*]; pos.* += 1; - result |= @as(u64, byte & 0x7f) << shift; + // 10th byte (i=9, shift=63): only bit 0 can fit in u64 + if (i == 9 and byte > 1) return null; + result |= @as(u64, byte & 0x7f) << @as(u6, @intCast(shift)); if (byte & 0x80 == 0) return result; - shift +|= 7; + shift += 7; } return null; // varint too long } @@ -711,8 +716,8 @@ pub fn readCidLink(data: []const u8, pos: usize) DecodeError!SliceResult { // Read the inner byte string const bytes_result = try readBytes(data, tag_arg.end); const payload = bytes_result.val; - // Must have at least the 0x00 prefix - if (payload.len == 0 or payload[0] != 0x00) return error.InvalidCid; + // Must have 0x00 prefix + at least version byte + codec byte (min 3 bytes) + if (payload.len < 3 or payload[0] != 0x00) return error.InvalidCid; return .{ .val = payload[1..], .end = bytes_result.end }; } @@ -756,7 +761,7 @@ pub fn skipValue(data: []const u8, pos: usize) DecodeError!usize { // map: push key+value count (2 per entry) if (arg.val > 0) { if (depth >= max_stack) return error.MaxDepthExceeded; - stack[depth] = arg.val * 2; + stack[depth] = std.math.mul(u64, arg.val, 2) catch return error.Overflow; depth += 1; continue; } @@ -796,7 +801,8 @@ pub fn peekTypeAt(data: []const u8, pos: usize) DecodeError!?[]const u8 { var cur = map_header.end; const count = map_header.val; - for (0..@as(usize, @intCast(count))) |_| { + const safe_count = std.math.cast(usize, count) orelse return null; + for (0..safe_count) |_| { // Read key — DAG-CBOR keys are always text strings const key = readText(data, cur) catch return null; cur = key.end; diff --git a/src/internal/repo/cbor_test.zig b/src/internal/repo/cbor_test.zig index 45ee6b3..6b048ee 100644 --- a/src/internal/repo/cbor_test.zig +++ b/src/internal/repo/cbor_test.zig @@ -1168,6 +1168,49 @@ test "get returns null for non-map value" { // === negative integer encode round-trip at min i64 === +// === readUvarint 10th byte overflow rejection === + +test "readUvarint rejects 10th byte with value > 1" { + // 9 continuation bytes (0x80) + 10th byte with value 2 (bit 1 set, would overflow u64) + const data = [_]u8{0x80} ** 9 ++ [_]u8{0x02}; + var pos: usize = 0; + try std.testing.expect(cbor.readUvarint(&data, &pos) == null); +} + +test "readUvarint accepts 10th byte with value 1 (max u64)" { + // 9 continuation bytes (0xff = 0x7f data + continuation) + 10th byte 0x01 + // This encodes 2^63 + (lower 63 bits all set) = max u64 + const data = [_]u8{0xff} ** 9 ++ [_]u8{0x01}; + var pos: usize = 0; + const val = cbor.readUvarint(&data, &pos); + try std.testing.expect(val != null); + try std.testing.expectEqual(std.math.maxInt(u64), val.?); +} + +// === CID minimum size === + +test "reject tag 42 with only 0x00 prefix (empty CID)" { + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + // tag(42) + bytes([0x00]) — prefix present but no actual CID bytes + try std.testing.expectError(error.InvalidCid, cbor.decode(arena.allocator(), &.{ + 0xd8, 0x2a, // tag(42) + 0x41, 0x00, // bytes(1) with just the 0x00 prefix + })); +} + +test "reject tag 42 with only prefix + 1 byte (too short for CID)" { + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + // tag(42) + bytes([0x00, 0x01]) — only 1 CID byte, need at least version + codec + try std.testing.expectError(error.InvalidCid, cbor.decode(arena.allocator(), &.{ + 0xd8, 0x2a, // tag(42) + 0x42, 0x00, 0x01, // bytes(2) — prefix + 1 byte + })); +} + +// === negative integer encode round-trip at min i64 === + test "round-trip encode min i64" { var arena = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena.deinit(); diff --git a/src/internal/repo/mst.zig b/src/internal/repo/mst.zig index 6e9128c..617fc4c 100644 --- a/src/internal/repo/mst.zig +++ b/src/internal/repo/mst.zig @@ -469,11 +469,9 @@ pub const Mst = struct { const root_node = try loadNodeFromData(allocator, repo_car, root_node_data); - // root layer = key height of first entry - var key_buf: [512]u8 = undefined; - const first = root_node_data.entries[0]; - @memcpy(key_buf[0..first.key_suffix.len], first.key_suffix); - const root_layer = keyHeight(key_buf[0..first.key_suffix.len]); + // root layer = key height of first entry (root entry has prefix_len=0, + // so key_suffix IS the full key — no need to copy) + const root_layer = keyHeight(root_node_data.entries[0].key_suffix); return .{ .allocator = allocator, -- 2.51.2