pkg: hardening: disallow negative ExpectedSize

VerifiedReadCloser previously would allow for negative ExpectedSize to
disable the size checking features added in commit ad66299 ("pkg:
hardening: expand to verify descriptor length").

This was based on a somewhat overly-permissive reading of the discussion
in opencontainers/image-spec#153 which was finally clarified in
opencontainers/image-spec#1285. Unknown sizes are a classic DoS vector,
so allowing them seems like a bad idea in general.

Signed-off-by: Aleksa Sarai <[email protected]>

Author: cyphar

pkg/hardening/verified_reader.go

@@ -33,8 +33,9 @@ import (
 // Exported errors for verification issues that occur during processing within
 // VerifiedReadCloser.
 var (
-	ErrDigestMismatch = errors.New("verified reader digest mismatch")
-	ErrSizeMismatch   = errors.New("verified reader size mismatch")
+	ErrDigestMismatch      = errors.New("verified reader digest mismatch")
+	ErrSizeMismatch        = errors.New("verified reader size mismatch")
+	ErrInvalidExpectedSize = errors.New("verified reader has invalid expected size")
 )
 
 // VerifiedReadCloser is a basic io.ReadCloser which allows for simple
@@ -87,21 +88,25 @@ func (v *VerifiedReadCloser) isNoop() bool {
 		innerV.ExpectedSize == v.ExpectedSize
 }
 
+func (v *VerifiedReadCloser) check() error {
+	if v.ExpectedSize < 0 {
+		return fmt.Errorf("%w: expected size must be non-negative", ErrInvalidExpectedSize)
+	}
+	return nil
+}
+
 func (v *VerifiedReadCloser) verify(nilErr error) error {
 	// Digest mismatch (always takes precedence)?
 	if actualDigest := v.digester.Digest(); actualDigest != v.ExpectedDigest {
 		return fmt.Errorf("expected %s not %s: %w", v.ExpectedDigest, actualDigest, ErrDigestMismatch)
 	}
-	// Do we need to check the size for mismatches?
-	if v.ExpectedSize >= 0 {
-		switch {
-		// Not enough bytes in the stream.
-		case v.currentSize < v.ExpectedSize:
-			return fmt.Errorf("expected %d bytes (only %d bytes in stream): %w", v.ExpectedSize, v.currentSize, ErrSizeMismatch)
-		// We don't read the entire blob, so the message needs to be slightly adjusted.
-		case v.currentSize > v.ExpectedSize:
-			return fmt.Errorf("expected %d bytes (extra bytes in stream): %w", v.ExpectedSize, ErrSizeMismatch)
-		}
+	switch {
+	// Not enough bytes in the stream.
+	case v.currentSize < v.ExpectedSize:
+		return fmt.Errorf("expected %d bytes (only %d bytes in stream): %w", v.ExpectedSize, v.currentSize, ErrSizeMismatch)
+	// We don't read the entire blob, so the message needs to be slightly adjusted.
+	case v.currentSize > v.ExpectedSize:
+		return fmt.Errorf("expected %d bytes (extra bytes in stream): %w", v.ExpectedSize, ErrSizeMismatch)
 	}
 	// Forward the provided error.
 	return nilErr
@@ -111,14 +116,13 @@ func (v *VerifiedReadCloser) verify(nilErr error) error {
 // EOF.  Make sure that you always check for EOF and read-to-the-end for all
 // files.
 func (v *VerifiedReadCloser) Read(p []byte) (n int, err error) {
+	if err := v.check(); err != nil {
+		return 0, err
+	}
 	// Make sure we don't read after v.ExpectedSize has been passed.
 	err = io.EOF
 	left := v.ExpectedSize - v.currentSize
 	switch {
-	// ExpectedSize has been disabled.
-	case v.ExpectedSize < 0:
-		n, err = v.Reader.Read(p)
-
 	// We still have something left to read.
 	case left > 0:
 		if int64(len(p)) > left {
@@ -180,6 +184,9 @@ func (v *VerifiedReadCloser) sourceName() string {
 // Close is a wrapper around VerifiedReadCloser.Reader, but with a digest check
 // which will return an error if the underlying Close() didn't.
 func (v *VerifiedReadCloser) Close() error {
+	if err := v.check(); err != nil {
+		return err
+	}
 	// Consume any remaining bytes to make sure that we've actually read to the
 	// end of the stream. VerifiedReadCloser.Read will not read past
 	// ExpectedSize+1, so we don't need to add a limit here.

pkg/hardening/verified_reader_test.go

@@ -60,7 +60,7 @@ func TestValid(t *testing.T) {
 	}
 }
 
-func TestValidIgnoreLength(t *testing.T) {
+func TestNegativeExpectedSize(t *testing.T) {
 	for size := 1; size <= 16384; size *= 2 {
 		t.Run(fmt.Sprintf("size:%d", size), func(t *testing.T) {
 			// Fill buffer with random data.
@@ -76,13 +76,22 @@ func TestValidIgnoreLength(t *testing.T) {
 				ExpectedSize:   -1,
 			}
 
-			// Make sure if we copy-to-EOF we get no errors.
-			_, err = io.Copy(io.Discard, verifiedReader)
-			require.NoError(t, err, "digest (size ignored) should be correct on EOF")
+			// Make sure that negative ExpectedSize always fails.
+			n, err := io.Copy(io.Discard, verifiedReader)
+			require.Error(t, err, "io.Copy (with ExpectedSize < 0) should fail")
+			require.Zero(t, n, "io.Copy (with ExpectedSize < 0) should read nothing")
+			assert.ErrorIs(t, err, ErrInvalidExpectedSize, "io.Copy (with ExpectedSize < 0) should fail")
+
+			// Bad ExpectedSize should read no data.
+			assert.EqualValues(t, size, buffer.Len(), "io.Copy (with ExpectedSize < 0) should not have read any data")
 
 			// And on close we shouldn't get an error either.
 			err = verifiedReader.Close()
-			require.NoError(t, err, "digest (size ignored) should be correct on Close")
+			require.Error(t, err, "Close (with ExpectedSize < 0) should fail")
+			assert.ErrorIs(t, err, ErrInvalidExpectedSize, "Close (with ExpectedSize < 0) should fail")
+
+			// Bad ExpectedSize should read no data.
+			assert.EqualValues(t, size, buffer.Len(), "close (with ExpectedSize < 0) should not have read any data")
 		})
 	}
 }
@@ -100,7 +109,7 @@ func TestValidTrailing(t *testing.T) {
 			verifiedReader := &VerifiedReadCloser{
 				Reader:         io.NopCloser(buffer),
 				ExpectedDigest: expectedDigest,
-				ExpectedSize:   -1,
+				ExpectedSize:   int64(size),
 			}
 
 			// Read *half* of the bytes, leaving some remaining. We should get
@@ -147,44 +156,6 @@ func TestInvalidDigest(t *testing.T) {
 	}
 }
 
-func TestInvalidDigest_Trailing_NoExpectedSize(t *testing.T) {
-	for size := 1; size <= 16384; size *= 2 {
-		for delta := 1; delta-1 <= size/2; delta *= 2 {
-			t.Run(fmt.Sprintf("size:%d_delta:%d", size, delta), func(t *testing.T) {
-				// Fill buffer with random data.
-				buffer := new(bytes.Buffer)
-				_, err := io.CopyN(buffer, rand.Reader, int64(size))
-				require.NoError(t, err, "fill buffer with random data")
-
-				// Generate a correct hash (for a shorter buffer), but don't
-				// verify the size -- this is to make sure that we actually
-				// read all the bytes.
-				shortBuffer := buffer.Bytes()[:size-delta]
-				expectedDigest := digest.SHA256.FromBytes(shortBuffer)
-				verifiedReader := &VerifiedReadCloser{
-					Reader:         io.NopCloser(buffer),
-					ExpectedDigest: expectedDigest,
-					ExpectedSize:   -1,
-				}
-
-				// Read up to the end of the short buffer. We should get no
-				// errors.
-				_, err = io.CopyN(io.Discard, verifiedReader, int64(size-delta))
-				require.NoErrorf(t, err, "should get no errors when reading %d (%d-%d) bytes", size-delta, size, delta)
-
-				// Check that the digest does actually match right now.
-				verifiedReader.init()
-				err = verifiedReader.verify(nil)
-				require.NoError(t, err, "digest check should succeed at the point we finish the subset")
-
-				// On close we should get the error.
-				err = verifiedReader.Close()
-				assert.ErrorIs(t, err, ErrDigestMismatch, "digest should be invalid on Close") //nolint:testifylint // assert.*Error* makes more sense
-			})
-		}
-	}
-}
-
 func TestInvalidSize_LongBuffer(t *testing.T) {
 	for size := 1; size <= 16384; size *= 2 {
 		for delta := 1; delta-1 <= size/2; delta *= 2 {