Skip to content

Commit

Permalink
fix: verify PGP hash algorithms
Browse files Browse the repository at this point in the history
Explicitly verify the signature hash algorithm used when
signing/verifying OpenPGP signatures.
  • Loading branch information
tri-adam committed Sep 28, 2022
1 parent 5f9bee6 commit 2197285
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 22 deletions.
34 changes: 32 additions & 2 deletions pkg/integrity/clearsign.go
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -7,6 +7,7 @@ package integrity

import (
"bytes"
"crypto"
"encoding/json"
"errors"
"io"
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}

Expand Down
17 changes: 13 additions & 4 deletions pkg/integrity/clearsign_test.go
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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"
Expand All @@ -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},
Expand Down Expand Up @@ -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},
Expand All @@ -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 {
Expand Down
16 changes: 15 additions & 1 deletion pkg/integrity/sign_test.go
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -318,6 +318,20 @@ func TestGroupSigner_SignWithEntity(t *testing.T) {
},
e: e,
},
{
name: "SignatureConfigSHA224",
gs: groupSigner{
f: twoGroups,
id: 1,
ods: []sif.Descriptor{d1, d2},
mdHash: crypto.SHA1,
sigConfig: &packet.Config{
DefaultHash: crypto.SHA224,
Time: fixedTime,
},
},
e: e,
},
{
name: "SignatureConfigSHA256",
gs: groupSigner{
Expand Down
Binary file not shown.
15 changes: 0 additions & 15 deletions pkg/integrity/testdata/TestSignAndEncodeJSON/SHA1.golden

This file was deleted.

0 comments on commit 2197285

Please sign in to comment.