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 added partially because a somewhat overly-permissive reading of
the discussion in opencontainers/image-spec#153 (which was finally
clarified in opencontainers/image-spec#1285), but was also necessary for
some users of VerifiedReadCloser that did not really know the proper
blob size. We have now adjusted all of those callers, so there is no
longer any reason to continue supporting this.

Unknown sizes are a classic DoS vector, so allowing them seems like a
bad idea in general. We might need to adjust this if/when umoci grows
OCI distribution-spec support, but for now it isn't needed.

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,20 @@ 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)
+			assert.ErrorIs(t, err, ErrInvalidExpectedSize, "io.Copy (with ExpectedSize < 0) should fail") //nolint:testifylint // assert.*Error* makes more sense
+			assert.Zero(t, n, "io.Copy (with ExpectedSize < 0) should read nothing")
+
+			// Bad ExpectedSize should read no data.
+			assert.Equal(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")
+			assert.ErrorIs(t, err, ErrInvalidExpectedSize, "Close (with ExpectedSize < 0) should fail") //nolint:testifylint // assert.*Error* makes more sense
+
+			// Bad ExpectedSize should read no data.
+			assert.Equal(t, size, buffer.Len(), "close (with ExpectedSize < 0) should not have read any data")
 		})
 	}
 }
@@ -147,44 +154,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 {