Quote various strings coming from untrusted sources

Typically, use %q instead of %s (or instead of "%s"), to expose various control characters and the like without interpreting them. This is not really comprehensive; the codebase makes no _general_ guarantee that any returned string values are free of control characters or other malicious/misleading metadata. Not even in returned "error" values (which can legitimately contain newlines, if nothing else). Signed-off-by: Miloslav Trmač <mitr@redhat.com>
containers · May 9, 2024 · cebe647 · cebe647
1 parent 24ac26d
commit cebe647
Show file tree

Hide file tree

Showing 28 changed files with 73 additions and 73 deletions.
diff --git a/copy/manifest.go b/copy/manifest.go
@@ -74,7 +74,7 @@ func determineManifestConversion(in determineManifestConversionInputs) (manifest
 	srcType := in.srcMIMEType
 	normalizedSrcType := manifest.NormalizedMIMEType(srcType)
 	if srcType != normalizedSrcType {
-		logrus.Debugf("Source manifest MIME type %s, treating it as %s", srcType, normalizedSrcType)
+		logrus.Debugf("Source manifest MIME type %q, treating it as %q", srcType, normalizedSrcType)
 		srcType = normalizedSrcType
 	}
 
@@ -237,7 +237,7 @@ func (c *copier) determineListConversion(currentListMIMEType string, destSupport
 		}
 	}
 
-	logrus.Debugf("Manifest list has MIME type %s, ordered candidate list [%s]", currentListMIMEType, strings.Join(destSupportedMIMETypes, ", "))
+	logrus.Debugf("Manifest list has MIME type %q, ordered candidate list [%s]", currentListMIMEType, strings.Join(destSupportedMIMETypes, ", "))
 	if len(prioritizedTypes.list) == 0 {
 		return "", nil, fmt.Errorf("destination does not support any supported manifest list types (%v)", manifest.SupportedListMIMETypes)
 	}

diff --git a/docker/archive/reader.go b/docker/archive/reader.go
@@ -78,7 +78,7 @@ func (r *Reader) List() ([][]types.ImageReference, error) {
 			}
 			nt, ok := parsedTag.(reference.NamedTagged)
 			if !ok {
-				return nil, fmt.Errorf("Invalid tag %s (%s): does not contain a tag", tag, parsedTag.String())
+				return nil, fmt.Errorf("Invalid tag %q (%s): does not contain a tag", tag, parsedTag.String())
 			}
 			ref, err := newReference(r.path, nt, -1, r.archive, nil)
 			if err != nil {

diff --git a/docker/daemon/daemon_dest.go b/docker/daemon/daemon_dest.go
@@ -116,7 +116,7 @@ func imageLoad(ctx context.Context, c *client.Client, reader *io.PipeReader) err
 			return fmt.Errorf("parsing docker load progress: %w", err)
 		}
 		if msg.Error != nil {
-			return fmt.Errorf("docker engine reported: %s", msg.Error.Message)
+			return fmt.Errorf("docker engine reported: %q", msg.Error.Message)
 		}
 	}
 	return nil // No error reported = success

diff --git a/docker/internal/tarfile/reader.go b/docker/internal/tarfile/reader.go
@@ -231,7 +231,7 @@ func (r *Reader) openTarComponent(componentPath string) (io.ReadCloser, error) {
 	}
 
 	if !header.FileInfo().Mode().IsRegular() {
-		return nil, fmt.Errorf("Error reading tar archive component %s: not a regular file", header.Name)
+		return nil, fmt.Errorf("Error reading tar archive component %q: not a regular file", header.Name)
 	}
 	succeeded = true
 	return &tarReadCloser{Reader: tarReader, backingFile: f}, nil
