Merge pull request from GHSA-m5m3-46gj-wch8

Improve Digital Signature Hash Algorithm Validation
sylabs · Oct 6, 2022 · 07fb860 · 07fb860
2 parents 5f9bee6 + a854038
commit 07fb860
Show file tree

Hide file tree

Showing 28 changed files with 149 additions and 73 deletions.
diff --git a/pkg/integrity/clearsign.go b/pkg/integrity/clearsign.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2020, Sylabs Inc. All rights reserved.
+// Copyright (c) 2020-2022, Sylabs Inc. All rights reserved.
 // This software is licensed under a 3-clause BSD license. Please consult the LICENSE.md file
 // distributed with the sources of this project regarding your rights to use or distribute this
 // software.
@@ -7,6 +7,7 @@ package integrity
 
 import (
 	"bytes"
+	"crypto"
 	"encoding/json"
 	"errors"
 	"io"
@@ -18,9 +19,32 @@ import (
 
 var errClearsignedMsgNotFound = errors.New("clearsigned message not found")
 
+// Hash functions specified for OpenPGP in RFC4880, excluding those that are not currently
+// recommended by NIST.
+var supportedPGPAlgorithms = []crypto.Hash{
+	crypto.SHA224,
+	crypto.SHA256,
+	crypto.SHA384,
+	crypto.SHA512,
+}
+
+// hashAlgorithmSupported returns whether h is a supported PGP hash function.
+func hashAlgorithmSupported(h crypto.Hash) bool {
+	for _, alg := range supportedPGPAlgorithms {
+		if alg == h {
+			return true
+		}
+	}
+	return false
+}
+
 // signAndEncodeJSON encodes v, clear-signs it with privateKey, and writes it to w. If config is
 // nil, sensible defaults are used.
 func signAndEncodeJSON(w io.Writer, v interface{}, privateKey *packet.PrivateKey, config *packet.Config) error {
+	if !hashAlgorithmSupported(config.Hash()) {
+		return errHashUnsupported
+	}
+
 	// Get clearsign encoder.
 	plaintext, err := clearsign.Encode(w, privateKey, config)
 	if err != nil {
@@ -59,7 +83,13 @@ func verifyAndDecode(data []byte, kr openpgp.KeyRing) (*openpgp.Entity, []byte,
 	}
 
 	// Check signature.
-	e, err := openpgp.CheckDetachedSignature(kr, bytes.NewReader(b.Bytes), b.ArmoredSignature.Body, nil)
+	e, err := openpgp.CheckDetachedSignatureAndHash(
+		kr,
+		bytes.NewReader(b.Bytes),
+		b.ArmoredSignature.Body,
+		supportedPGPAlgorithms,
+		nil,
+	)
 	return e, b.Plaintext, rest, err
 }
 

diff --git a/pkg/integrity/clearsign_test.go b/pkg/integrity/clearsign_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2020-2021, Sylabs Inc. All rights reserved.
+// Copyright (c) 2020-2022, Sylabs Inc. All rights reserved.
 // This software is licensed under a 3-clause BSD license. Please consult the LICENSE.md file
 // distributed with the sources of this project regarding your rights to use or distribute this
 // software.
@@ -9,13 +9,15 @@ import (
 	"bufio"
 	"bytes"
 	"crypto"
+	"encoding/json"
 	"errors"
 	"io"
 	"reflect"
 	"strings"
 	"testing"
 
 	"github.com/ProtonMail/go-crypto/openpgp"
+	"github.com/ProtonMail/go-crypto/openpgp/clearsign"
 	pgperrors "github.com/ProtonMail/go-crypto/openpgp/errors"
 	"github.com/ProtonMail/go-crypto/openpgp/packet"
 	"github.com/sebdah/goldie/v2"
@@ -41,7 +43,7 @@ func TestSignAndEncodeJSON(t *testing.T) {
 	}{
 		{name: "EncryptedKey", key: &encryptedKey, wantErr: true},
 		{name: "DefaultHash", key: e.PrivateKey},
-		{name: "SHA1", key: e.PrivateKey, hash: crypto.SHA1},
+		{name: "SHA1", key: e.PrivateKey, hash: crypto.SHA1, wantErr: true},
 		{name: "SHA224", key: e.PrivateKey, hash: crypto.SHA224},
 		{name: "SHA256", key: e.PrivateKey, hash: crypto.SHA256},
 		{name: "SHA384", key: e.PrivateKey, hash: crypto.SHA384},
@@ -121,7 +123,7 @@ func TestVerifyAndDecodeJSON(t *testing.T) {
 		{name: "CorruptedSignature", el: openpgp.EntityList{e}, corrupter: corruptSignature},
 		{name: "VerifyOnly", el: openpgp.EntityList{e}, wantEntity: e},
 		{name: "DefaultHash", el: openpgp.EntityList{e}, output: &testType{}, wantEntity: e},
-		{name: "SHA1", hash: crypto.SHA1, el: openpgp.EntityList{e}, output: &testType{}, wantEntity: e},
+		{name: "SHA1", hash: crypto.SHA1, el: openpgp.EntityList{e}, wantErr: pgperrors.StructuralError("hash algorithm mismatch with cleartext message headers")}, //nolint:lll
 		{name: "SHA224", hash: crypto.SHA224, el: openpgp.EntityList{e}, output: &testType{}, wantEntity: e},
 		{name: "SHA256", hash: crypto.SHA256, el: openpgp.EntityList{e}, output: &testType{}, wantEntity: e},
 		{name: "SHA384", hash: crypto.SHA384, el: openpgp.EntityList{e}, output: &testType{}, wantEntity: e},
@@ -136,10 +138,17 @@ func TestVerifyAndDecodeJSON(t *testing.T) {
 			config := packet.Config{
 				DefaultHash: tt.hash,
 			}
-			err := signAndEncodeJSON(&b, testValue, e.PrivateKey, &config)
+
+			// Manually sign and encode rather than calling signAndEncodeJSON, since we want to
+			// test unsupported hash algorithms.
+			plaintext, err := clearsign.Encode(&b, e.PrivateKey, &config)
 			if err != nil {
 				t.Fatal(err)
 			}
+			if err := json.NewEncoder(plaintext).Encode(testValue); err != nil {
+				t.Fatal(err)
+			}
+			plaintext.Close()
 
 			// Introduce corruption, if applicable.
 			if tt.corrupter != nil {

diff --git a/pkg/integrity/digest.go b/pkg/integrity/digest.go
@@ -22,12 +22,14 @@ var (
 	errDigestMalformed = errors.New("digest malformed")
 )
 
-var supportedAlgorithms = map[crypto.Hash]string{
-	crypto.SHA1:   "sha1",
-	crypto.SHA224: "sha224",
-	crypto.SHA256: "sha256",
-	crypto.SHA384: "sha384",
-	crypto.SHA512: "sha512",
+// Hash functions supported for digests.
+var supportedDigestAlgorithms = map[crypto.Hash]string{
+	crypto.SHA224:     "sha224",
+	crypto.SHA256:     "sha256",
+	crypto.SHA384:     "sha384",
+	crypto.SHA512:     "sha512",
+	crypto.SHA512_224: "sha512_224",
+	crypto.SHA512_256: "sha512_256",
 }
 
 // hashValue calculates a digest by applying hash function h to the contents read from r. If h is
@@ -52,7 +54,7 @@ type digest struct {
 // newDigest returns a new digest. If h is not supported, errHashUnsupported is returned. If digest
 // is malformed, errDigestMalformed is returned.
 func newDigest(h crypto.Hash, value []byte) (digest, error) {
-	if _, ok := supportedAlgorithms[h]; !ok {
+	if _, ok := supportedDigestAlgorithms[h]; !ok {
 		return digest{}, errHashUnsupported
 	}
 
@@ -104,7 +106,7 @@ func (d digest) matches(r io.Reader) (bool, error) {
 
 // MarshalJSON marshals d into string of format "alg:value".
 func (d digest) MarshalJSON() ([]byte, error) {
-	n, ok := supportedAlgorithms[d.hash]
+	n, ok := supportedDigestAlgorithms[d.hash]
 	if !ok {
 		return nil, errHashUnsupported
 	}
@@ -130,7 +132,7 @@ func (d *digest) UnmarshalJSON(data []byte) error {
 		return fmt.Errorf("%w: %v", errDigestMalformed, err)
 	}
 
-	for h, n := range supportedAlgorithms {
+	for h, n := range supportedDigestAlgorithms {
 		if n == name {
 			digest, err := newDigest(h, v)
 			if err != nil {

diff --git a/pkg/integrity/digest_test.go b/pkg/integrity/digest_test.go
@@ -114,14 +114,14 @@ func TestDigest_MarshalJSON(t *testing.T) {
 		wantErr error
 	}{
 		{
-			name:    "UnsupportedHash",
+			name:    "HashUnsupportedMD5",
 			hash:    crypto.MD5,
 			wantErr: errHashUnsupported,
 		},
 		{
-			name:  "SHA1",
-			hash:  crypto.SHA1,
-			value: "597f6a540010f94c15d71806a99a2c8710e747bd",
+			name:    "HashUnsupportedSHA1",
+			hash:    crypto.SHA1,
+			wantErr: errHashUnsupported,
 		},
 		{
 			name:  "SHA224",
@@ -143,6 +143,16 @@ func TestDigest_MarshalJSON(t *testing.T) {
 			hash:  crypto.SHA512,
 			value: "db3974a97f2407b7cae1ae637c0030687a11913274d578492558e39c16c017de84eacdc8c62fe34ee4e12b4b1428817f09b6a2760c3f8a664ceae94d2434a593", //nolint:lll
 		},
+		{
+			name:  "SHA512_224",
+			hash:  crypto.SHA512_224,
+			value: "06001bf08dfb17d2b54925116823be230e98b5c6c278303bc4909a8c",
+		},
+		{
+			name:  "SHA512_256",
+			hash:  crypto.SHA512_256,
+			value: "3d37fe58435e0d87323dee4a2c1b339ef954de63716ee79f5747f94d974f913f",
+		},
 	}
 
 	for _, tt := range tests {
@@ -193,60 +203,80 @@ func TestDigest_UnmarshalJSON(t *testing.T) {
 			wantErr: errDigestMalformed,
 		},
 		{
-			name:    "UnsupportedHash",
+			name:    "HashUnsupportedMD5",
 			r:       strings.NewReader(`"md5:b0804ec967f48520697662a204f5fe72"`),
 			wantErr: errHashUnsupported,
 		},
+		{
+			name:    "HashUnsupportedSHA1",
+			r:       strings.NewReader(`"sha1:597f6a540010f94c15d71806a99a2c8710e747bd"`),
+			wantErr: errHashUnsupported,
+		},
 		{
 			name:    "DigestMalformedNotHex",
-			r:       strings.NewReader(`"sha1:oops"`),
+			r:       strings.NewReader(`"sha256:oops"`),
 			wantErr: errDigestMalformed,
 		},
 		{
 			name:    "DigestMalformedIncorrectLen",
-			r:       strings.NewReader(`"sha1:597f"`),
+			r:       strings.NewReader(`"sha256:597f"`),
 			wantErr: errDigestMalformed,
 		},
-		{
-			name:      "SHA1",
-			r:         strings.NewReader(`"sha1:597f6a540010f94c15d71806a99a2c8710e747bd"`),
-			wantHash:  crypto.SHA1,
-			wantValue: "597f6a540010f94c15d71806a99a2c8710e747bd",
-		},
 		{
 			name:      "SHA224",
 			r:         strings.NewReader(`"sha224:95041dd60ab08c0bf5636d50be85fe9790300f39eb84602858a9b430"`),
-			wantHash:  crypto.SHA1,
+			wantHash:  crypto.SHA224,
 			wantValue: "95041dd60ab08c0bf5636d50be85fe9790300f39eb84602858a9b430",
 		},
 		{
 			name:      "SHA256",
 			r:         strings.NewReader(`"sha256:a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447"`),
-			wantHash:  crypto.SHA1,
+			wantHash:  crypto.SHA256,
 			wantValue: "a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447",
 		},
 		{
 			name:      "SHA384",
 			r:         strings.NewReader(`"sha384:6b3b69ff0a404f28d75e98a066d3fc64fffd9940870cc68bece28545b9a75086b343d7a1366838083e4b8f3ca6fd3c80"`), //nolint:lll
-			wantHash:  crypto.SHA1,
+			wantHash:  crypto.SHA384,
 			wantValue: "6b3b69ff0a404f28d75e98a066d3fc64fffd9940870cc68bece28545b9a75086b343d7a1366838083e4b8f3ca6fd3c80",
 		},
 		{
 			name:      "SHA512",
 			r:         strings.NewReader(`"sha512:db3974a97f2407b7cae1ae637c0030687a11913274d578492558e39c16c017de84eacdc8c62fe34ee4e12b4b1428817f09b6a2760c3f8a664ceae94d2434a593"`), //nolint:lll
-			wantHash:  crypto.SHA1,
+			wantHash:  crypto.SHA512,
 			wantValue: "db3974a97f2407b7cae1ae637c0030687a11913274d578492558e39c16c017de84eacdc8c62fe34ee4e12b4b1428817f09b6a2760c3f8a664ceae94d2434a593", //nolint:lll
 		},
+		{
+			name:      "SHA512_224",
+			r:         strings.NewReader(`"sha512_224:06001bf08dfb17d2b54925116823be230e98b5c6c278303bc4909a8c"`),
+			wantHash:  crypto.SHA512_224,
+			wantValue: "06001bf08dfb17d2b54925116823be230e98b5c6c278303bc4909a8c",
+		},
+		{
+			name:      "SHA512_256",
+			r:         strings.NewReader(`"sha512_256:3d37fe58435e0d87323dee4a2c1b339ef954de63716ee79f5747f94d974f913f"`),
+			wantHash:  crypto.SHA512_256,
+			wantValue: "3d37fe58435e0d87323dee4a2c1b339ef954de63716ee79f5747f94d974f913f",
+		},
 	}
 
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			var d digest
+
 			err := json.NewDecoder(tt.r).Decode(&d)
 			if got, want := err, tt.wantErr; !errors.Is(got, want) {
 				t.Fatalf("got error %v, want %v", got, want)
 			}
+
+			if got, want := d.hash, tt.wantHash; got != want {
+				t.Errorf("got hash %v, want %v", got, want)
+			}
+
+			if got, want := hex.EncodeToString(d.value), tt.wantValue; got != want {
+				t.Errorf("got value %v, want %v", got, want)
+			}
 		})
 	}
 }
diff --git a/pkg/integrity/metadata_test.go b/pkg/integrity/metadata_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2020-2021, Sylabs Inc. All rights reserved.
+// Copyright (c) 2020-2022, Sylabs Inc. All rights reserved.
 // This software is licensed under a 3-clause BSD license. Please consult the LICENSE.md file
 // distributed with the sources of this project regarding your rights to use or distribute this
 // software.
@@ -35,12 +35,14 @@ func TestGetHeaderMetadata(t *testing.T) {
 		wantErr error
 	}{
 		{name: "HashUnavailable", header: bytes.NewReader(b), hash: crypto.MD4, wantErr: errHashUnavailable},
-		{name: "HashUnsupported", header: bytes.NewReader(b), hash: crypto.MD5, wantErr: errHashUnsupported},
-		{name: "SHA1", header: bytes.NewReader(b), hash: crypto.SHA1},
+		{name: "HashUnsupportedMD5", header: bytes.NewReader(b), hash: crypto.MD5, wantErr: errHashUnsupported},
+		{name: "HashUnsupportedSHA1", header: bytes.NewReader(b), hash: crypto.SHA1, wantErr: errHashUnsupported},
 		{name: "SHA224", header: bytes.NewReader(b), hash: crypto.SHA224},
 		{name: "SHA256", header: bytes.NewReader(b), hash: crypto.SHA256},
 		{name: "SHA384", header: bytes.NewReader(b), hash: crypto.SHA384},
 		{name: "SHA512", header: bytes.NewReader(b), hash: crypto.SHA512},
+		{name: "SHA512_224", header: bytes.NewReader(b), hash: crypto.SHA512_224},
+		{name: "SHA512_256", header: bytes.NewReader(b), hash: crypto.SHA512_256},
 	}
 
 	for _, tt := range tests {
@@ -88,13 +90,15 @@ func TestGetObjectMetadata(t *testing.T) {
 		wantErr    error
 	}{
 		{name: "HashUnavailable", descr: bytes.NewReader(rid0), hash: crypto.MD4, wantErr: errHashUnavailable},
-		{name: "HashUnsupported", descr: bytes.NewReader(rid0), hash: crypto.MD5, wantErr: errHashUnsupported},
-		{name: "RelativeID", relativeID: 1, descr: bytes.NewReader(rid1), data: strings.NewReader("blah"), hash: crypto.SHA1},
-		{name: "SHA1", descr: bytes.NewReader(rid0), data: strings.NewReader("blah"), hash: crypto.SHA1},
+		{name: "HashUnsupportedMD5", descr: bytes.NewReader(rid0), hash: crypto.MD5, wantErr: errHashUnsupported},
+		{name: "HashUnsupportedSHA1", descr: bytes.NewReader(rid0), hash: crypto.SHA1, wantErr: errHashUnsupported},
+		{name: "RelativeID", relativeID: 1, descr: bytes.NewReader(rid1), data: strings.NewReader("blah"), hash: crypto.SHA256}, //nolint:lll
 		{name: "SHA224", descr: bytes.NewReader(rid0), data: strings.NewReader("blah"), hash: crypto.SHA224},
 		{name: "SHA256", descr: bytes.NewReader(rid0), data: strings.NewReader("blah"), hash: crypto.SHA256},
 		{name: "SHA384", descr: bytes.NewReader(rid0), data: strings.NewReader("blah"), hash: crypto.SHA384},
 		{name: "SHA512", descr: bytes.NewReader(rid0), data: strings.NewReader("blah"), hash: crypto.SHA512},
+		{name: "SHA512_224", descr: bytes.NewReader(rid0), data: strings.NewReader("blah"), hash: crypto.SHA512_224},
+		{name: "SHA512_256", descr: bytes.NewReader(rid0), data: strings.NewReader("blah"), hash: crypto.SHA512_256},
 	}
 
 	for _, tt := range tests {
@@ -139,11 +143,11 @@ func TestGetImageMetadata(t *testing.T) {
 		wantErr error
 	}{
 		{name: "HashUnavailable", hash: crypto.MD4, wantErr: errHashUnavailable},
-		{name: "HashUnsupported", hash: crypto.MD5, wantErr: errHashUnsupported},
-		{name: "MinimumIDInvalid", minID: 2, ods: []sif.Descriptor{od1}, hash: crypto.SHA1, wantErr: errMinimumIDInvalid},
-		{name: "Object1", minID: 1, ods: []sif.Descriptor{od1}, hash: crypto.SHA1},
-		{name: "Object2", minID: 1, ods: []sif.Descriptor{od2}, hash: crypto.SHA1},
-		{name: "SHA1", minID: 1, ods: []sif.Descriptor{od1, od2}, hash: crypto.SHA1},
+		{name: "HashUnsupportedMD5", hash: crypto.MD5, wantErr: errHashUnsupported},
+		{name: "HashUnsupportedSHA1", hash: crypto.SHA1, wantErr: errHashUnsupported},
+		{name: "MinimumIDInvalid", minID: 2, ods: []sif.Descriptor{od1}, hash: crypto.SHA256, wantErr: errMinimumIDInvalid},
+		{name: "Object1", minID: 1, ods: []sif.Descriptor{od1}, hash: crypto.SHA256},
+		{name: "Object2", minID: 1, ods: []sif.Descriptor{od2}, hash: crypto.SHA256},
 		{name: "SHA224", minID: 1, ods: []sif.Descriptor{od1, od2}, hash: crypto.SHA224},
 		{name: "SHA256", minID: 1, ods: []sif.Descriptor{od1, od2}, hash: crypto.SHA256},
 		{name: "SHA384", minID: 1, ods: []sif.Descriptor{od1, od2}, hash: crypto.SHA384},