diff --git a/cobhan.go b/cobhan.go index 415cf49..8446656 100644 --- a/cobhan.go +++ b/cobhan.go @@ -103,12 +103,41 @@ func bufferPtrToDataPtr(bufferPtr unsafe.Pointer) unsafe.Pointer { } func bufferPtrToString(bufferPtr unsafe.Pointer, length C.int) string { + if length == 0 { + // bufferPtrToDataPtr(bufferPtr) would point exactly one byte past + // the end of a header-only allocation; forming that pointer via + // raw uintptr arithmetic (rather than Go's slice-indexing, which + // is allowed to compute one-past-the-end addresses) is invalid + // under checkptr, even though it's never dereferenced. + return "" + } dataPtr := bufferPtrToDataPtr(bufferPtr) //Allocation return C.GoStringN((*C.char)(dataPtr), length) } +// bufferPtrToBytes trusts length completely -- it comes from the buffer's +// own header, which Cobhan's own write path (BytesToBuffer) always sets to +// exactly the number of bytes actually written. There is no independent +// capacity available here to validate that against: this function only +// ever sees a raw pointer, and callers may be foreign (non-Go-allocated) +// memory that Go's runtime has no visibility into at all. A header that +// overstates the real buffer size (from a corrupted buffer, or a bug in +// another language's Cobhan binding) will read past the end of the real +// allocation with no error and no crash in a normal build -- this is an +// inherent limitation of a length-prefixed wire format that trusts its own +// length field, not something checkable from a bare pointer + length pair. +// A fix would require changing this function's signature (and therefore +// the C ABI every Cobhan binding depends on) to also carry a capacity. func bufferPtrToBytes(bufferPtr unsafe.Pointer, length C.int) ([]byte, int32) { + if length == 0 { + // See bufferPtrToString. + if copyBuffers { + return []byte{}, ERR_NONE + } + return nil, ERR_NONE + } + src := unsafe.Slice((*byte)(bufferPtrToDataPtr(bufferPtr)), length) if copyBuffers { @@ -130,6 +159,11 @@ func tempToBytes(ptr unsafe.Pointer, length C.int) ([]byte, int32) { } length = 0 - length + if length < 0 { + // Only math.MinInt32 negates to itself (two's complement overflow). + // There's no valid filename-length encoding for it. + return nil, ERR_READ_TEMP_FILE_FAILED + } if bufferMaximum < int(length) { return nil, ERR_BUFFER_TOO_LARGE @@ -323,7 +357,13 @@ func BufferToJson(srcPtr unsafe.Pointer) (map[string]interface{}, int32) { if err != nil { return nil, ERR_JSON_DECODE_FAILED } - return loadedJson.(map[string]interface{}), ERR_NONE + jsonMap, ok := loadedJson.(map[string]interface{}) + if !ok { + // Valid JSON, but the top-level value isn't an object (e.g. an + // array, string, number, bool, or null). + return nil, ERR_JSON_DECODE_FAILED + } + return jsonMap, ERR_NONE } func BufferToJsonStruct(srcPtr unsafe.Pointer, dst interface{}) int32 { @@ -399,8 +439,16 @@ func BytesToBuffer(bytes []byte, dstPtr unsafe.Pointer) int32 { dstCapInt := int(dstCap) bytesLen := len(bytes) - // Construct a byte slice out of the unsafe pointers - var dst []byte = unsafe.Slice((*byte)(bufferPtrToDataPtr(dstPtr)), dstCapInt) + // Construct a byte slice out of the unsafe pointers. A zero-capacity + // destination is header-only; bufferPtrToDataPtr(dstPtr) would point + // exactly one byte past the end of that allocation, which checkptr + // flags as invalid even though it's never dereferenced -- avoid + // forming it at all and just use a nil slice (copy(nil, ...) is a + // valid no-op). + var dst []byte + if dstCapInt > 0 { + dst = unsafe.Slice((*byte)(bufferPtrToDataPtr(dstPtr)), dstCapInt) + } var result int if dstCapInt < bytesLen { // Output will not fit in supplied buffer diff --git a/cobhan_test.go b/cobhan_test.go index e10ed36..5164f35 100644 --- a/cobhan_test.go +++ b/cobhan_test.go @@ -4,9 +4,11 @@ import ( "bytes" "encoding/binary" "errors" + "math" "os" "strings" "testing" + "unsafe" ) func testAllocateStringBuffer(t *testing.T, str string) []byte { @@ -58,7 +60,7 @@ func TestStringRoundTripTemp(t *testing.T) { reader := bytes.NewReader(buf) binary.Read(reader, binary.LittleEndian, &fileNameLen) reader.Seek(int64(BUFFER_HEADER_SIZE), 0) - fileName := string(buf[BUFFER_HEADER_SIZE:BUFFER_HEADER_SIZE-fileNameLen]) + fileName := string(buf[BUFFER_HEADER_SIZE : BUFFER_HEADER_SIZE-fileNameLen]) output, result := BufferToStringSafe(&buf) if result != 0 { @@ -448,3 +450,139 @@ func TestDisableTempFile(t *testing.T) { t.Error("Expected ERR_BUFFER_TOO_SMALL") } } + +// --- Fuzz targets --- +// +// These target the Cobhan FFI boundary: buffers and length headers that, in +// production, arrive from a foreign caller (C, Java, Python, Node, .NET...) +// and cannot be trusted to be well-formed. Run with -race: several of the +// bugs these are designed to catch only manifest under checkptr +// instrumentation, which plain `go test` does not enable. +// +// go test -race -fuzz=FuzzStringRoundTrip -fuzztime=60s + +func FuzzStringRoundTrip(f *testing.F) { + f.Add("") + f.Add("hello") + f.Add("world of strings") + f.Fuzz(func(t *testing.T, s string) { + buf, result := AllocateStringBuffer(s) + if result != ERR_NONE { + return + } + out, result := BufferToStringSafe(&buf) + if result != ERR_NONE { + t.Fatalf("BufferToStringSafe returned %v for input %q", result, s) + } + if out != s { + t.Fatalf("roundtrip mismatch: got %q want %q", out, s) + } + }) +} + +func FuzzBytesRoundTrip(f *testing.F) { + f.Add([]byte{}) + f.Add([]byte{1, 2, 3, 4}) + f.Fuzz(func(t *testing.T, b []byte) { + buf, result := AllocateBytesBuffer(b) + if result != ERR_NONE { + return + } + out, result := BufferToBytesSafe(&buf) + if result != ERR_NONE { + t.Fatalf("BufferToBytesSafe returned %v for input %v", result, b) + } + if !bytes.Equal(out, b) { + t.Fatalf("roundtrip mismatch: got %v want %v", out, b) + } + }) +} + +func FuzzBufferToJson(f *testing.F) { + f.Add([]byte(testJson)) + f.Add([]byte("[1,2,3]")) + f.Add([]byte(`"just a string"`)) + f.Add([]byte("42")) + f.Add([]byte("null")) + f.Add([]byte("true")) + f.Fuzz(func(t *testing.T, data []byte) { + buf, result := AllocateBytesBuffer(data) + if result != ERR_NONE { + return + } + // Must never panic, regardless of whether data is valid JSON, valid + // JSON that isn't an object, or garbage -- only ERR_NONE or + // ERR_JSON_DECODE_FAILED are acceptable outcomes. + _, result = BufferToJsonSafe(&buf) + if result != ERR_NONE && result != ERR_JSON_DECODE_FAILED { + t.Fatalf("BufferToJsonSafe returned unexpected error %v for input %q", result, data) + } + }) +} + +func FuzzBufferToJsonStruct(f *testing.F) { + f.Add([]byte(testJson)) + f.Add([]byte("[1,2,3]")) + f.Add([]byte(`"just a string"`)) + f.Fuzz(func(t *testing.T, data []byte) { + buf, result := AllocateBytesBuffer(data) + if result != ERR_NONE { + return + } + dst := &MyStruct{} + // Must never panic; ERR_NONE or ERR_JSON_DECODE_FAILED are the only + // acceptable outcomes. + result = BufferToJsonStructSafe(&buf, dst) + if result != ERR_NONE && result != ERR_JSON_DECODE_FAILED { + t.Fatalf("BufferToJsonStructSafe returned unexpected error %v for input %q", result, data) + } + }) +} + +// FuzzRawBufferHeader constructs a raw Cobhan buffer by hand -- an arbitrary +// length header followed by arbitrary payload bytes -- bypassing +// AllocateBuffer entirely. This simulates a foreign caller handing Go a +// buffer with a header that doesn't match the actual data it allocated, +// which is exactly the threat model the ERR_BUFFER_TOO_LARGE/ERR_BUFFER_TOO_SMALL +// error codes exist for. +// +// A header claiming more data than the payload actually holds -- whether +// directly (a non-negative header larger than len(payload)) or via the +// temp-file-name-length encoding (a negative header whose negation is +// larger than len(payload)) -- is deliberately excluded (via t.Skip +// below), not exercised here: there is no independent capacity available +// to validate the header against in bufferPtrToBytes/bufferPtrToString, so +// a mismatch in that direction is an inherent, unfixable limitation of the +// wire format rather than something this target can usefully assert on -- +// see the comment on bufferPtrToBytes. math.MinInt32 is the one exception: +// its negation overflows back to itself, which tempToBytes explicitly +// guards against, so it's always safe regardless of payload length and +// must not be skipped (it's the regression case for that fix). +func FuzzRawBufferHeader(f *testing.F) { + f.Add(int32(0), []byte{}) // well-formed empty buffer + f.Add(int32(4), []byte{1, 2, 3, 4}) // well-formed buffer + f.Add(int32(-1), []byte{1, 2, 3, 4}) // temp-file path, garbage filename + f.Add(int32(math.MinInt32), []byte{1, 2, 3, 4}) // negation overflow in tempToBytes + f.Fuzz(func(t *testing.T, header int32, payload []byte) { + if header != math.MinInt32 { + claimedLen := int64(header) + if claimedLen < 0 { + claimedLen = -claimedLen + } + if claimedLen > int64(len(payload)) { + t.Skip("header claiming more data than the buffer actually holds is an accepted, unfixable limitation -- see bufferPtrToBytes") + } + } + + buf := make([]byte, BUFFER_HEADER_SIZE+len(payload)) + *(*int32)(unsafe.Pointer(&buf[0])) = header + copy(buf[BUFFER_HEADER_SIZE:], payload) + + ptr := unsafe.Pointer(&buf[0]) + + // Must never panic or fatally crash the process for any header + // that's within the range this buffer can actually satisfy. + BufferToBytes(ptr) + BufferToString(ptr) + }) +} diff --git a/scripts/test.sh b/scripts/test.sh index 3aec5db..c0be542 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -1,2 +1,2 @@ #!/bin/bash -go test -v -failfast -coverprofile cover.out +go test -race -v -failfast -coverprofile cover.out diff --git a/testdata/fuzz/FuzzBufferToJson/caf81e9797b19c76 b/testdata/fuzz/FuzzBufferToJson/caf81e9797b19c76 new file mode 100644 index 0000000..67322c7 --- /dev/null +++ b/testdata/fuzz/FuzzBufferToJson/caf81e9797b19c76 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("") diff --git a/testdata/fuzz/FuzzBufferToJsonStruct/caf81e9797b19c76 b/testdata/fuzz/FuzzBufferToJsonStruct/caf81e9797b19c76 new file mode 100644 index 0000000..67322c7 --- /dev/null +++ b/testdata/fuzz/FuzzBufferToJsonStruct/caf81e9797b19c76 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("")