From 76624704c3727489b96b5ab0f0715eba3c9cd10b Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Mon, 14 Sep 2015 21:12:33 -0700 Subject: [PATCH] Correct unmarshal order for SignedManifest To ensure that we only unmarshal the verified payload into the contained manifest, we first copy the entire incoming buffer into Raw and then unmarshal only the Payload portion of the incoming bytes. If the contents is later verified, the caller can then be sure that the contents of the Manifest fields can be trusted. Signed-off-by: Aaron Lehmann --- manifest/schema1/manifest.go | 13 ++++-- registry/client/repository_test.go | 63 +++++++++++++++++------------- registry/handlers/api_test.go | 40 +++++++++++++++---- registry/handlers/images.go | 2 +- 4 files changed, 77 insertions(+), 41 deletions(-) diff --git a/manifest/schema1/manifest.go b/manifest/schema1/manifest.go index 4e8611fe..e7cbf958 100644 --- a/manifest/schema1/manifest.go +++ b/manifest/schema1/manifest.go @@ -62,15 +62,20 @@ type SignedManifest struct { // UnmarshalJSON populates a new ImageManifest struct from JSON data. func (sm *SignedManifest) UnmarshalJSON(b []byte) error { + sm.Raw = make([]byte, len(b), len(b)) + copy(sm.Raw, b) + + p, err := sm.Payload() + if err != nil { + return err + } + var manifest Manifest - if err := json.Unmarshal(b, &manifest); err != nil { + if err := json.Unmarshal(p, &manifest); err != nil { return err } sm.Manifest = manifest - sm.Raw = make([]byte, len(b), len(b)) - copy(sm.Raw, b) - return nil } diff --git a/registry/client/repository_test.go b/registry/client/repository_test.go index 6e4a017e..1e6eb25f 100644 --- a/registry/client/repository_test.go +++ b/registry/client/repository_test.go @@ -3,7 +3,6 @@ package client import ( "bytes" "crypto/rand" - "encoding/json" "fmt" "io" "log" @@ -14,8 +13,6 @@ import ( "testing" "time" - "github.com/docker/distribution/uuid" - "github.com/docker/distribution" "github.com/docker/distribution/context" "github.com/docker/distribution/digest" @@ -23,6 +20,8 @@ import ( "github.com/docker/distribution/manifest/schema1" "github.com/docker/distribution/registry/api/errcode" "github.com/docker/distribution/testutil" + "github.com/docker/distribution/uuid" + "github.com/docker/libtrust" ) func testServer(rrm testutil.RequestResponseMap) (string, func()) { @@ -420,7 +419,7 @@ func TestBlobUploadMonolithic(t *testing.T) { } } -func newRandomSchemaV1Manifest(name, tag string, blobCount int) (*schema1.SignedManifest, digest.Digest) { +func newRandomSchemaV1Manifest(name, tag string, blobCount int) (*schema1.SignedManifest, digest.Digest, []byte) { blobs := make([]schema1.FSLayer, blobCount) history := make([]schema1.History, blobCount) @@ -431,30 +430,38 @@ func newRandomSchemaV1Manifest(name, tag string, blobCount int) (*schema1.Signed history[i] = schema1.History{V1Compatibility: fmt.Sprintf("{\"Hex\": \"%x\"}", blob)} } - m := &schema1.SignedManifest{ - Manifest: schema1.Manifest{ - Name: name, - Tag: tag, - Architecture: "x86", - FSLayers: blobs, - History: history, - Versioned: manifest.Versioned{ - SchemaVersion: 1, - }, + m := schema1.Manifest{ + Name: name, + Tag: tag, + Architecture: "x86", + FSLayers: blobs, + History: history, + Versioned: manifest.Versioned{ + SchemaVersion: 1, }, } - manifestBytes, err := json.Marshal(m) - if err != nil { - panic(err) - } - dgst, err := digest.FromBytes(manifestBytes) + + pk, err := libtrust.GenerateECP256PrivateKey() if err != nil { panic(err) } - m.Raw = manifestBytes + sm, err := schema1.Sign(&m, pk) + if err != nil { + panic(err) + } - return m, dgst + p, err := sm.Payload() + if err != nil { + panic(err) + } + + dgst, err := digest.FromBytes(p) + if err != nil { + panic(err) + } + + return sm, dgst, p } func addTestManifestWithEtag(repo, reference string, content []byte, m *testutil.RequestResponseMap, dgst string) { @@ -551,7 +558,7 @@ func checkEqualManifest(m1, m2 *schema1.SignedManifest) error { func TestManifestFetch(t *testing.T) { ctx := context.Background() repo := "test.example.com/repo" - m1, dgst := newRandomSchemaV1Manifest(repo, "latest", 6) + m1, dgst, _ := newRandomSchemaV1Manifest(repo, "latest", 6) var m testutil.RequestResponseMap addTestManifest(repo, dgst.String(), m1.Raw, &m) @@ -586,9 +593,9 @@ func TestManifestFetch(t *testing.T) { func TestManifestFetchWithEtag(t *testing.T) { repo := "test.example.com/repo/by/tag" - m1, d1 := newRandomSchemaV1Manifest(repo, "latest", 6) + _, d1, p1 := newRandomSchemaV1Manifest(repo, "latest", 6) var m testutil.RequestResponseMap - addTestManifestWithEtag(repo, "latest", m1.Raw, &m, d1.String()) + addTestManifestWithEtag(repo, "latest", p1, &m, d1.String()) e, c := testServer(m) defer c() @@ -611,8 +618,8 @@ func TestManifestFetchWithEtag(t *testing.T) { func TestManifestDelete(t *testing.T) { repo := "test.example.com/repo/delete" - _, dgst1 := newRandomSchemaV1Manifest(repo, "latest", 6) - _, dgst2 := newRandomSchemaV1Manifest(repo, "latest", 6) + _, dgst1, _ := newRandomSchemaV1Manifest(repo, "latest", 6) + _, dgst2, _ := newRandomSchemaV1Manifest(repo, "latest", 6) var m testutil.RequestResponseMap m = append(m, testutil.RequestResponseMapping{ Request: testutil.Request{ @@ -651,7 +658,7 @@ func TestManifestDelete(t *testing.T) { func TestManifestPut(t *testing.T) { repo := "test.example.com/repo/delete" - m1, dgst := newRandomSchemaV1Manifest(repo, "other", 6) + m1, dgst, _ := newRandomSchemaV1Manifest(repo, "other", 6) var m testutil.RequestResponseMap m = append(m, testutil.RequestResponseMapping{ Request: testutil.Request{ @@ -744,7 +751,7 @@ func TestManifestTags(t *testing.T) { func TestManifestUnauthorized(t *testing.T) { repo := "test.example.com/repo" - _, dgst := newRandomSchemaV1Manifest(repo, "latest", 6) + _, dgst, _ := newRandomSchemaV1Manifest(repo, "latest", 6) var m testutil.RequestResponseMap m = append(m, testutil.RequestResponseMapping{ diff --git a/registry/handlers/api_test.go b/registry/handlers/api_test.go index 52a74a2b..adc7647d 100644 --- a/registry/handlers/api_test.go +++ b/registry/handlers/api_test.go @@ -760,14 +760,32 @@ func testManifestAPI(t *testing.T, env *testEnv, args manifestArgs) (*testEnv, m resp = putManifest(t, "putting unsigned manifest", manifestURL, unsignedManifest) defer resp.Body.Close() - checkResponse(t, "posting unsigned manifest", resp, http.StatusBadRequest) - _, p, counts := checkBodyHasErrorCodes(t, "getting unknown manifest tags", resp, - v2.ErrorCodeManifestUnverified, v2.ErrorCodeBlobUnknown, v2.ErrorCodeDigestInvalid) + checkResponse(t, "putting unsigned manifest", resp, http.StatusBadRequest) + _, p, counts := checkBodyHasErrorCodes(t, "getting unknown manifest tags", resp, v2.ErrorCodeManifestInvalid) expectedCounts := map[errcode.ErrorCode]int{ - v2.ErrorCodeManifestUnverified: 1, - v2.ErrorCodeBlobUnknown: 2, - v2.ErrorCodeDigestInvalid: 2, + v2.ErrorCodeManifestInvalid: 1, + } + + if !reflect.DeepEqual(counts, expectedCounts) { + t.Fatalf("unexpected number of error codes encountered: %v\n!=\n%v\n---\n%s", counts, expectedCounts, string(p)) + } + + // sign the manifest and still get some interesting errors. + sm, err := schema1.Sign(unsignedManifest, env.pk) + if err != nil { + t.Fatalf("error signing manifest: %v", err) + } + + resp = putManifest(t, "putting signed manifest with errors", manifestURL, sm) + defer resp.Body.Close() + checkResponse(t, "putting signed manifest with errors", resp, http.StatusBadRequest) + _, p, counts = checkBodyHasErrorCodes(t, "putting signed manifest with errors", resp, + v2.ErrorCodeManifestBlobUnknown, v2.ErrorCodeDigestInvalid) + + expectedCounts = map[errcode.ErrorCode]int{ + v2.ErrorCodeManifestBlobUnknown: 2, + v2.ErrorCodeDigestInvalid: 2, } if !reflect.DeepEqual(counts, expectedCounts) { @@ -1426,7 +1444,7 @@ func TestRegistryAsCacheMutationAPIs(t *testing.T) { } // Manifest upload - unsignedManifest := &schema1.Manifest{ + m := &schema1.Manifest{ Versioned: manifest.Versioned{ SchemaVersion: 1, }, @@ -1434,7 +1452,13 @@ func TestRegistryAsCacheMutationAPIs(t *testing.T) { Tag: tag, FSLayers: []schema1.FSLayer{}, } - resp := putManifest(t, "putting unsigned manifest", manifestURL, unsignedManifest) + + sm, err := schema1.Sign(m, env.pk) + if err != nil { + t.Fatalf("error signing manifest: %v", err) + } + + resp := putManifest(t, "putting unsigned manifest", manifestURL, sm) checkResponse(t, "putting signed manifest to cache", resp, errcode.ErrorCodeUnsupported.Descriptor().HTTPStatusCode) // Manifest Delete diff --git a/registry/handlers/images.go b/registry/handlers/images.go index e1931730..deb9cf49 100644 --- a/registry/handlers/images.go +++ b/registry/handlers/images.go @@ -163,7 +163,7 @@ func (imh *imageManifestHandler) PutImageManifest(w http.ResponseWriter, r *http for _, verificationError := range err { switch verificationError := verificationError.(type) { case distribution.ErrManifestBlobUnknown: - imh.Errors = append(imh.Errors, v2.ErrorCodeBlobUnknown.WithDetail(verificationError.Digest)) + imh.Errors = append(imh.Errors, v2.ErrorCodeManifestBlobUnknown.WithDetail(verificationError.Digest)) case distribution.ErrManifestUnverified: imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnverified) default: