Reject ambiguous manifest formats

Refuse to process manifest / manifest list data
that could possibly be interpreted as two different
manifest formats, because differences in how those
ambiguities are resolved could be used to bypass
image verification or review mechanisms.

Fixes CVE-2021-41190 / GHSA-77vh-xpmg-72qh .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

manifest/common.go

@@ -1,6 +1,7 @@
 package manifest
 
 import (
+	"encoding/json"
 	"fmt"
 
 	compressiontypes "github.com/containers/image/v5/pkg/compression/types"
@@ -32,6 +33,72 @@ func dupStringStringMap(m map[string]string) map[string]string {
 	return result
 }
 
+// allowedManifestFields is a bit mask of “essential” manifest fields that validateUnambiguousManifestFormat
+// can expect to be present.
+type allowedManifestFields int
+
+const (
+	allowedFieldConfig allowedManifestFields = 1 << iota
+	allowedFieldFSLayers
+	allowedFieldHistory
+	allowedFieldLayers
+	allowedFieldManifests
+	allowedFieldFirstUnusedBit // Keep this at the end!
+)
+
+// validateUnambiguousManifestFormat rejects manifests (incl. multi-arch) that look like more than
+// one kind we currently recognize, i.e. if they contain any of the known “essential” format fields
+// other than the ones the caller specifically allows.
+// expectedMIMEType is used only for diagnostics.
+// NOTE: The caller should do the non-heuristic validations (e.g. check for any specified format
+// identification/version, or other “magic numbers”) before calling this, to cleanly reject unambigous
+// data that just isn’t what was expected, as opposed to actually ambiguous data.
+func validateUnambiguousManifestFormat(manifest []byte, expectedMIMEType string,
+	allowed allowedManifestFields) error {
+	if allowed >= allowedFieldFirstUnusedBit {
+		return fmt.Errorf("internal error: invalid allowedManifestFields value %#v", allowed)
+	}
+	// Use a private type to decode, not just a map[string]interface{}, because we want
+	// to also reject case-insensitive matches (which would be used by Go when really decoding
+	// the manifest).
+	// (It is expected that as manifest formats are added or extended over time, more fields will be added
+	// here.)
+	detectedFields := struct {
+		Config    interface{} `json:"config"`
+		FSLayers  interface{} `json:"fsLayers"`
+		History   interface{} `json:"history"`
+		Layers    interface{} `json:"layers"`
+		Manifests interface{} `json:"manifests"`
+	}{}
+	if err := json.Unmarshal(manifest, &detectedFields); err != nil {
+		// The caller was supposed to already validate version numbers, so this shold not happen;
+		// let’s not bother with making this error “nice”.
+		return err
+	}
+	unexpected := []string{}
+	// Sadly this isn’t easy to automate in Go, without reflection. So, copy&paste.
+	if detectedFields.Config != nil && (allowed&allowedFieldConfig) == 0 {
+		unexpected = append(unexpected, "config")
+	}
+	if detectedFields.FSLayers != nil && (allowed&allowedFieldFSLayers) == 0 {
+		unexpected = append(unexpected, "fsLayers")
+	}
+	if detectedFields.History != nil && (allowed&allowedFieldHistory) == 0 {
+		unexpected = append(unexpected, "history")
+	}
+	if detectedFields.Layers != nil && (allowed&allowedFieldLayers) == 0 {
+		unexpected = append(unexpected, "layers")
+	}
+	if detectedFields.Manifests != nil && (allowed&allowedFieldManifests) == 0 {
+		unexpected = append(unexpected, "manifests")
+	}
+	if len(unexpected) != 0 {
+		return fmt.Errorf(`rejecting ambiguous manifest, unexpected fields %#v in supposedly %s`,
+			unexpected, expectedMIMEType)
+	}
+	return nil
+}
+
 // layerInfosToStrings converts a list of layer infos, presumably obtained from a Manifest.LayerInfos()
 // method call, into a format suitable for inclusion in a types.ImageInspectInfo structure.
 func layerInfosToStrings(infos []LayerInfo) []string {

manifest/common_test.go

@@ -1,6 +1,10 @@
 package manifest
 
 import (
+	"bytes"
+	"fmt"
+	"io/ioutil"
+	"path/filepath"
 	"testing"
 
 	"github.com/containers/image/v5/pkg/compression"
@@ -11,6 +15,86 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestValidateUnambiguousManifestFormat(t *testing.T) {
+	const allAllowedFields = allowedFieldFirstUnusedBit - 1
+	const mt = "text/plain" // Just some MIME type that shows up in error messages
+
+	type test struct {
+		manifest string
+		allowed  allowedManifestFields
+	}
+
+	// Smoke tests: Success
+	for _, c := range []test{
+		{"{}", allAllowedFields},
+		{"{}", 0},
+	} {
+		err := validateUnambiguousManifestFormat([]byte(c.manifest), mt, c.allowed)
+		assert.NoError(t, err, c)
+	}
+	// Smoke tests: Failure
+	for _, c := range []test{
+		{"{}", allowedFieldFirstUnusedBit}, // Invalid "allowed"
+		{"@", allAllowedFields},            // Invalid JSON
+	} {
+		err := validateUnambiguousManifestFormat([]byte(c.manifest), mt, c.allowed)
+		assert.Error(t, err, c)
+	}
+
+	fields := map[allowedManifestFields]string{
+		allowedFieldConfig:    "config",
+		allowedFieldFSLayers:  "fsLayers",
+		allowedFieldHistory:   "history",
+		allowedFieldLayers:    "layers",
+		allowedFieldManifests: "manifests",
+	}
+	// Ensure this test covers all defined allowedManifestFields values
+	allFields := allowedManifestFields(0)
+	for k := range fields {
+		allFields |= k
+	}
+	assert.Equal(t, allAllowedFields, allFields)
+
+	// Every single field is allowed by its bit, and rejected by any other bit
+	for bit, fieldName := range fields {
+		json := []byte(fmt.Sprintf(`{"%s":[]}`, fieldName))
+		err := validateUnambiguousManifestFormat(json, mt, bit)
+		assert.NoError(t, err, fieldName)
+		err = validateUnambiguousManifestFormat(json, mt, allAllowedFields^bit)
+		assert.Error(t, err, fieldName)
+	}
+}
+
+// Test that parser() rejects all of the provided manifest fixtures.
+// Intended to help test manifest parsers' detection of schema mismatches.
+func testManifestFixturesAreRejected(t *testing.T, parser func([]byte) error, fixtures []string) {
+	for _, fixture := range fixtures {
+		manifest, err := ioutil.ReadFile(filepath.Join("fixtures", fixture))
+		require.NoError(t, err, fixture)
+		err = parser(manifest)
+		assert.Error(t, err, fixture)
+	}
+}
+
+// Test that parser() rejects validManifest with an added top-level field with any of the provided field names.
+// Intended to help test callers of validateUnambiguousManifestFormat.
+func testValidManifestWithExtraFieldsIsRejected(t *testing.T, parser func([]byte) error,
+	validManifest []byte, fields []string) {
+	for _, field := range fields {
+		// end (the final '}') is not always at len(validManifest)-1 because the manifest can end with
+		// white space.
+		end := bytes.LastIndexByte(validManifest, '}')
+		require.NotEqual(t, end, -1)
+		updatedManifest := []byte(string(validManifest[:end]) +
+			fmt.Sprintf(`,"%s":[]}`, field))
+		err := parser(updatedManifest)
+		assert.Error(t, err, field)
+		// Make sure it is the error from validateUnambiguousManifestFormat, not something that
+		// went wrong with creating updatedManifest.
+		assert.Contains(t, err.Error(), "rejecting ambiguous manifest")
+	}
+}
+
 func TestLayerInfosToStrings(t *testing.T) {
 	strings := layerInfosToStrings([]LayerInfo{})
 	assert.Equal(t, []string{}, strings)

manifest/docker_schema1.go

@@ -60,6 +60,10 @@ func Schema1FromManifest(manifest []byte) (*Schema1, error) {
 	if s1.SchemaVersion != 1 {
 		return nil, errors.Errorf("unsupported schema version %d", s1.SchemaVersion)
 	}
+	if err := validateUnambiguousManifestFormat(manifest, DockerV2Schema1SignedMediaType,
+		allowedFieldFSLayers|allowedFieldHistory); err != nil {
+		return nil, err
+	}
 	if err := s1.initialize(); err != nil {
 		return nil, err
 	}

manifest/docker_schema1_test.go

@@ -20,6 +20,32 @@ func manifestSchema1FromFixture(t *testing.T, fixture string) *Schema1 {
 	return m
 }
 
+func TestSchema1FromManifest(t *testing.T) {
+	validManifest, err := ioutil.ReadFile(filepath.Join("fixtures", "schema2-to-schema1-by-docker.json"))
+	require.NoError(t, err)
+
+	// Invalid manifest version is rejected
+	m, err := Schema1FromManifest(validManifest)
+	require.NoError(t, err)
+	m.SchemaVersion = 2
+	manifest, err := m.Serialize()
+	require.NoError(t, err)
+	_, err = Schema1FromManifest(manifest)
+	assert.Error(t, err)
+
+	parser := func(m []byte) error {
+		_, err := Schema1FromManifest(m)
+		return err
+	}
+	// Schema mismatch is rejected
+	testManifestFixturesAreRejected(t, parser, []string{
+		"v2s2.manifest.json", "v2list.manifest.json",
+		"ociv1.manifest.json", "ociv1.image.index.json",
+	})
+	// Extra fields are rejected
+	testValidManifestWithExtraFieldsIsRejected(t, parser, validManifest, []string{"config", "layers", "manifests"})
+}
+
 func TestSchema1Initialize(t *testing.T) {
 	// Test this indirectly via Schema1FromComponents; otherwise we would have to break the API and create an instance manually.
 

manifest/docker_schema2.go

@@ -165,6 +165,10 @@ func Schema2FromManifest(manifest []byte) (*Schema2, error) {
 	if err := json.Unmarshal(manifest, &s2); err != nil {
 		return nil, err
 	}
+	if err := validateUnambiguousManifestFormat(manifest, DockerV2Schema2MediaType,
+		allowedFieldConfig|allowedFieldLayers); err != nil {
+		return nil, err
+	}
 	// Check manifest's and layers' media types.
 	if err := SupportedSchema2MediaType(s2.MediaType); err != nil {
 		return nil, err

manifest/docker_schema2_list.go

@@ -192,6 +192,10 @@ func Schema2ListFromManifest(manifest []byte) (*Schema2List, error) {
 	if err := json.Unmarshal(manifest, &list); err != nil {
 		return nil, errors.Wrapf(err, "unmarshaling Schema2List %q", string(manifest))
 	}
+	if err := validateUnambiguousManifestFormat(manifest, DockerV2ListMediaType,
+		allowedFieldManifests); err != nil {
+		return nil, err
+	}
 	return &list, nil
 }
 

manifest/docker_schema2_list_test.go

@@ -0,0 +1,28 @@
+package manifest
+
+import (
+	"io/ioutil"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestSchema2ListFromManifest(t *testing.T) {
+	validManifest, err := ioutil.ReadFile(filepath.Join("fixtures", "v2list.manifest.json"))
+	require.NoError(t, err)
+
+	parser := func(m []byte) error {
+		_, err := Schema2ListFromManifest(m)
+		return err
+	}
+	// Schema mismatch is rejected
+	testManifestFixturesAreRejected(t, parser, []string{
+		"schema2-to-schema1-by-docker.json",
+		"v2s2.manifest.json",
+		"ociv1.manifest.json",
+		// Not "ociv1.image.index.json" yet, without validating mediaType the two are too similar to tell the difference.
+	})
+	// Extra fields are rejected
+	testValidManifestWithExtraFieldsIsRejected(t, parser, validManifest, []string{"config", "fsLayers", "history", "layers"})
+}

manifest/docker_schema2_test.go

@@ -2,11 +2,13 @@ package manifest
 
 import (
 	"io/ioutil"
+	"path/filepath"
 	"testing"
 
 	"github.com/containers/image/v5/pkg/compression"
 	"github.com/containers/image/v5/types"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestSupportedSchema2MediaType(t *testing.T) {
@@ -58,6 +60,24 @@ func TestSupportedSchema2MediaType(t *testing.T) {
 	}
 }
 
+func TestSchema2FromManifest(t *testing.T) {
+	validManifest, err := ioutil.ReadFile(filepath.Join("fixtures", "v2s2.manifest.json"))
+	require.NoError(t, err)
+
+	parser := func(m []byte) error {
+		_, err := Schema2FromManifest(m)
+		return err
+	}
+	// Schema mismatch is rejected
+	testManifestFixturesAreRejected(t, parser, []string{
+		"schema2-to-schema1-by-docker.json",
+		"v2list.manifest.json",
+		"ociv1.manifest.json", "ociv1.image.index.json",
+	})
+	// Extra fields are rejected
+	testValidManifestWithExtraFieldsIsRejected(t, parser, validManifest, []string{"fsLayers", "history", "manifests"})
+}
+
 func TestUpdateLayerInfosV2S2GzipToZstd(t *testing.T) {
 	bytes, err := ioutil.ReadFile("fixtures/v2s2.manifest.json")
 	assert.Nil(t, err)

manifest/oci.go

@@ -54,6 +54,10 @@ func OCI1FromManifest(manifest []byte) (*OCI1, error) {
 	if err := json.Unmarshal(manifest, &oci1); err != nil {
 		return nil, err
 	}
+	if err := validateUnambiguousManifestFormat(manifest, imgspecv1.MediaTypeImageIndex,
+		allowedFieldConfig|allowedFieldLayers); err != nil {
+		return nil, err
+	}
 	return &oci1, nil
 }
 

manifest/oci_index.go

@@ -202,6 +202,10 @@ func OCI1IndexFromManifest(manifest []byte) (*OCI1Index, error) {
 	if err := json.Unmarshal(manifest, &index); err != nil {
 		return nil, errors.Wrapf(err, "unmarshaling OCI1Index %q", string(manifest))
 	}
+	if err := validateUnambiguousManifestFormat(manifest, imgspecv1.MediaTypeImageIndex,
+		allowedFieldManifests); err != nil {
+		return nil, err
+	}
 	return &index, nil
 }
 

manifest/oci_index_test.go

@@ -0,0 +1,28 @@
+package manifest
+
+import (
+	"io/ioutil"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestOCI1IndexFromManifest(t *testing.T) {
+	validManifest, err := ioutil.ReadFile(filepath.Join("fixtures", "ociv1.image.index.json"))
+	require.NoError(t, err)
+
+	parser := func(m []byte) error {
+		_, err := OCI1IndexFromManifest(m)
+		return err
+	}
+	// Schema mismatch is rejected
+	testManifestFixturesAreRejected(t, parser, []string{
+		"schema2-to-schema1-by-docker.json",
+		"v2s2.manifest.json",
+		// Not "v2list.manifest.json" yet, without mediaType the two are too similar to tell the difference.
+		"ociv1.manifest.json",
+	})
+	// Extra fields are rejected
+	testValidManifestWithExtraFieldsIsRejected(t, parser, validManifest, []string{"config", "fsLayers", "history", "layers"})
+}