Merge commit from fork

fix(client): verify manifest and blob content against requested digest (GHSA-685w-q87j-wqw3)

internal/client/repository.go

@@ -536,6 +536,10 @@ func (ms *manifests) Get(ctx context.Context, dgst digest.Digest, options ...dis
 		}
 	}
 
+	// byDigest records whether the caller pinned the manifest by digest (as
+	// opposed to fetching by tag). When true, the returned content must hash to
+	// the requested digest.
+	byDigest := digestOrTag == ""
 	if digestOrTag == "" {
 		digestOrTag = dgst.String()
 		ref, err = reference.WithDigest(ms.name, dgst)
@@ -589,6 +593,28 @@ func (ms *manifests) Get(ctx context.Context, dgst digest.Digest, options ...dis
 	if err != nil {
 		return nil, err
 	}
+
+	// When the manifest was requested by digest, verify that the content the
+	// registry returned actually hashes to that digest. The server-supplied
+	// Docker-Content-Digest header is not trustworthy: a malicious registry or
+	// a MITM can forge it while serving arbitrary bytes. Without this check,
+	// digest pinning provides no integrity guarantee.
+	if byDigest {
+		// Reject a digest whose algorithm is unsupported/unavailable rather
+		// than letting dgst.Verifier() panic; such a digest cannot be verified,
+		// so fail closed.
+		if err := dgst.Validate(); err != nil {
+			return nil, err
+		}
+		verifier := dgst.Verifier()
+		if _, err := verifier.Write(body); err != nil {
+			return nil, err
+		}
+		if !verifier.Verified() {
+			return nil, fmt.Errorf("manifest digest mismatch: requested %s but received content does not match", dgst)
+		}
+	}
+
 	m, _, err := distribution.UnmarshalManifest(mt, body)
 	if err != nil {
 		return nil, err
@@ -719,16 +745,45 @@ func (bs *blobs) Stat(ctx context.Context, dgst digest.Digest) (v1.Descriptor, e
 	return bs.statter.Stat(ctx, dgst)
 }
 
+// Get fetches the blob for dgst and verifies that the returned content hashes
+// to it. Because the blob is content-addressed, a malicious registry or a MITM
+// can serve arbitrary bytes for a digest-pinned request; buffering and hashing
+// the whole body is what makes digest pinning meaningful. Callers that need an
+// integrity guarantee should use Get rather than Open (see Open).
 func (bs *blobs) Get(ctx context.Context, dgst digest.Digest) ([]byte, error) {
+	// Reject a digest whose algorithm is unsupported/unavailable rather than
+	// letting dgst.Verifier() panic; such a digest cannot be verified, so fail
+	// closed.
+	if err := dgst.Validate(); err != nil {
+		return nil, err
+	}
 	reader, err := bs.Open(ctx, dgst)
 	if err != nil {
 		return nil, err
 	}
 	defer reader.Close()
 
-	return io.ReadAll(reader)
+	body, err := io.ReadAll(reader)
+	if err != nil {
+		return nil, err
+	}
+
+	verifier := dgst.Verifier()
+	if _, err := verifier.Write(body); err != nil {
+		return nil, err
+	}
+	if !verifier.Verified() {
+		return nil, fmt.Errorf("blob digest mismatch: requested %s but received content does not match", dgst)
+	}
+	return body, nil
 }
 
+// Open returns a reader that streams the blob for dgst. The returned content is
+// NOT verified against dgst: a content digest can only be checked once the whole
+// blob has been read, but Open hands bytes to the caller as they stream (and a
+// caller may read only part of the blob or seek), so verification cannot be
+// guaranteed in a single pass. Callers that rely on the digest for integrity
+// must use Get, which buffers and verifies the entire blob before returning it.
 func (bs *blobs) Open(ctx context.Context, dgst digest.Digest) (io.ReadSeekCloser, error) {
 	ref, err := reference.WithDigest(bs.name, dgst)
 	if err != nil {

internal/client/repository_test.go

@@ -346,6 +346,74 @@ func TestBlobFetch(t *testing.T) {
 	// TODO(dmcgowan): Test for unknown blob case
 }
 
+// TestBlobFetchByDigestVerifies ensures that a blob requested by digest via Get
+// is rejected when the registry returns content that does not hash to the
+// requested digest. Regression test for GHSA-685w-q87j-wqw3 (blob path).
+//
+// Get is the integrity-providing API: it buffers and verifies the whole blob.
+// Open streams and is intentionally not verified (a content digest cannot be
+// checked without reading the entire content, which Open does not require).
+func TestBlobFetchByDigestVerifies(t *testing.T) {
+	pinnedDgst, _ := newRandomBlob(1024)
+	_, evilBlob := newRandomBlob(1024)
+
+	// Serve the evil bytes at the pinned digest's blob URL.
+	var m testutil.RequestResponseMap
+	for _, method := range []string{http.MethodGet, http.MethodHead} {
+		resp := testutil.Response{
+			StatusCode: http.StatusOK,
+			Headers: http.Header(map[string][]string{
+				"Content-Length": {fmt.Sprint(len(evilBlob))},
+				"Content-Type":   {"application/octet-stream"},
+				"Last-Modified":  {time.Now().Add(-1 * time.Second).Format(time.ANSIC)},
+			}),
+		}
+		if method == http.MethodGet {
+			resp.Body = evilBlob
+		}
+		m = append(m, testutil.RequestResponseMapping{
+			Request: testutil.Request{
+				Method: method,
+				Route:  "/v2/test.example.com/repo1/blobs/" + pinnedDgst.String(),
+			},
+			Response: resp,
+		})
+	}
+
+	e, c := testServer(m)
+	defer c()
+
+	ctx := dcontext.Background()
+	repo, _ := reference.WithName("test.example.com/repo1")
+	r, err := NewRepository(repo, e, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	l := r.Blobs(ctx)
+
+	if _, err := l.Get(ctx, pinnedDgst); err == nil {
+		t.Fatal("Get accepted blob content that does not match the requested digest")
+	}
+}
+
+// TestBlobGetUnsupportedDigestAlgorithm ensures a syntactically valid digest
+// with an unavailable algorithm is rejected by Get rather than panicking
+// dgst.Verifier().
+func TestBlobGetUnsupportedDigestAlgorithm(t *testing.T) {
+	ctx := dcontext.Background()
+	repo, _ := reference.WithName("test.example.com/repo1")
+	r, err := NewRepository(repo, "http://localhost:5000", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	l := r.Blobs(ctx)
+
+	bad := digest.Digest("md5:0123456789abcdef0123456789abcdef")
+	if _, err := l.Get(ctx, bad); err == nil {
+		t.Fatal("Get accepted a digest with an unsupported algorithm")
+	}
+}
+
 func TestBlobExistsNoContentLength(t *testing.T) {
 	var m testutil.RequestResponseMap
 
@@ -1139,6 +1207,61 @@ func TestOCIManifestFetch(t *testing.T) {
 	}
 }
 
+// TestManifestFetchByDigestVerifies ensures that a manifest requested by digest
+// is rejected when the registry returns content that does not hash to the
+// requested digest, even if it forges the Docker-Content-Digest header.
+// Regression test for GHSA-685w-q87j-wqw3.
+func TestManifestFetchByDigestVerifies(t *testing.T) {
+	ctx := dcontext.Background()
+	repo, _ := reference.WithName("test.example.com/repo")
+
+	// The manifest the caller pinned by digest.
+	_, pinnedDgst, _ := newRandomOCIManifest(t, 6)
+	// A different, attacker-chosen manifest the malicious server returns instead.
+	_, _, evilPayload := newRandomOCIManifest(t, 6)
+
+	var m testutil.RequestResponseMap
+	// Serve the evil payload at the pinned digest's URL, but lie in the
+	// Docker-Content-Digest header, claiming it matches what was requested.
+	m = append(m, testutil.RequestResponseMapping{
+		Request: testutil.Request{
+			Method: http.MethodGet,
+			Route:  "/v2/" + repo.Name() + "/manifests/" + pinnedDgst.String(),
+		},
+		Response: testutil.Response{
+			StatusCode: http.StatusOK,
+			Body:       evilPayload,
+			Headers: http.Header(map[string][]string{
+				"Content-Length":        {fmt.Sprint(len(evilPayload))},
+				"Last-Modified":         {time.Now().Add(-1 * time.Second).Format(time.ANSIC)},
+				"Content-Type":          {v1.MediaTypeImageManifest},
+				"Docker-Content-Digest": {pinnedDgst.String()}, // forged
+			}),
+		},
+	})
+
+	// Sanity check: the two manifests really are different.
+	if digest.FromBytes(evilPayload) == pinnedDgst {
+		t.Fatal("test setup error: evil payload matches pinned digest")
+	}
+
+	e, c := testServer(m)
+	defer c()
+
+	r, err := NewRepository(repo, e, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	ms, err := r.Manifests(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if _, err := ms.Get(ctx, pinnedDgst); err == nil {
+		t.Fatal("expected digest mismatch error, but Get accepted substituted content")
+	}
+}
+
 func TestManifestFetchWithEtag(t *testing.T) {
 	repo, _ := reference.WithName("test.example.com/repo/by/tag")
 	_, d1, p1 := newRandomOCIManifest(t, 6)