@@ -262,7 +262,7 @@ func findTarComponent(inputFile io.Reader, componentPath string) (*tar.Reader, *
 func (r *Reader) readTarComponent(path string, limit int) ([]byte, error) {
 	file, err := r.openTarComponent(path)
 	if err != nil {
-		return nil, fmt.Errorf("loading tar component %s: %w", path, err)
+		return nil, fmt.Errorf("loading tar component %q: %w", path, err)
 	}
 	defer file.Close()
 	bytes, err := iolimits.ReadAtMost(file, limit)

diff --git a/docker/internal/tarfile/src.go b/docker/internal/tarfile/src.go
@@ -95,10 +95,10 @@ func (s *Source) ensureCachedDataIsPresentPrivate() error {
 	}
 	var parsedConfig manifest.Schema2Image // There's a lot of info there, but we only really care about layer DiffIDs.
 	if err := json.Unmarshal(configBytes, &parsedConfig); err != nil {
-		return fmt.Errorf("decoding tar config %s: %w", tarManifest.Config, err)
+		return fmt.Errorf("decoding tar config %q: %w", tarManifest.Config, err)
 	}
 	if parsedConfig.RootFS == nil {
-		return fmt.Errorf("Invalid image config (rootFS is not set): %s", tarManifest.Config)
+		return fmt.Errorf("Invalid image config (rootFS is not set): %q", tarManifest.Config)
 	}
 
 	knownLayers, err := s.prepareLayerData(tarManifest, &parsedConfig)
@@ -144,7 +144,7 @@ func (s *Source) prepareLayerData(tarManifest *ManifestItem, parsedConfig *manif
 		}
 		layerPath := path.Clean(tarManifest.Layers[i])
 		if _, ok := unknownLayerSizes[layerPath]; ok {
-			return nil, fmt.Errorf("Layer tarfile %s used for two different DiffID values", layerPath)
+			return nil, fmt.Errorf("Layer tarfile %q used for two different DiffID values", layerPath)
 		}
 		li := &layerInfo{ // A new element in each iteration
 			path: layerPath,
@@ -179,15 +179,15 @@ func (s *Source) prepareLayerData(tarManifest *ManifestItem, parsedConfig *manif
 			// the slower method of checking if it's compressed.
 			uncompressedStream, isCompressed, err := compression.AutoDecompress(t)
 			if err != nil {
-				return nil, fmt.Errorf("auto-decompressing %s to determine its size: %w", layerPath, err)
+				return nil, fmt.Errorf("auto-decompressing %q to determine its size: %w", layerPath, err)
 			}
 			defer uncompressedStream.Close()
 
 			uncompressedSize := h.Size
 			if isCompressed {
 				uncompressedSize, err = io.Copy(io.Discard, uncompressedStream)
 				if err != nil {
-					return nil, fmt.Errorf("reading %s to find its size: %w", layerPath, err)
+					return nil, fmt.Errorf("reading %q to find its size: %w", layerPath, err)
 				}
 			}
 			li.size = uncompressedSize

diff --git a/docker/registries_d.go b/docker/registries_d.go
@@ -140,7 +140,7 @@ func loadAndMergeConfig(dirPath string) (*registryConfiguration, error) {
 
 		if config.DefaultDocker != nil {
 			if mergedConfig.DefaultDocker != nil {
-				return nil, fmt.Errorf(`Error parsing signature storage configuration: "default-docker" defined both in "%s" and "%s"`,
+				return nil, fmt.Errorf(`Error parsing signature storage configuration: "default-docker" defined both in %q and %q`,
 					dockerDefaultMergedFrom, configPath)
 			}
 			mergedConfig.DefaultDocker = config.DefaultDocker
@@ -149,7 +149,7 @@ func loadAndMergeConfig(dirPath string) (*registryConfiguration, error) {
 
 		for nsName, nsConfig := range config.Docker { // includes config.Docker == nil
 			if _, ok := mergedConfig.Docker[nsName]; ok {
-				return nil, fmt.Errorf(`Error parsing signature storage configuration: "docker" namespace "%s" defined both in "%s" and "%s"`,
+				return nil, fmt.Errorf(`Error parsing signature storage configuration: "docker" namespace %q defined both in %q and %q`,
 					nsName, nsMergedFrom[nsName], configPath)
 			}
 			mergedConfig.Docker[nsName] = nsConfig

diff --git a/internal/image/manifest.go b/internal/image/manifest.go
@@ -76,7 +76,7 @@ func manifestInstanceFromBlob(ctx context.Context, sys *types.SystemContext, src
 	case imgspecv1.MediaTypeImageIndex:
 		return manifestOCI1FromImageIndex(ctx, sys, src, manblob)
 	default: // Note that this may not be reachable, manifest.NormalizedMIMEType has a default for unknown values.
-		return nil, fmt.Errorf("Unimplemented manifest MIME type %s", mt)
+		return nil, fmt.Errorf("Unimplemented manifest MIME type %q", mt)
 	}
 }
 

diff --git a/internal/manifest/docker_schema2_list.go b/internal/manifest/docker_schema2_list.go
@@ -164,7 +164,7 @@ func (list *Schema2ListPublic) ChooseInstance(ctx *types.SystemContext) (digest.
 			}
 		}
 	}
-	return "", fmt.Errorf("no image found in manifest list for architecture %s, variant %q, OS %s", wantedPlatforms[0].Architecture, wantedPlatforms[0].Variant, wantedPlatforms[0].OS)
+	return "", fmt.Errorf("no image found in manifest list for architecture %q, variant %q, OS %q", wantedPlatforms[0].Architecture, wantedPlatforms[0].Variant, wantedPlatforms[0].OS)
 }
 
 // Serialize returns the list in a blob format.

diff --git a/internal/manifest/list.go b/internal/manifest/list.go
@@ -129,5 +129,5 @@ func ListFromBlob(manifest []byte, manifestMIMEType string) (List, error) {
 	case DockerV2Schema1MediaType, DockerV2Schema1SignedMediaType, imgspecv1.MediaTypeImageManifest, DockerV2Schema2MediaType:
 		return nil, fmt.Errorf("Treating single images as manifest lists is not implemented")
 	}
-	return nil, fmt.Errorf("Unimplemented manifest list MIME type %s (normalized as %s)", manifestMIMEType, normalized)
+	return nil, fmt.Errorf("Unimplemented manifest list MIME type %q (normalized as %q)", manifestMIMEType, normalized)
 }
diff --git a/internal/manifest/oci_index.go b/internal/manifest/oci_index.go
@@ -260,7 +260,7 @@ func (index *OCI1IndexPublic) chooseInstance(ctx *types.SystemContext, preferGzi
 	if bestMatch != nil {
 		return bestMatch.digest, nil
 	}
-	return "", fmt.Errorf("no image found in image index for architecture %s, variant %q, OS %s", wantedPlatforms[0].Architecture, wantedPlatforms[0].Variant, wantedPlatforms[0].OS)
+	return "", fmt.Errorf("no image found in image index for architecture %q, variant %q, OS %q", wantedPlatforms[0].Architecture, wantedPlatforms[0].Variant, wantedPlatforms[0].OS)
 }
 
 func (index *OCI1Index) ChooseInstanceByCompression(ctx *types.SystemContext, preferGzip types.OptionalBool) (digest.Digest, error) {

diff --git a/manifest/common.go b/manifest/common.go
@@ -67,15 +67,15 @@ func compressionVariantMIMEType(variantTable []compressionMIMETypeSet, mimeType
 			return "", ManifestLayerCompressionIncompatibilityError{fmt.Sprintf("uncompressed variant is not supported for type %q", mimeType)}
 		}
 		if name != mtsUncompressed {
-			return "", ManifestLayerCompressionIncompatibilityError{fmt.Sprintf("unknown compressed with algorithm %s variant for type %s", name, mimeType)}
+			return "", ManifestLayerCompressionIncompatibilityError{fmt.Sprintf("unknown compressed with algorithm %s variant for type %q", name, mimeType)}
 		}
 		// We can't very well say “the idea of no compression is unknown”
 		return "", ManifestLayerCompressionIncompatibilityError{fmt.Sprintf("uncompressed variant is not supported for type %q", mimeType)}
 	}
 	if algorithm != nil {
-		return "", fmt.Errorf("unsupported MIME type for compression: %s", mimeType)
+		return "", fmt.Errorf("unsupported MIME type for compression: %q", mimeType)
 	}
-	return "", fmt.Errorf("unsupported MIME type for decompression: %s", mimeType)
+	return "", fmt.Errorf("unsupported MIME type for decompression: %q", mimeType)
 }
 
 // updatedMIMEType returns the result of applying edits in updated (MediaType, CompressionOperation) to

diff --git a/manifest/docker_schema1.go b/manifest/docker_schema1.go
@@ -221,7 +221,7 @@ func (m *Schema1) fixManifestLayers() error {
 			m.History = slices.Delete(m.History, i, i+1)
 			m.ExtractedV1Compatibility = slices.Delete(m.ExtractedV1Compatibility, i, i+1)
 		} else if m.ExtractedV1Compatibility[i].Parent != m.ExtractedV1Compatibility[i+1].ID {
-			return fmt.Errorf("Invalid parent ID. Expected %v, got %v", m.ExtractedV1Compatibility[i+1].ID, m.ExtractedV1Compatibility[i].Parent)
+			return fmt.Errorf("Invalid parent ID. Expected %v, got %q", m.ExtractedV1Compatibility[i+1].ID, m.ExtractedV1Compatibility[i].Parent)
 		}
 	}
 	return nil

diff --git a/manifest/manifest.go b/manifest/manifest.go
@@ -166,5 +166,5 @@ func FromBlob(manblob []byte, mt string) (Manifest, error) {
 		return nil, fmt.Errorf("Treating manifest lists as individual manifests is not implemented")
 	}
 	// Note that this may not be reachable, NormalizedMIMEType has a default for unknown values.
-	return nil, fmt.Errorf("Unimplemented manifest MIME type %s (normalized as %s)", mt, nmt)
+	return nil, fmt.Errorf("Unimplemented manifest MIME type %q (normalized as %q)", mt, nmt)
 }
diff --git a/manifest/oci.go b/manifest/oci.go
@@ -167,7 +167,7 @@ func (m *OCI1) UpdateLayerInfos(layerInfos []types.BlobInfo) error {
 // an error if the mediatype does not support encryption
 func getEncryptedMediaType(mediatype string) (string, error) {
 	if slices.Contains(strings.Split(mediatype, "+")[1:], "encrypted") {
-		return "", fmt.Errorf("unsupported mediaType: %v already encrypted", mediatype)
+		return "", fmt.Errorf("unsupported mediaType: %q already encrypted", mediatype)
 	}
 	unsuffixedMediatype := strings.Split(mediatype, "+")[0]
 	switch unsuffixedMediatype {
@@ -176,15 +176,15 @@ func getEncryptedMediaType(mediatype string) (string, error) {
 		return mediatype + "+encrypted", nil
 	}
 
-	return "", fmt.Errorf("unsupported mediaType to encrypt: %v", mediatype)
+	return "", fmt.Errorf("unsupported mediaType to encrypt: %q", mediatype)
 }
 
 // getDecryptedMediaType will return the mediatype to its encrypted counterpart and return
 // an error if the mediatype does not support decryption
 func getDecryptedMediaType(mediatype string) (string, error) {
 	res, ok := strings.CutSuffix(mediatype, "+encrypted")
 	if !ok {
-		return "", fmt.Errorf("unsupported mediaType to decrypt: %v", mediatype)
+		return "", fmt.Errorf("unsupported mediaType to decrypt: %q", mediatype)
 	}
 
 	return res, nil

diff --git a/oci/layout/oci_src.go b/oci/layout/oci_src.go
@@ -182,19 +182,19 @@ func (s *ociImageSource) getExternalBlob(ctx context.Context, urls []string) (io
 		hasSupportedURL = true
 		req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil)
 		if err != nil {
-			errWrap = fmt.Errorf("fetching %s failed %s: %w", u, err.Error(), errWrap)
+			errWrap = fmt.Errorf("fetching %q failed %s: %w", u, err.Error(), errWrap)
 			continue
 		}
 
 		resp, err := s.client.Do(req)
 		if err != nil {
-			errWrap = fmt.Errorf("fetching %s failed %s: %w", u, err.Error(), errWrap)
+			errWrap = fmt.Errorf("fetching %q failed %s: %w", u, err.Error(), errWrap)
 			continue
 		}
 
 		if resp.StatusCode != http.StatusOK {
 			resp.Body.Close()
-			errWrap = fmt.Errorf("fetching %s failed, response code not 200: %w", u, errWrap)
+			errWrap = fmt.Errorf("fetching %q failed, response code not 200: %w", u, errWrap)
 			continue
 		}
 

diff --git a/openshift/openshift-copies.go b/openshift/openshift-copies.go
@@ -553,7 +553,7 @@ func (rules *clientConfigLoadingRules) Load() (*clientcmdConfig, error) {
 			continue
 		}
 		if err != nil {
-			errlist = append(errlist, fmt.Errorf("loading config file \"%s\": %w", filename, err))
+			errlist = append(errlist, fmt.Errorf("loading config file %q: %w", filename, err))
 			continue
 		}
 

diff --git a/openshift/openshift.go b/openshift/openshift.go
@@ -152,7 +152,7 @@ func (c *openshiftClient) getImage(ctx context.Context, imageStreamImageName str
 func (c *openshiftClient) convertDockerImageReference(ref string) (string, error) {
 	_, repo, gotRepo := strings.Cut(ref, "/")
 	if !gotRepo {
-		return "", fmt.Errorf("Invalid format of docker reference %s: missing '/'", ref)
+		return "", fmt.Errorf("Invalid format of docker reference %q: missing '/'", ref)
 	}
 	return reference.Domain(c.ref.dockerReference) + "/" + repo, nil
 }

diff --git a/signature/docker.go b/signature/docker.go
@@ -76,10 +76,10 @@ func VerifyImageManifestSignatureUsingKeyIdentityList(unverifiedSignature, unver
 		validateSignedDockerReference: func(signedDockerReference string) error {
 			signedRef, err := reference.ParseNormalizedNamed(signedDockerReference)
 			if err != nil {
-				return internal.NewInvalidSignatureError(fmt.Sprintf("Invalid docker reference %s in signature", signedDockerReference))
+				return internal.NewInvalidSignatureError(fmt.Sprintf("Invalid docker reference %q in signature", signedDockerReference))
 			}
 			if signedRef.String() != expectedRef.String() {
-				return internal.NewInvalidSignatureError(fmt.Sprintf("Docker reference %s does not match %s",
+				return internal.NewInvalidSignatureError(fmt.Sprintf("Docker reference %q does not match %q",
 					signedDockerReference, expectedDockerReference))
 			}
 			return nil

diff --git a/signature/fulcio_cert.go b/signature/fulcio_cert.go
@@ -178,7 +178,7 @@ func (f *fulcioTrustRoot) verifyFulcioCertificateAtTime(relevantTime time.Time,
 
 	// == Validate the OIDC subject
 	if !slices.Contains(untrustedCertificate.EmailAddresses, f.subjectEmail) {
-		return nil, internal.NewInvalidSignatureError(fmt.Sprintf("Required email %s not found (got %#v)",
+		return nil, internal.NewInvalidSignatureError(fmt.Sprintf("Required email %q not found (got %q)",
 			f.subjectEmail,
 			untrustedCertificate.EmailAddresses))
 	}

diff --git a/signature/fulcio_cert_test.go b/signature/fulcio_cert_test.go
@@ -327,7 +327,7 @@ func TestFulcioTrustRootVerifyFulcioCertificateAtTime(t *testing.T) {
 					Value:    sansBytes,
 				})
 			},
-			errorFragment: "Required email test-user@example.com not found",
+			errorFragment: `Required email "test-user@example.com" not found`,
 		},
 		{ // Other completely unrecognized critical extensions still cause failures
 			name: "Unhandled critical extension",
@@ -367,7 +367,7 @@ func TestFulcioTrustRootVerifyFulcioCertificateAtTime(t *testing.T) {
 			fn: func(cert *x509.Certificate) {
 				cert.EmailAddresses = nil
 			},
-			errorFragment: "Required email test-user@example.com not found",
+			errorFragment: `Required email "test-user@example.com" not found`,
 		},
 		{
 			name: "Multiple emails, one matches",
@@ -381,14 +381,14 @@ func TestFulcioTrustRootVerifyFulcioCertificateAtTime(t *testing.T) {
 			fn: func(cert *x509.Certificate) {
 				cert.EmailAddresses = []string{"a@example.com"}
 			},
-			errorFragment: "Required email test-user@example.com not found",
+			errorFragment: `Required email "test-user@example.com" not found`,
 		},
 		{
 			name: "Multiple emails, no matches",
 			fn: func(cert *x509.Certificate) {
 				cert.EmailAddresses = []string{"a@example.com", "b@example.com", "c@example.com"}
 			},
-			errorFragment: "Required email test-user@example.com not found",
+			errorFragment: `Required email "test-user@example.com" not found`,
 		},
 	} {
 		testLeafKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)

diff --git a/signature/internal/json.go b/signature/internal/json.go
@@ -31,7 +31,7 @@ func ParanoidUnmarshalJSONObject(data []byte, fieldResolver func(string) any) er
 		return JSONFormatError(err.Error())
 	}
 	if t != json.Delim('{') {
-		return JSONFormatError(fmt.Sprintf("JSON object expected, got \"%s\"", t))
+		return JSONFormatError(fmt.Sprintf("JSON object expected, got %#v", t))
 	}
 	for {
 		t, err := dec.Token()
@@ -45,16 +45,16 @@ func ParanoidUnmarshalJSONObject(data []byte, fieldResolver func(string) any) er
 		key, ok := t.(string)
 		if !ok {
 			// Coverage: This should never happen, dec.Token() rejects non-string-literals in this state.
-			return JSONFormatError(fmt.Sprintf("Key string literal expected, got \"%s\"", t))
+			return JSONFormatError(fmt.Sprintf("Key string literal expected, got %#v", t))
 		}
 		if seenKeys.Contains(key) {
-			return JSONFormatError(fmt.Sprintf("Duplicate key \"%s\"", key))
+			return JSONFormatError(fmt.Sprintf("Duplicate key %q", key))
 		}
 		seenKeys.Add(key)
 
 		valuePtr := fieldResolver(key)
 		if valuePtr == nil {
-			return JSONFormatError(fmt.Sprintf("Unknown key \"%s\"", key))
+			return JSONFormatError(fmt.Sprintf("Unknown key %q", key))
 		}
 		// This works like json.Unmarshal, in particular it allows us to implement UnmarshalJSON to implement strict parsing of the field value.
 		if err := dec.Decode(valuePtr); err != nil {
@@ -83,7 +83,7 @@ func ParanoidUnmarshalJSONObjectExactFields(data []byte, exactFields map[string]
 	}
 	for key := range exactFields {
 		if !seenKeys.Contains(key) {
-			return JSONFormatError(fmt.Sprintf(`Key "%s" missing in a JSON object`, key))
+			return JSONFormatError(fmt.Sprintf(`Key %q missing in a JSON object`, key))
 		}
 	}
 	return nil
-Original file line number
+Diff line change
@@ Expand Up @@
     			}
     		}
     	}
-    	return "", fmt.Errorf("no image found in manifest list for architecture %s, variant %q, OS %s", wantedPlatforms[0].Architecture, wantedPlatforms[0].Variant, wantedPlatforms[0].OS)
+    	return "", fmt.Errorf("no image found in manifest list for architecture %q, variant %q, OS %q", wantedPlatforms[0].Architecture, wantedPlatforms[0].Variant, wantedPlatforms[0].OS)
     }
     // Serialize returns the list in a blob format.
@@ Expand Down @@