gmrtd ReadFile Vulnerable to Denial of Service via Excessive TLV Length Values

@@ -18,6 +18,14 @@ const INS_INTERNAL_AUTHENTICATE = byte(0x88)
 const INS_SELECT = byte(0xA4)
 const INS_READ_BINARY = byte(0xB0)
 
+// default to 65,535 maximum file (TLV) size
+// - as we always read the first 4 bytes (so max 2 byte length)
+const READ_FILE_MAX_TLV_LENGTH = tlv.TlvLength(65535)
+
+// default to 1,000 chunks when reading a file
+// - some older passports support <100 byte reads
+const READ_FILE_MAX_CHUNKS = 1000
+
 // TODO - extended length support? odd INS read-binary can support larger offset.. and potentially avoid SELECT FILE
 //
 // 3.5.2 READ BINARY
@@ -27,15 +35,19 @@ const INS_READ_BINARY = byte(0xB0)
 // TODO - review and align with 9303 p10.. 3.6 Command Formats and Parameter Options (LDS1 and LDS2)
 
 type NfcSession struct {
-	transceiver Transceiver
-	SM          *SecureMessaging
-	MaxLe       int
-	ApduLog     []ApduLog
+	transceiver          Transceiver
+	readFileMaxTlvLength tlv.TlvLength
+	readFileMaxChunks    int
+	SM                   *SecureMessaging
+	MaxLe                int
+	ApduLog              []ApduLog
 }
 
 func NewNfcSession(transceiver Transceiver) *NfcSession {
 	var nfc NfcSession
 	nfc.transceiver = transceiver
+	nfc.readFileMaxTlvLength = READ_FILE_MAX_TLV_LENGTH
+	nfc.readFileMaxChunks = READ_FILE_MAX_CHUNKS
 	nfc.MaxLe = 256
 	return &nfc
 }
@@ -219,9 +231,12 @@ func (nfc *NfcSession) ReadBinaryFromOffset(offset, length int) ([]byte, error)
 		return nil, fmt.Errorf("[ReadBinaryFromOffset] Invalid status (offset:%d,length:%d):%X", offset, length, rapdu.Status)
 	}
 
-	out := rapdu.Data
+	if len(rapdu.Data) > length {
+		// more data than requested, possible abuse
+		return nil, fmt.Errorf("[ReadBinaryFromOffset] More data than requested (act:%1d, req:%1d)", len(rapdu.Data), length)
+	}
 
-	return out, nil
+	return rapdu.Data, nil
 }
 
 // returns: file contents OR nil if file not found
@@ -258,15 +273,27 @@ func (nfc *NfcSession) ReadFile(fileId uint16) (fileData []byte, err error) {
 			return nil, fmt.Errorf("[ReadFile] ParseTagAndLength error: %w", err)
 		}
 
+		// abort if file length (TLV) exceeds configured maximum
+		if tmpTlvLength > nfc.readFileMaxTlvLength {
+			return nil, fmt.Errorf("[ReadFile] TLV length exceeds permitted maximum (len:%1d, max:%1d)", tmpTlvLength, nfc.readFileMaxTlvLength)
+		}
+
 		totalBytes = int(tmpTlvLength)
 		totalBytes += 4 - tmpBuf.Len()
 	}
 
 	// read remainder of file
 	if fileBuf.Len() < totalBytes {
+		chunkCnt := 0
 		maxReadAmount := nfc.MaxLe
 
 		for {
+			// limit the maximum number of chunks permitted when reading a file
+			if chunkCnt >= nfc.readFileMaxChunks {
+				return nil, fmt.Errorf("[ReadFile] Max chunks reached (cnt:%1d)", chunkCnt)
+			}
+			chunkCnt++
+
 			bytesToRead := min(maxReadAmount, totalBytes-fileBuf.Len())
 
 			tmpData, err := nfc.ReadBinaryFromOffset(fileBuf.Len(), bytesToRead)

@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/gmrtd/gmrtd/cryptoutils"
+	"github.com/gmrtd/gmrtd/tlv"
 	"github.com/gmrtd/gmrtd/utils"
 )
 
@@ -535,6 +536,41 @@ func TestReadBinaryFromOffsetCardDeadErr(t *testing.T) {
 	}
 }
 
