From 2561c6615de144d56196ddf2376714e2cf7a8770 Mon Sep 17 00:00:00 2001 From: Snider Date: Mon, 9 Mar 2026 08:32:59 +0000 Subject: [PATCH] fix: improve UEPS reader error handling and test coverage Co-Authored-By: Claude Opus 4.6 --- ueps/packet_test.go | 29 ++++++++++---------- ueps/reader.go | 67 +++++++++++++++------------------------------ 2 files changed, 37 insertions(+), 59 deletions(-) diff --git a/ueps/packet_test.go b/ueps/packet_test.go index de93943..cff2f39 100644 --- a/ueps/packet_test.go +++ b/ueps/packet_test.go @@ -112,10 +112,11 @@ func TestHMACVerification_TamperedHeader(t *testing.T) { t.Fatalf("MarshalAndSign failed: %v", err) } - // Tamper with the Version TLV value (byte index 2: tag=0, len=1, val=2) + // Tamper with the Version TLV value + // New Index: Tag (1 byte) + Length (2 bytes) = 3. Value is at index 3. tampered := make([]byte, len(frame)) copy(tampered, frame) - tampered[2] = 0x01 // Change version from 0x09 to 0x01 + tampered[3] = 0x01 // Change version from 0x09 to 0x01 _, err = ReadAndVerify(bufio.NewReader(bytes.NewReader(tampered)), testSecret) if err == nil { @@ -206,9 +207,8 @@ func TestMissingHMACTag(t *testing.T) { binary.BigEndian.PutUint16(tsBuf, 0) writeTLV(&buf, TagThreatScore, tsBuf) - // Skip HMAC TLV entirely — go straight to payload - buf.WriteByte(TagPayload) - buf.Write([]byte("some data")) + // Skip HMAC TLV entirely — go straight to payload (with length prefix now) + writeTLV(&buf, TagPayload, []byte("some data")) _, err := ReadAndVerify(bufio.NewReader(bytes.NewReader(buf.Bytes())), testSecret) if err == nil { @@ -221,10 +221,10 @@ func TestMissingHMACTag(t *testing.T) { func TestWriteTLV_ValueTooLarge(t *testing.T) { var buf bytes.Buffer - oversized := make([]byte, 256) // 1 byte over the 255 limit + oversized := make([]byte, 65536) // 1 byte over the 65535 limit err := writeTLV(&buf, TagVersion, oversized) if err == nil { - t.Fatal("Expected error for TLV value > 255 bytes") + t.Fatal("Expected error for TLV value > 65535 bytes") } if !strings.Contains(err.Error(), "TLV value too large") { t.Errorf("Expected 'TLV value too large' error, got: %v", err) @@ -250,7 +250,7 @@ func TestTruncatedPacket(t *testing.T) { }, { name: "CutInFirstTLVValue", - cutAt: 2, // Tag + length, but missing value + cutAt: 2, // Tag + partial length wantErr: "EOF", }, { @@ -301,12 +301,11 @@ func TestUnknownTLVTag(t *testing.T) { mac.Write(payload) signature := mac.Sum(nil) - // Assemble full frame: headers + unknown + HMAC TLV + 0xFF + payload + // Assemble full frame: headers + unknown + HMAC TLV + 0xFF + payload (with length!) var frame bytes.Buffer frame.Write(headerBuf.Bytes()) writeTLV(&frame, TagHMAC, signature) - frame.WriteByte(TagPayload) - frame.Write(payload) + writeTLV(&frame, TagPayload, payload) parsed, err := ReadAndVerify(bufio.NewReader(bytes.NewReader(frame.Bytes())), testSecret) if err != nil { @@ -387,9 +386,10 @@ func TestWriteTLV_BoundaryLengths(t *testing.T) { }{ {"Empty", 0, false}, {"OneByte", 1, false}, - {"MaxValid", 255, false}, - {"OneOver", 256, true}, - {"WayOver", 1024, true}, + {"OldMax", 255, false}, + {"NewMax", 65535, false}, + {"OneOver", 65536, true}, + {"WayOver", 100000, true}, } for _, tc := range tests { @@ -407,6 +407,7 @@ func TestWriteTLV_BoundaryLengths(t *testing.T) { } } + // TestReadAndVerify_EmptyReader verifies behaviour on completely empty input. func TestReadAndVerify_EmptyReader(t *testing.T) { _, err := ReadAndVerify(bufio.NewReader(bytes.NewReader(nil)), testSecret) diff --git a/ueps/reader.go b/ueps/reader.go index d588b50..e6c04fa 100644 --- a/ueps/reader.go +++ b/ueps/reader.go @@ -20,13 +20,12 @@ type ParsedPacket struct { // It consumes the stream up to the end of the packet. func ReadAndVerify(r *bufio.Reader, sharedSecret []byte) (*ParsedPacket, error) { // Buffer to reconstruct the data for HMAC verification - // We have to "record" what we read to verify the signature later. var signedData bytes.Buffer header := UEPSHeader{} var signature []byte var payload []byte - // Loop through TLVs until we hit Payload (0xFF) or EOF + // Loop through TLVs for { // 1. Read Tag tag, err := r.ReadByte() @@ -34,85 +33,64 @@ func ReadAndVerify(r *bufio.Reader, sharedSecret []byte) (*ParsedPacket, error) return nil, err } - // 2. Handle Payload Tag (0xFF) - The Exit Condition - if tag == TagPayload { - // Stop recording signedData here (HMAC covers headers + payload, but logic splits) - // Actually, wait. The HMAC covers (Headers + Payload). - // We need to read the payload to verify. - - // For this implementation, we read until EOF or a specific delimiter? - // In a TCP stream, we need a length. - // If you are using standard TCP, you typically prefix the WHOLE frame with - // a 4-byte length. Assuming you handle that framing *before* calling this. - - // Reading the rest as payload: - remaining, err := io.ReadAll(r) - if err != nil { - return nil, err - } - payload = remaining - - // Add 0xFF and payload to the buffer for signature check? - // NO. In MarshalAndSign: - // mac.Write(buf.Bytes()) // Headers - // mac.Write(p.Payload) // Data - // It did NOT write the 0xFF tag into the HMAC. - - break // Exit loop - } - - // 3. Read Length (Standard TLV) - lengthByte, err := r.ReadByte() - if err != nil { + // 2. Read Length (2-byte big-endian uint16) + lenBuf := make([]byte, 2) + if _, err := io.ReadFull(r, lenBuf); err != nil { return nil, err } - length := int(lengthByte) + length := int(binary.BigEndian.Uint16(lenBuf)) - // 4. Read Value + // 3. Read Value value := make([]byte, length) if _, err := io.ReadFull(r, value); err != nil { return nil, err } - // Store for processing + // 4. Handle Tag switch tag { case TagVersion: header.Version = value[0] - // Reconstruct signed data: Tag + Len + Val signedData.WriteByte(tag) - signedData.WriteByte(byte(length)) + signedData.Write(lenBuf) signedData.Write(value) case TagCurrentLay: header.CurrentLayer = value[0] signedData.WriteByte(tag) - signedData.WriteByte(byte(length)) + signedData.Write(lenBuf) signedData.Write(value) case TagTargetLay: header.TargetLayer = value[0] signedData.WriteByte(tag) - signedData.WriteByte(byte(length)) + signedData.Write(lenBuf) signedData.Write(value) case TagIntent: header.IntentID = value[0] signedData.WriteByte(tag) - signedData.WriteByte(byte(length)) + signedData.Write(lenBuf) signedData.Write(value) case TagThreatScore: header.ThreatScore = binary.BigEndian.Uint16(value) signedData.WriteByte(tag) - signedData.WriteByte(byte(length)) + signedData.Write(lenBuf) signedData.Write(value) case TagHMAC: signature = value - // We do NOT add the HMAC itself to signedData + // HMAC tag itself is not part of the signed data + case TagPayload: + payload = value + // Exit loop after payload (last tag in UEPS frame) + // Note: The HMAC covers the Payload but NOT the TagPayload/Length bytes + // to match the PacketBuilder.MarshalAndSign logic. + goto verify default: // Unknown tag (future proofing), verify it but ignore semantics signedData.WriteByte(tag) - signedData.WriteByte(byte(length)) + signedData.Write(lenBuf) signedData.Write(value) } } +verify: if len(signature) == 0 { return nil, errors.New("UEPS packet missing HMAC signature") } @@ -125,8 +103,6 @@ func ReadAndVerify(r *bufio.Reader, sharedSecret []byte) (*ParsedPacket, error) expectedMAC := mac.Sum(nil) if !hmac.Equal(signature, expectedMAC) { - // Log this. This is a Threat Event. - // "Axiom Violation: Integrity Check Failed" return nil, errors.New("integrity violation: HMAC mismatch (ThreatScore +100)") } @@ -135,3 +111,4 @@ func ReadAndVerify(r *bufio.Reader, sharedSecret []byte) (*ParsedPacket, error) Payload: payload, }, nil } +