From a9225e40c84ab94bd477e77ce26d055269586a6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 18 Apr 2024 00:14:38 +0200 Subject: [PATCH] Call .Validate() before digest.Digest.String() if necessary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to prevent unexpected behavior on invalid values. Signed-off-by: Miloslav Trmač --- docker/docker_client.go | 20 +++++++++++++++++--- docker/docker_image_dest.go | 11 ++++++++++- docker/docker_image_src.go | 8 ++++++++ docker/internal/tarfile/writer.go | 3 +++ openshift/openshift_src.go | 3 +++ pkg/blobcache/blobcache.go | 12 +++++++++--- pkg/blobcache/dest.go | 15 ++++++++++++--- pkg/blobcache/src.go | 16 ++++++++++++---- storage/storage_dest.go | 12 ++++++++++-- storage/storage_image.go | 7 +++++-- storage/storage_reference.go | 10 ++++++++-- storage/storage_src.go | 10 ++++++++-- 12 files changed, 105 insertions(+), 22 deletions(-) diff --git a/docker/docker_client.go b/docker/docker_client.go index a60ec563d3..737c778a05 100644 --- a/docker/docker_client.go +++ b/docker/docker_client.go @@ -952,6 +952,8 @@ func (c *dockerClient) detectProperties(ctx context.Context) error { return c.detectPropertiesError } +// fetchManifest fetches a manifest for (the repo of ref) + tagOrDigest. +// The caller is responsible for ensuring tagOrDigest uses the expected format. func (c *dockerClient) fetchManifest(ctx context.Context, ref dockerReference, tagOrDigest string) ([]byte, string, error) { path := fmt.Sprintf(manifestPath, reference.Path(ref.ref), tagOrDigest) headers := map[string][]string{ @@ -1035,6 +1037,9 @@ func (c *dockerClient) getBlob(ctx context.Context, ref dockerReference, info ty } } + if err := info.Digest.Validate(); err != nil { // Make sure info.Digest.String() does not contain any unexpected characters + return nil, 0, err + } path := fmt.Sprintf(blobsPath, reference.Path(ref.ref), info.Digest.String()) logrus.Debugf("Downloading %s", path) res, err := c.makeRequest(ctx, http.MethodGet, path, nil, nil, v2Auth, nil) @@ -1098,7 +1103,10 @@ func isManifestUnknownError(err error) bool { // digest in ref. // It returns (nil, nil) if the manifest does not exist. func (c *dockerClient) getSigstoreAttachmentManifest(ctx context.Context, ref dockerReference, digest digest.Digest) (*manifest.OCI1, error) { - tag := sigstoreAttachmentTag(digest) + tag, err := sigstoreAttachmentTag(digest) + if err != nil { + return nil, err + } sigstoreRef, err := reference.WithTag(reference.TrimNamed(ref.ref), tag) if err != nil { return nil, err @@ -1131,6 +1139,9 @@ func (c *dockerClient) getSigstoreAttachmentManifest(ctx context.Context, ref do // getExtensionsSignatures returns signatures from the X-Registry-Supports-Signatures API extension, // using the original data structures. func (c *dockerClient) getExtensionsSignatures(ctx context.Context, ref dockerReference, manifestDigest digest.Digest) (*extensionSignatureList, error) { + if err := manifestDigest.Validate(); err != nil { // Make sure manifestDigest.String() does not contain any unexpected characters + return nil, err + } path := fmt.Sprintf(extensionsSignaturePath, reference.Path(ref.ref), manifestDigest) res, err := c.makeRequest(ctx, http.MethodGet, path, nil, nil, v2Auth, nil) if err != nil { @@ -1154,8 +1165,11 @@ func (c *dockerClient) getExtensionsSignatures(ctx context.Context, ref dockerRe } // sigstoreAttachmentTag returns a sigstore attachment tag for the specified digest. -func sigstoreAttachmentTag(d digest.Digest) string { - return strings.Replace(d.String(), ":", "-", 1) + ".sig" +func sigstoreAttachmentTag(d digest.Digest) (string, error) { + if err := d.Validate(); err != nil { // Make sure d.String() doesn’t contain any unexpected characters + return "", err + } + return strings.Replace(d.String(), ":", "-", 1) + ".sig", nil } // Close removes resources associated with an initialized dockerClient, if any. diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index b9146c26d9..a29150eb7e 100644 --- a/docker/docker_image_dest.go +++ b/docker/docker_image_dest.go @@ -230,6 +230,9 @@ func (d *dockerImageDestination) PutBlobWithOptions(ctx context.Context, stream // If the destination does not contain the blob, or it is unknown, blobExists ordinarily returns (false, -1, nil); // it returns a non-nil error only on an unexpected failure. func (d *dockerImageDestination) blobExists(ctx context.Context, repo reference.Named, digest digest.Digest, extraScope *authScope) (bool, int64, error) { + if err := digest.Validate(); err != nil { // Make sure digest.String() does not contain any unexpected characters + return false, -1, err + } checkPath := fmt.Sprintf(blobsPath, reference.Path(repo), digest.String()) logrus.Debugf("Checking %s", checkPath) res, err := d.c.makeRequest(ctx, http.MethodHead, checkPath, nil, nil, v2Auth, extraScope) @@ -469,6 +472,7 @@ func (d *dockerImageDestination) PutManifest(ctx context.Context, m []byte, inst // particular instance. refTail = instanceDigest.String() // Double-check that the manifest we've been given matches the digest we've been given. + // This also validates the format of instanceDigest. matches, err := manifest.MatchesDigest(m, *instanceDigest) if err != nil { return fmt.Errorf("digesting manifest in PutManifest: %w", err) @@ -783,8 +787,12 @@ func (d *dockerImageDestination) putSignaturesToSigstoreAttachments(ctx context. if err != nil { return err } + attachmentTag, err := sigstoreAttachmentTag(manifestDigest) + if err != nil { + return err + } logrus.Debugf("Uploading sigstore attachment manifest") - return d.uploadManifest(ctx, manifestBlob, sigstoreAttachmentTag(manifestDigest)) + return d.uploadManifest(ctx, manifestBlob, attachmentTag) } func layerMatchesSigstoreSignature(layer imgspecv1.Descriptor, mimeType string, @@ -900,6 +908,7 @@ func (d *dockerImageDestination) putSignaturesToAPIExtension(ctx context.Context return err } + // manifestDigest is known to be valid because it was not rejected by getExtensionsSignatures above. path := fmt.Sprintf(extensionsSignaturePath, reference.Path(d.ref.ref), manifestDigest.String()) res, err := d.c.makeRequest(ctx, http.MethodPut, path, nil, bytes.NewReader(body), v2Auth, nil) if err != nil { diff --git a/docker/docker_image_src.go b/docker/docker_image_src.go index b78b9b5163..274cd6dd2c 100644 --- a/docker/docker_image_src.go +++ b/docker/docker_image_src.go @@ -194,6 +194,9 @@ func simplifyContentType(contentType string) string { // this never happens if the primary manifest is not a manifest list (e.g. if the source never returns manifest lists). func (s *dockerImageSource) GetManifest(ctx context.Context, instanceDigest *digest.Digest) ([]byte, string, error) { if instanceDigest != nil { + if err := instanceDigest.Validate(); err != nil { // Make sure instanceDigest.String() does not contain any unexpected characters + return nil, "", err + } return s.fetchManifest(ctx, instanceDigest.String()) } err := s.ensureManifestIsLoaded(ctx) @@ -203,6 +206,8 @@ func (s *dockerImageSource) GetManifest(ctx context.Context, instanceDigest *dig return s.cachedManifest, s.cachedManifestMIMEType, nil } +// fetchManifest fetches a manifest for tagOrDigest. +// The caller is responsible for ensuring tagOrDigest uses the expected format. func (s *dockerImageSource) fetchManifest(ctx context.Context, tagOrDigest string) ([]byte, string, error) { return s.c.fetchManifest(ctx, s.physicalRef, tagOrDigest) } @@ -352,6 +357,9 @@ func (s *dockerImageSource) GetBlobAt(ctx context.Context, info types.BlobInfo, return nil, nil, fmt.Errorf("external URLs not supported with GetBlobAt") } + if err := info.Digest.Validate(); err != nil { // Make sure info.Digest.String() does not contain any unexpected characters + return nil, nil, err + } path := fmt.Sprintf(blobsPath, reference.Path(s.physicalRef.ref), info.Digest.String()) logrus.Debugf("Downloading %s", path) res, err := s.c.makeRequest(ctx, http.MethodGet, path, headers, nil, v2Auth, nil) diff --git a/docker/internal/tarfile/writer.go b/docker/internal/tarfile/writer.go index 2d83254d78..7f6bd0e6be 100644 --- a/docker/internal/tarfile/writer.go +++ b/docker/internal/tarfile/writer.go @@ -142,6 +142,9 @@ func (w *Writer) writeLegacyMetadataLocked(layerDescriptors []manifest.Schema2De } // This chainID value matches the computation in docker/docker/layer.CreateChainID … + if err := l.Digest.Validate(); err != nil { // This should never fail on this code path, still: make sure the chainID computation is unambiguous. + return err + } if chainID == "" { chainID = l.Digest } else { diff --git a/openshift/openshift_src.go b/openshift/openshift_src.go index 0ac0127ee7..62774afbb7 100644 --- a/openshift/openshift_src.go +++ b/openshift/openshift_src.go @@ -109,6 +109,9 @@ func (s *openshiftImageSource) GetSignaturesWithFormat(ctx context.Context, inst } imageStreamImageName = s.imageStreamImageName } else { + if err := instanceDigest.Validate(); err != nil { // Make sure instanceDigest.String() does not contain any unexpected characters + return nil, err + } imageStreamImageName = instanceDigest.String() } image, err := s.client.getImage(ctx, imageStreamImageName) diff --git a/pkg/blobcache/blobcache.go b/pkg/blobcache/blobcache.go index 2bbf48848a..f4de6cebfe 100644 --- a/pkg/blobcache/blobcache.go +++ b/pkg/blobcache/blobcache.go @@ -78,12 +78,15 @@ func (b *BlobCache) DeleteImage(ctx context.Context, sys *types.SystemContext) e } // blobPath returns the path appropriate for storing a blob with digest. -func (b *BlobCache) blobPath(digest digest.Digest, isConfig bool) string { +func (b *BlobCache) blobPath(digest digest.Digest, isConfig bool) (string, error) { + if err := digest.Validate(); err != nil { // Make sure digest.String() does not contain any unexpected characters + return "", err + } baseName := digest.String() if isConfig { baseName += ".config" } - return filepath.Join(b.directory, baseName) + return filepath.Join(b.directory, baseName), nil } // findBlob checks if we have a blob for info in cache (whether a config or not) @@ -95,7 +98,10 @@ func (b *BlobCache) findBlob(info types.BlobInfo) (string, int64, bool, error) { } for _, isConfig := range []bool{false, true} { - path := b.blobPath(info.Digest, isConfig) + path, err := b.blobPath(info.Digest, isConfig) + if err != nil { + return "", -1, false, err + } fileInfo, err := os.Stat(path) if err == nil && (info.Size == -1 || info.Size == fileInfo.Size()) { return path, fileInfo.Size(), isConfig, nil diff --git a/pkg/blobcache/dest.go b/pkg/blobcache/dest.go index fb2efe4c4b..d8899f2ed9 100644 --- a/pkg/blobcache/dest.go +++ b/pkg/blobcache/dest.go @@ -114,7 +114,10 @@ func (d *blobCacheDestination) saveStream(wg *sync.WaitGroup, decompressReader i } // Determine the name that we should give to the uncompressed copy of the blob. - decompressedFilename := d.reference.blobPath(digester.Digest(), isConfig) + decompressedFilename, err := d.reference.blobPath(digester.Digest(), isConfig) + if err != nil { + return + } // Rename the temporary file. if err := os.Rename(tempFile.Name(), decompressedFilename); err != nil { logrus.Debugf("error renaming new decompressed copy of blob %q into place at %q: %v", digester.Digest().String(), decompressedFilename, err) @@ -152,7 +155,10 @@ func (d *blobCacheDestination) PutBlobWithOptions(ctx context.Context, stream io needToWait := false compression := archive.Uncompressed if inputInfo.Digest != "" { - filename := d.reference.blobPath(inputInfo.Digest, options.IsConfig) + filename, err2 := d.reference.blobPath(inputInfo.Digest, options.IsConfig) + if err2 != nil { + return private.UploadedBlob{}, err2 + } tempfile, err = os.CreateTemp(filepath.Dir(filename), filepath.Base(filename)) if err == nil { stream = io.TeeReader(stream, tempfile) @@ -281,7 +287,10 @@ func (d *blobCacheDestination) PutManifest(ctx context.Context, manifestBytes [] if err != nil { logrus.Warnf("error digesting manifest %q: %v", string(manifestBytes), err) } else { - filename := d.reference.blobPath(manifestDigest, false) + filename, err := d.reference.blobPath(manifestDigest, false) + if err != nil { + return err + } if err = ioutils.AtomicWriteFile(filename, manifestBytes, 0600); err != nil { logrus.Warnf("error saving manifest as %q: %v", filename, err) } diff --git a/pkg/blobcache/src.go b/pkg/blobcache/src.go index 2fe108cda1..600d2fa7a5 100644 --- a/pkg/blobcache/src.go +++ b/pkg/blobcache/src.go @@ -56,7 +56,10 @@ func (s *blobCacheSource) Close() error { func (s *blobCacheSource) GetManifest(ctx context.Context, instanceDigest *digest.Digest) ([]byte, string, error) { if instanceDigest != nil { - filename := s.reference.blobPath(*instanceDigest, false) + filename, err := s.reference.blobPath(*instanceDigest, false) + if err != nil { + return nil, "", err + } manifestBytes, err := os.ReadFile(filename) if err == nil { s.cacheHits++ @@ -136,8 +139,10 @@ func (s *blobCacheSource) LayerInfosForCopy(ctx context.Context, instanceDigest replacedInfos := make([]types.BlobInfo, 0, len(infos)) for _, info := range infos { var replaceDigest []byte - var err error - blobFile := s.reference.blobPath(info.Digest, false) + blobFile, err := s.reference.blobPath(info.Digest, false) + if err != nil { + return nil, err + } var alternate string switch s.reference.compress { case types.Compress: @@ -148,7 +153,10 @@ func (s *blobCacheSource) LayerInfosForCopy(ctx context.Context, instanceDigest replaceDigest, err = os.ReadFile(alternate) } if err == nil && digest.Digest(replaceDigest).Validate() == nil { - alternate = s.reference.blobPath(digest.Digest(replaceDigest), false) + alternate, err = s.reference.blobPath(digest.Digest(replaceDigest), false) + if err != nil { + return nil, err + } fileInfo, err := os.Stat(alternate) if err == nil { switch info.MediaType { diff --git a/storage/storage_dest.go b/storage/storage_dest.go index 3803c85a05..bc9790ca31 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -1070,8 +1070,12 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t if err != nil { return fmt.Errorf("digesting top-level manifest: %w", err) } + key, err := manifestBigDataKey(manifestDigest) + if err != nil { + return err + } options.BigData = append(options.BigData, storage.ImageBigDataOption{ - Key: manifestBigDataKey(manifestDigest), + Key: key, Data: toplevelManifest, Digest: manifestDigest, }) @@ -1079,8 +1083,12 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t // Set up to save the image's manifest. Allow looking it up by digest by using the key convention defined by the Store. // Record the manifest twice: using a digest-specific key to allow references to that specific digest instance, // and using storage.ImageDigestBigDataKey for future users that don’t specify any digest and for compatibility with older readers. + key, err := manifestBigDataKey(s.manifestDigest) + if err != nil { + return err + } options.BigData = append(options.BigData, storage.ImageBigDataOption{ - Key: manifestBigDataKey(s.manifestDigest), + Key: key, Data: s.manifest, Digest: s.manifestDigest, }) diff --git a/storage/storage_image.go b/storage/storage_image.go index 530f964b42..7e368857ee 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -21,8 +21,11 @@ var ( // manifestBigDataKey returns a key suitable for recording a manifest with the specified digest using storage.Store.ImageBigData and related functions. // If a specific manifest digest is explicitly requested by the user, the key returned by this function should be used preferably; // for compatibility, if a manifest is not available under this key, check also storage.ImageDigestBigDataKey -func manifestBigDataKey(digest digest.Digest) string { - return storage.ImageDigestManifestBigDataNamePrefix + "-" + digest.String() +func manifestBigDataKey(digest digest.Digest) (string, error) { + if err := digest.Validate(); err != nil { // Make sure info.Digest.String() uses the expected format and does not collide with other BigData keys. + return "", err + } + return storage.ImageDigestManifestBigDataNamePrefix + "-" + digest.String(), nil } // signatureBigDataKey returns a key suitable for recording the signatures associated with the manifest with the specified digest using storage.Store.ImageBigData and related functions. diff --git a/storage/storage_reference.go b/storage/storage_reference.go index a55e34054a..6b7565fd85 100644 --- a/storage/storage_reference.go +++ b/storage/storage_reference.go @@ -73,7 +73,10 @@ func multiArchImageMatchesSystemContext(store storage.Store, img *storage.Image, // We don't need to care about storage.ImageDigestBigDataKey because // manifests lists are only stored into storage by c/image versions // that know about manifestBigDataKey, and only using that key. - key := manifestBigDataKey(manifestDigest) + key, err := manifestBigDataKey(manifestDigest) + if err != nil { + return false // This should never happen, manifestDigest comes from a reference.Digested, and that validates the format. + } manifestBytes, err := store.ImageBigData(img.ID, key) if err != nil { return false @@ -95,7 +98,10 @@ func multiArchImageMatchesSystemContext(store storage.Store, img *storage.Image, if err != nil { return false } - key = manifestBigDataKey(chosenInstance) + key, err = manifestBigDataKey(chosenInstance) + if err != nil { + return false + } _, err = store.ImageBigData(img.ID, key) return err == nil // true if img.ID is based on chosenInstance. } diff --git a/storage/storage_src.go b/storage/storage_src.go index f35a1996e1..27febe1d39 100644 --- a/storage/storage_src.go +++ b/storage/storage_src.go @@ -237,7 +237,10 @@ func (s *storageImageSource) getBlobAndLayerID(digest digest.Digest, layers []st // GetManifest() reads the image's manifest. func (s *storageImageSource) GetManifest(ctx context.Context, instanceDigest *digest.Digest) (manifestBlob []byte, mimeType string, err error) { if instanceDigest != nil { - key := manifestBigDataKey(*instanceDigest) + key, err := manifestBigDataKey(*instanceDigest) + if err != nil { + return nil, "", err + } blob, err := s.imageRef.transport.store.ImageBigData(s.image.ID, key) if err != nil { return nil, "", fmt.Errorf("reading manifest for image instance %q: %w", *instanceDigest, err) @@ -249,7 +252,10 @@ func (s *storageImageSource) GetManifest(ctx context.Context, instanceDigest *di // Prefer the manifest corresponding to the user-specified digest, if available. if s.imageRef.named != nil { if digested, ok := s.imageRef.named.(reference.Digested); ok { - key := manifestBigDataKey(digested.Digest()) + key, err := manifestBigDataKey(digested.Digest()) + if err != nil { + return nil, "", err + } blob, err := s.imageRef.transport.store.ImageBigData(s.image.ID, key) if err != nil && !os.IsNotExist(err) { // os.IsNotExist is true if the image exists but there is no data corresponding to key return nil, "", err