+func TestReadBinaryFromOffsetTooManyBytes(t *testing.T) {
+	/*
+	* expect an error if we get more bytes than we requested
+	 */
+	var nfc *NfcSession = NewNfcSession(&StaticTransceiver{utils.HexToBytes("12345678909000")}) // 5 bytes (x1234567890)
+
+	data, err := nfc.ReadBinaryFromOffset(0, 4) // only 4 bytes requested
+	if err == nil {
+		t.Errorf("expected error (too many bytes)")
+	}
+
+	if len(data) != 0 {
+		t.Errorf("didn't expect data")
+	}
+}
+
+func TestReadFileTlvLengthTooLong(t *testing.T) {
+	/*
+	* expect an error when the TLV file length exceeds the configured maximum (i.e. 65534 > 65535)
+	 */
+	var nfc *NfcSession = NewNfcSession(&StaticTransceiver{utils.HexToBytes("6182FFFF9000")}) // len:65535
+
+	// set to smaller maximum which is less than advertised file length (65534 < 65535)
+	nfc.readFileMaxTlvLength = tlv.TlvLength(65534)
+
+	data, err := nfc.ReadFile(0x0101) // DG1
+	if err == nil {
+		t.Errorf("expected error (TLV file length exceeds maximum) %s", err)
+	}
+
+	if len(data) != 0 {
+		t.Errorf("didn't expect data")
+	}
+}
+
 // simulates an error reading the file header (to determine the file length)
 // - select file (ok)
 // - read binary from offset (0,4) --> error rApdu response (6985 - conditions of use not satisfied)
@@ -655,3 +691,51 @@ func TestReadFileErrorLastFrame(t *testing.T) {
 		t.Errorf("didn't expect data")
 	}
 }
+
+// MockTransceiverHugeLength simulates evil chip sending TLV with gigantic length
+type MockTransceiverTooManyChunks struct {
+	chunkCnt int
+}
+
+func (t *MockTransceiverTooManyChunks) Transceive(cla, ins, p1, p2 int, data []byte, le int, rapdu []byte) []byte {
+	if ins == int(INS_SELECT) {
+		// SELECT FILE - return success
+		return []byte{0x90, 0x00}
+	} else if ins == int(INS_READ_BINARY) {
+		offset := p1*256 + p2
+
+		if offset == 0 {
+			// Tag: 0x61 (DG1)
+			// Length: 0x82 0xFF FF
+			return append([]byte{
+				0x61,       // Tag
+				0x82,       // 2 byte length
+				0xFF, 0xFF, // Length = 65535
+			}, []byte{0x90, 0x00}...)
+		} else {
+			// Subsequent reads - just return 1 byte to keep it going
+			t.chunkCnt++
+			return []byte{0x00, 0x90, 0x00}
+		}
+	}
+
+	panic("unexpected APDU")
+}
+
+func TestReadFileTooManyChunksErr(t *testing.T) {
+	// expect error due to too many chunks during ReadFile
+	mockTrans := &MockTransceiverTooManyChunks{}
+	nfc := NewNfcSession(mockTrans)
+
+	nfc.readFileMaxChunks = 100
+
+	_, err := nfc.ReadFile(0x0101) // DG1
+
+	if err == nil {
+		t.Errorf("Expected error")
+	}
+
+	if mockTrans.chunkCnt != 100 {
+		t.Errorf("Expected 100 chunks before error (actual:%1d)", mockTrans.chunkCnt)
+	}
+}

@@ -0,0 +1,53 @@
+package iso7816
+
+import "testing"
+
+// MockTransceiverHugeLength simulates evil chip sending TLV with gigantic length
+type MockTransceiverHugeLength struct {
+	iterationCount int
+}
+
+func (t *MockTransceiverHugeLength) Transceive(cla, ins, p1, p2 int, data []byte, le int, rapdu []byte) []byte {
+	t.iterationCount++
+
+	if ins == int(INS_SELECT) {
+		// SELECT FILE - return success
+		return []byte{0x90, 0x00}
+	}
+
+	if ins == int(INS_READ_BINARY) {
+		offset := p1*256 + p2
+
+		if offset == 0 {
+			// Tag: 0x61 (DG1)
+			// Length: 0x84 0xFF FF FF FF (4GB in long form)
+			return append([]byte{
+				0x61,                   // Tag
+				0x84,                   // Long form length (4 bytes follow)
+				0xFF, 0xFF, 0xFF, 0xFF, // Length = 4GB
+			}, []byte{0x90, 0x00}...)
+		} else {
+			// Subsequent reads - just return some data to keep it going
+			dummyData := make([]byte, le)
+			for i := range dummyData {
+				dummyData[i] = 0xAA
+			}
+			return append(dummyData, []byte{0x90, 0x00}...)
+		}
+	}
+
+	return []byte{0x90, 0x00}
+}
+
+func TestReadFileDoSViaHugeLength(t *testing.T) {
+	mockTrans := &MockTransceiverHugeLength{}
+	nfc := NewNfcSession(mockTrans)
+
+	nfc.MaxLe = 256
+
+	_, err := nfc.ReadFile(0x0101) // DG1
+
+	if err == nil {
+		t.Errorf("Expected error")
+	}
+}