Merge pull request #1409 from mtrmac/ambiguous-1

Reject ambiguous manifest formats

Author: mtrmac

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
 }