Repository: google/go-tpm (go)

Issue:
Should unpack of U16/U32Bytes validate a max size?
Malicious attacker-controlled input could make its way to `tpmutil.Unpack()` of `U32Bytes` or `U16Bytes`. For instance, calling `tpm.UnmarshalPubRSAPublicKey()` with `20ff ffff 2020 20ff ffff 2020` will cause a massive allocation, crashing the machine.

Given that `TPMUnmarshal` takes a reader, theres no upfront knowledge of the size of the buffer being processed.

Anyone have an opinion on these options?

1. Add a max size (maybe 1mb?) which is larger than all expected use-cases
2. Refactor `tpmutil.Unpack` to pass the size of the buffer down the chain so size prefix fields can be properly validated.

WDYT?

Pull request description:
Add bounds checks for unpacks into U32/U16 bytes
Fixes #154 

Patch:
diff --git a/tpmutil/structures.go b/tpmutil/structures.go
index c67b51a..d7d05fb 100644
--- a/tpmutil/structures.go
+++ b/tpmutil/structures.go
@@ -15,11 +15,19 @@
 package tpmutil
 
 import (
+	"bytes"
 	"encoding/binary"
 	"fmt"
 	"io"
 )
 
+// maxBytesBufferSize sets a sane upper bound on the size of a U32Bytes
+// buffer. This limit exists to prevent a maliciously large size prefix
+// from resulting in a massive memory allocation, potentially causing
+// an OOM condition on the system.
+// We expect no buffer from a TPM to approach 1Mb in size.
+const maxBytesBufferSize = 1024 * 1024 // 1Mb.
+
 // RawBytes is for Pack and RunCommand arguments that are already encoded.
 // Compared to []byte, RawBytes will not be prepended with slice length during
 // encoding.
@@ -52,6 +60,7 @@ func (b *U16Bytes) TPMUnmarshal(in io.Reader) error {
 		return err
 	}
 	size := int(tmpSize)
+
 	if len(*b) >= size {
 		*b = (*b)[:size]
 	} else {
@@ -63,7 +72,7 @@ func (b *U16Bytes) TPMUnmarshal(in io.Reader) error {
 		return err
 	}
 	if n != size {
-		return fmt.Errorf("unable to read all contents in to U16Bytes")
+		return io.ErrUnexpectedEOF
 	}
 	return nil
 }
@@ -95,6 +104,11 @@ func (b *U32Bytes) TPMUnmarshal(in io.Reader) error {
 		return err
 	}
 	size := int(tmpSize)
+
+	if size > maxBytesBufferSize {
+		return bytes.ErrTooLarge
+	}
+
 	if len(*b) >= size {
 		*b = (*b)[:size]
 	} else {


Test patch:
diff --git a/tpmutil/encoding_test.go b/tpmutil/encoding_test.go
index 4cf4aa1..9b4238e 100644
--- a/tpmutil/encoding_test.go
+++ b/tpmutil/encoding_test.go
@@ -364,3 +364,17 @@ func TestUnpackHandlesArea(t *testing.T) {
 		t.Errorf("Unpack(%v, %T): %T = %v, want %v", buf, &out, out, out, want)
 	}
 }
+
+func TestUnpackMalformedBytes(t *testing.T) {
+	// buf is malformed because the leading size prefix is illegally large.
+	buf := []byte{0xff, 0xff, 0xff, 0xff, 0x20}
+
+	var u32 U32Bytes
+	if _, err := Unpack(buf, &u32); err != bytes.ErrTooLarge {
+		t.Errorf("Unpack(U32Bytes{}) returned %q, want %q", err, bytes.ErrTooLarge)
+	}
+	var u16 U16Bytes
+	if _, err := Unpack(buf, &u16); err != io.ErrUnexpectedEOF {
+		t.Errorf("Unpack(U16Bytes{}) returned %q, want %q", err, io.ErrUnexpectedEOF)
+	}
+}