diff --git a/manifest/common.go b/manifest/common.go index 4692211c0..511cdcc37 100644 --- a/manifest/common.go +++ b/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 { diff --git a/manifest/common_test.go b/manifest/common_test.go index 83f1c9e80..ace32a78d 100644 --- a/manifest/common_test.go +++ b/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) diff --git a/manifest/docker_schema1.go b/manifest/docker_schema1.go index 8679cad11..6d12c4cec 100644 --- a/manifest/docker_schema1.go +++ b/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 } diff --git a/manifest/docker_schema1_test.go b/manifest/docker_schema1_test.go index 1f211b821..72d570340 100644 --- a/manifest/docker_schema1_test.go +++ b/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. diff --git a/manifest/docker_schema2.go b/manifest/docker_schema2.go index 2711ca5eb..1f4db54ee 100644 --- a/manifest/docker_schema2.go +++ b/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 diff --git a/manifest/docker_schema2_list.go b/manifest/docker_schema2_list.go index 9ebb8d6b9..e97dfbd88 100644 --- a/manifest/docker_schema2_list.go +++ b/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 } diff --git a/manifest/docker_schema2_list_test.go b/manifest/docker_schema2_list_test.go new file mode 100644 index 000000000..40ffa8ddd --- /dev/null +++ b/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"}) +} diff --git a/manifest/docker_schema2_test.go b/manifest/docker_schema2_test.go index c643ba027..f6bc16b41 100644 --- a/manifest/docker_schema2_test.go +++ b/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) diff --git a/manifest/oci.go b/manifest/oci.go index 29a479c94..c4616b965 100644 --- a/manifest/oci.go +++ b/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 } diff --git a/manifest/oci_index.go b/manifest/oci_index.go index 5b4111e4e..5bec43ff9 100644 --- a/manifest/oci_index.go +++ b/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 } diff --git a/manifest/oci_index_test.go b/manifest/oci_index_test.go new file mode 100644 index 000000000..1461ee5b6 --- /dev/null +++ b/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"}) +} diff --git a/manifest/oci_test.go b/manifest/oci_test.go index 8eeef7738..cc1e71250 100644 --- a/manifest/oci_test.go +++ b/manifest/oci_test.go @@ -2,12 +2,14 @@ package manifest import ( "io/ioutil" + "path/filepath" "testing" "github.com/containers/image/v5/pkg/compression" "github.com/containers/image/v5/types" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestSupportedOCI1MediaType(t *testing.T) { @@ -71,6 +73,25 @@ func TestSupportedOCI1MediaType(t *testing.T) { } } +func TestOCI1FromManifest(t *testing.T) { + validManifest, err := ioutil.ReadFile(filepath.Join("fixtures", "ociv1.manifest.json")) + require.NoError(t, err) + + parser := func(m []byte) error { + _, err := OCI1FromManifest(m) + return err + } + // Schema mismatch is rejected + testManifestFixturesAreRejected(t, parser, []string{ + "schema2-to-schema1-by-docker.json", + // Not "v2s2.manifest.json" yet, without mediaType the two are too similar to tell the difference. + "v2list.manifest.json", + "ociv1.image.index.json", + }) + // Extra fields are rejected + testValidManifestWithExtraFieldsIsRejected(t, parser, validManifest, []string{"fsLayers", "history", "manifests"}) +} + func TestUpdateLayerInfosOCIGzipToZstd(t *testing.T) { bytes, err := ioutil.ReadFile("fixtures/ociv1.manifest.json") assert.Nil(t, err)