Skip to content

Commit

Permalink
Merge pull request #151 from DrDaveD/sylabs-GHSA-m5m3-46gj-wch8
Browse files Browse the repository at this point in the history 
Improve Digital Signature Hash Algorithm Validation (from sylabs GHSA-m5m3-46gj-wch8)
  • Loading branch information
@DrDaveD
DrDaveD authored Oct 6, 2022
2 parents 1d7be4f + 0b6868f commit 683739e
Show file tree
Hide file tree
Showing 28 changed files with 149 additions and 73 deletions.
34 changes: 32 additions & 2 deletions pkg/integrity/clearsign.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Apptainer a Series of LF Projects LLC.
// For website terms of use, trademark policy, privacy policy and other
// project policies see https://lfprojects.org/policies
// 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.
Expand All @@ -11,6 +11,7 @@ package integrity

import (
"bytes"
"crypto"
"encoding/json"
"errors"
"io"
Expand All @@ -22,9 +23,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 {
Expand Down Expand Up @@ -63,7 +87,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
}

Expand Down
17 changes: 13 additions & 4 deletions pkg/integrity/clearsign_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Apptainer a Series of LF Projects LLC.
// For website terms of use, trademark policy, privacy policy and other
// project policies see https://lfprojects.org/policies
// 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.
Expand All @@ -13,13 +13,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"
Expand All @@ -45,7 +47,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},
Expand Down Expand Up @@ -125,7 +127,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},
Expand All @@ -140,10 +142,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 {
Expand Down
20 changes: 11 additions & 9 deletions pkg/integrity/digest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,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
Expand All @@ -56,7 +58,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
}

Expand Down Expand Up @@ -108,7 +110,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
}
Expand All @@ -134,7 +136,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 {
Expand Down
64 changes: 47 additions & 17 deletions pkg/integrity/digest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,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",
Expand All @@ -147,6 +147,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 {
Expand Down Expand Up @@ -197,60 +207,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)
}
})
}
}
26 changes: 15 additions & 11 deletions pkg/integrity/metadata_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Apptainer a Series of LF Projects LLC.
// For website terms of use, trademark policy, privacy policy and other
// project policies see https://lfprojects.org/policies
// 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.
Expand Down Expand Up @@ -39,12 +39,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 {
Expand Down Expand Up @@ -92,13 +94,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 {
Expand Down Expand Up @@ -143,11 +147,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},
Expand Down
Loading

0 comments on commit 683739e

Please sign in to comment.