Skip to content

Commit

Permalink
[v2] Check signature details of binding signatures (#218)
Browse files Browse the repository at this point in the history 
Check the hash algorithm, creation time, signature notations, and
signature expiry (when relevant) of binding signatures when using
keys.

To be able to check that all critical signature notations are known,
and the hash algorithm used is valid, we add `config` parameters to
all functions on the path to verifying key binding signatures in v2.

By default, we reject binding signatures using MD5 and RIPEMD-160, but
this can be modified by setting the new `config.RejectHashAlgorithms`
property. In the future, we should also reject binding signatures
using SHA-1, but this would be a larger breaking change.

Co-authored-by: Lukas Burkhalter <[email protected]>
  • Loading branch information
@twiss @lubux
twiss and lubux committed Jul 18, 2024
1 parent c556c9c commit ad60d74
Show file tree
Hide file tree
Showing 10 changed files with 147 additions and 83 deletions.
2 changes: 1 addition & 1 deletion .github/test-suite/build_gosop.sh
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
cd gosop
echo "replace github.com/ProtonMail/go-crypto => ../go-crypto" >> go.mod
go get github.com/ProtonMail/go-crypto
go get github.com/ProtonMail/gopenpgp/v3/crypto@80762a9ce60ba09d8a0d4f7b2a9a9665e7716351
go get github.com/ProtonMail/gopenpgp/v3/crypto@db2db2c2cd366696183a2d3cf6fea63eb679e54c
go build .
16 changes: 16 additions & 0 deletions openpgp/packet/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ var (
PubKeyAlgoElGamal: true,
PubKeyAlgoDSA: true,
}
defaultRejectHashAlgorithms = map[crypto.Hash]bool{
crypto.MD5: true,
crypto.RIPEMD160: true,
}
defaultRejectMessageHashAlgorithms = map[crypto.Hash]bool{
crypto.SHA1: true,
crypto.MD5: true,
Expand Down Expand Up @@ -104,6 +108,7 @@ type Config struct {
MinRSABits uint16
// Reject insecure algorithms, only works with v2 api
RejectPublicKeyAlgorithms map[PublicKeyAlgorithm]bool
RejectHashAlgorithms map[crypto.Hash]bool
RejectMessageHashAlgorithms map[crypto.Hash]bool
RejectCurves map[Curve]bool
// "The validity period of the key. This is the number of seconds after
Expand Down Expand Up @@ -339,6 +344,17 @@ func (c *Config) RejectPublicKeyAlgorithm(alg PublicKeyAlgorithm) bool {
return rejectedAlgorithms[alg]
}

func (c *Config) RejectHashAlgorithm(hash crypto.Hash) bool {
var rejectedAlgorithms map[crypto.Hash]bool
if c == nil || c.RejectHashAlgorithms == nil {
// Default
rejectedAlgorithms = defaultRejectHashAlgorithms
} else {
rejectedAlgorithms = c.RejectHashAlgorithms
}
return rejectedAlgorithms[hash]
}

func (c *Config) RejectMessageHashAlgorithm(hash crypto.Hash) bool {
var rejectedAlgorithms map[crypto.Hash]bool
if c == nil || c.RejectMessageHashAlgorithms == nil {
Expand Down
8 changes: 4 additions & 4 deletions openpgp/read.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) {
key := scr.md.SignedBy
signatureError := key.PublicKey.VerifySignature(scr.h, sig)
if signatureError == nil {
signatureError = checkSignatureDetails(key, sig, scr.config)
signatureError = checkMessageSignatureDetails(key, sig, scr.config)
}
scr.md.Signature = sig
scr.md.SignatureError = signatureError
Expand Down Expand Up @@ -546,7 +546,7 @@ func verifyDetachedSignature(keyring KeyRing, signed, signature io.Reader, expec
for _, key := range keys {
err = key.PublicKey.VerifySignature(h, sig)
if err == nil {
return sig, key.Entity, checkSignatureDetails(&key, sig, config)
return sig, key.Entity, checkMessageSignatureDetails(&key, sig, config)
}
}

Expand All @@ -564,7 +564,7 @@ func CheckArmoredDetachedSignature(keyring KeyRing, signed, signature io.Reader,
return CheckDetachedSignature(keyring, signed, body, config)
}

// checkSignatureDetails returns an error if:
// checkMessageSignatureDetails returns an error if:
// - The signature (or one of the binding signatures mentioned below)
// has a unknown critical notation data subpacket
// - The primary key of the signing entity is revoked
Expand All @@ -582,7 +582,7 @@ func CheckArmoredDetachedSignature(keyring KeyRing, signed, signature io.Reader,
// NOTE: The order of these checks is important, as the caller may choose to
// ignore ErrSignatureExpired or ErrKeyExpired errors, but should never
// ignore any other errors.
func checkSignatureDetails(key *Key, signature *packet.Signature, config *packet.Config) error {
func checkMessageSignatureDetails(key *Key, signature *packet.Signature, config *packet.Config) error {
now := config.Now()
primarySelfSignature, primaryIdentity := key.Entity.PrimarySelfSignature()
signedBySubKey := key.PublicKey != key.Entity.PrimaryKey
Expand Down
36 changes: 19 additions & 17 deletions openpgp/v2/keys.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,11 @@ type KeyRing interface {
// PrimaryIdentity returns a valid non-revoked Identity while preferring
// identities marked as primary, or the latest-created identity, in that order.
// Returns an nil for both return values if there is no valid primary identity.
func (e *Entity) PrimaryIdentity(date time.Time) (*packet.Signature, *Identity) {
func (e *Entity) PrimaryIdentity(date time.Time, config *packet.Config) (*packet.Signature, *Identity) {
var primaryIdentityCandidates []*Identity
var primaryIdentityCandidatesSelfSigs []*packet.Signature
for _, identity := range e.Identities {
selfSig, err := identity.Verify(date) // identity must be valid at date
selfSig, err := identity.Verify(date, config) // identity must be valid at date
if err == nil { // verification is successful
primaryIdentityCandidates = append(primaryIdentityCandidates, identity)
primaryIdentityCandidatesSelfSigs = append(primaryIdentityCandidatesSelfSigs, selfSig)
Expand Down Expand Up @@ -98,7 +98,7 @@ func shouldPreferIdentity(existingId, potentialNewId *packet.Signature) bool {
// given Entity.
func (e *Entity) EncryptionKey(now time.Time, config *packet.Config) (Key, bool) {
// The primary key has to be valid at time now
primarySelfSignature, err := e.VerifyPrimaryKey(now)
primarySelfSignature, err := e.VerifyPrimaryKey(now, config)
if err != nil { // primary key is not valid
return Key{}, false
}
Expand All @@ -113,7 +113,7 @@ func (e *Entity) EncryptionKey(now time.Time, config *packet.Config) (Key, bool)
var maxTime time.Time
var selectedSubkeySelfSig *packet.Signature
for i, subkey := range e.Subkeys {
subkeySelfSig, err := subkey.Verify(now) // subkey has to be valid at time now
subkeySelfSig, err := subkey.Verify(now, config) // subkey has to be valid at time now
if err == nil &&
isValidEncryptionKey(subkeySelfSig, subkey.PublicKey.PubKeyAlgo) &&
checkKeyRequirements(subkey.PublicKey, config) == nil &&
Expand Down Expand Up @@ -155,13 +155,13 @@ func (e *Entity) EncryptionKey(now time.Time, config *packet.Config) (Key, bool)
// which should be proffered to decrypt older messages.
// If id is 0 all decryption keys are returned.
// This is useful to retrieve keys for session key decryption.
func (e *Entity) DecryptionKeys(id uint64, date time.Time) (keys []Key) {
primarySelfSignature, err := e.PrimarySelfSignature(date)
func (e *Entity) DecryptionKeys(id uint64, date time.Time, config *packet.Config) (keys []Key) {
primarySelfSignature, err := e.PrimarySelfSignature(date, config)
if err != nil { // primary key is not valid
return
}
for _, subkey := range e.Subkeys {
subkeySelfSig, err := subkey.LatestValidBindingSignature(date)
subkeySelfSig, err := subkey.LatestValidBindingSignature(date, config)
if err == nil &&
isValidEncryptionKey(subkeySelfSig, subkey.PublicKey.PubKeyAlgo) &&
(id == 0 || subkey.PublicKey.KeyId == id) {
Expand Down Expand Up @@ -201,7 +201,7 @@ func (e *Entity) SigningKeyById(now time.Time, id uint64, config *packet.Config)
}

func (e *Entity) signingKeyByIdUsage(now time.Time, id uint64, flags int, config *packet.Config) (Key, error) {
primarySelfSignature, err := e.VerifyPrimaryKey(now)
primarySelfSignature, err := e.VerifyPrimaryKey(now, config)
if err != nil {
return Key{}, err
}
Expand All @@ -216,7 +216,7 @@ func (e *Entity) signingKeyByIdUsage(now time.Time, id uint64, flags int, config
var maxTime time.Time
var selectedSubkeySelfSig *packet.Signature
for idx, subkey := range e.Subkeys {
subkeySelfSig, err := subkey.Verify(now)
subkeySelfSig, err := subkey.Verify(now, config)
if err == nil &&
(flags&packet.KeyFlagCertify == 0 || isValidCertificationKey(subkeySelfSig, subkey.PublicKey.PubKeyAlgo)) &&
(flags&packet.KeyFlagSign == 0 || isValidSigningKey(subkeySelfSig, subkey.PublicKey.PubKeyAlgo)) &&
Expand Down Expand Up @@ -667,13 +667,16 @@ func (e *Entity) SignIdentity(identity string, signer *Entity, config *packet.Co
}

// LatestValidDirectSignature returns the latest valid direct key-signature of the entity.
func (e *Entity) LatestValidDirectSignature(date time.Time) (selectedSig *packet.Signature, err error) {
func (e *Entity) LatestValidDirectSignature(date time.Time, config *packet.Config) (selectedSig *packet.Signature, err error) {
for sigIdx := len(e.DirectSignatures) - 1; sigIdx >= 0; sigIdx-- {
sig := e.DirectSignatures[sigIdx]
if (date.IsZero() || date.Unix() >= sig.Packet.CreationTime.Unix()) &&
(selectedSig == nil || selectedSig.CreationTime.Unix() < sig.Packet.CreationTime.Unix()) {
if sig.Valid == nil {
err := e.PrimaryKey.VerifyDirectKeySignature(sig.Packet)
if err == nil {
err = checkSignatureDetails(e.PrimaryKey, sig.Packet, date, config)
}
valid := err == nil
sig.Valid = &valid
}
Expand All @@ -693,12 +696,11 @@ func (e *Entity) LatestValidDirectSignature(date time.Time) (selectedSig *packet
// For V6 keys, returns the latest valid direct-key self-signature, and no identity (nil).
// This self-signature is to be used to check the key expiration,
// algorithm preferences, and so on.
func (e *Entity) PrimarySelfSignature(date time.Time) (primarySig *packet.Signature, err error) {
func (e *Entity) PrimarySelfSignature(date time.Time, config *packet.Config) (primarySig *packet.Signature, err error) {
if e.PrimaryKey.Version == 6 {
primarySig, err = e.LatestValidDirectSignature(date)
return
return e.LatestValidDirectSignature(date, config)
}
primarySig, _ = e.PrimaryIdentity(date)
primarySig, _ = e.PrimaryIdentity(date, config)
if primarySig == nil {
return nil, errors.StructuralError("no primary identity found")
}
Expand All @@ -710,8 +712,8 @@ func (e *Entity) PrimarySelfSignature(date time.Time) (primarySig *packet.Signat
// - that there is valid non-expired self-signature,
// - that the primary key is not expired given its self-signature.
// If date is zero (i.e., date.IsZero() == true) the time checks are not performed.
func (e *Entity) VerifyPrimaryKey(date time.Time) (*packet.Signature, error) {
primarySelfSignature, err := e.PrimarySelfSignature(date)
func (e *Entity) VerifyPrimaryKey(date time.Time, config *packet.Config) (*packet.Signature, error) {
primarySelfSignature, err := e.PrimarySelfSignature(date, config)
if err != nil {
return nil, goerrors.New("no valid self signature found")
}
Expand All @@ -727,7 +729,7 @@ func (e *Entity) VerifyPrimaryKey(date time.Time) (*packet.Signature, error) {

if e.PrimaryKey.Version != 6 && len(e.DirectSignatures) > 0 {
// check for expiration time in direct signatures (for V6 keys, the above already did so)
primaryDirectKeySignature, _ := e.LatestValidDirectSignature(date)
primaryDirectKeySignature, _ := e.LatestValidDirectSignature(date, config)
if primaryDirectKeySignature != nil &&
(!date.IsZero() && e.PrimaryKey.KeyExpired(primaryDirectKeySignature, date)) {
return primarySelfSignature, errors.ErrKeyExpired
Expand Down
47 changes: 26 additions & 21 deletions openpgp/v2/keys_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ func TestMissingCrossSignature(t *testing.T) {
// contain a cross-signature.
keys, _ := ReadArmoredKeyRing(bytes.NewBufferString(missingCrossSignatureKey))
var config *packet.Config
_, err := keys[0].Subkeys[0].Verify(config.Now())
_, err := keys[0].Subkeys[0].Verify(config.Now(), config)
if err == nil {
t.Fatal("Failed to detect error in keyring with missing cross signature")
}
Expand All @@ -282,7 +282,7 @@ func TestInvalidCrossSignature(t *testing.T) {
// not correctly validate over the primary and subkey.
keys, _ := ReadArmoredKeyRing(bytes.NewBufferString(invalidCrossSignatureKey))
var config *packet.Config
_, err := keys[0].Subkeys[0].Verify(config.Now())
_, err := keys[0].Subkeys[0].Verify(config.Now(), config)
if err == nil {
t.Fatal("Failed to detect error in keyring with an invalid cross signature")
}
Expand Down Expand Up @@ -341,11 +341,11 @@ func TestRevokedUserID(t *testing.T) {
t.Errorf("missing second identity")
}

if firstIdentity.Revoked(nil, time.Now()) {
if firstIdentity.Revoked(nil, time.Now(), &packet.Config{}) {
t.Errorf("expected first identity not to be revoked")
}

if !secondIdentity.Revoked(nil, time.Now()) {
if !secondIdentity.Revoked(nil, time.Now(), &packet.Config{}) {
t.Errorf("expected second identity to be revoked")
}

Expand Down Expand Up @@ -390,11 +390,11 @@ func TestFirstUserIDRevoked(t *testing.T) {
t.Errorf("missing second identity")
}

if !firstIdentity.Revoked(nil, time.Now()) {
if !firstIdentity.Revoked(nil, time.Now(), &packet.Config{}) {
t.Errorf("expected first identity to be revoked")
}

if secondIdentity.Revoked(nil, time.Now()) {
if secondIdentity.Revoked(nil, time.Now(), &packet.Config{}) {
t.Errorf("expected second identity not to be revoked")
}

Expand Down Expand Up @@ -432,7 +432,7 @@ func TestOnlyUserIDRevoked(t *testing.T) {
t.Errorf("missing identity")
}

if !identity.Revoked(nil, time.Now()) {
if !identity.Revoked(nil, time.Now(), &packet.Config{}) {
t.Errorf("expected identity to be revoked")
}

Expand Down Expand Up @@ -675,7 +675,8 @@ func TestKeyWithSubKeyAndBadSelfSigOrder(t *testing.T) {

subKey := key.Subkeys[0]
var zeroTime time.Time
selfSig, err := subKey.LatestValidBindingSignature(zeroTime)
var config *packet.Config
selfSig, err := subKey.LatestValidBindingSignature(zeroTime, config)
if err != nil {
t.Fatal("expected a self signature to be found")
}
Expand Down Expand Up @@ -759,7 +760,7 @@ func TestNewEntityWithDefaultHash(t *testing.T) {

for _, identity := range entity.Identities {
var zeroTime time.Time
selfSig, err := identity.LatestValidSelfCertification(zeroTime)
selfSig, err := identity.LatestValidSelfCertification(zeroTime, c)
if err != nil {
t.Fatal("expected a self signature to be found ")
}
Expand All @@ -783,7 +784,7 @@ func TestNewEntityNilConfigPreferredHash(t *testing.T) {

for _, identity := range entity.Identities {
var zeroTime time.Time
selfSig, err := identity.LatestValidSelfCertification(zeroTime)
selfSig, err := identity.LatestValidSelfCertification(zeroTime, &packet.Config{})
if err != nil {
t.Fatal("expected a self signature to be found ")
}
Expand Down Expand Up @@ -824,7 +825,7 @@ func TestNewEntityWithDefaultCipher(t *testing.T) {

for _, identity := range entity.Identities {
var zeroTime time.Time
selfSig, err := identity.LatestValidSelfCertification(zeroTime)
selfSig, err := identity.LatestValidSelfCertification(zeroTime, c)
if err != nil {
t.Fatal("expected a self signature to be found ")
}
Expand All @@ -847,7 +848,7 @@ func TestNewEntityNilConfigPreferredSymmetric(t *testing.T) {

for _, identity := range entity.Identities {
var zeroTime time.Time
selfSig, err := identity.LatestValidSelfCertification(zeroTime)
selfSig, err := identity.LatestValidSelfCertification(zeroTime, &packet.Config{})
if err != nil {
t.Fatal("expected a self signature to be found ")
}
Expand All @@ -872,7 +873,7 @@ func TestNewEntityWithDefaultAead(t *testing.T) {

for _, identity := range entity.Identities {
var zeroTime time.Time
selfSig, err := identity.LatestValidSelfCertification(zeroTime)
selfSig, err := identity.LatestValidSelfCertification(zeroTime, cfg)
if err != nil {
t.Fatal("expected a self signature to be found ")
}
Expand Down Expand Up @@ -1042,7 +1043,7 @@ func TestAddUserId(t *testing.T) {

for _, sk := range entity.Identities {
var zeroTime time.Time
selfSig, err := sk.LatestValidSelfCertification(zeroTime)
selfSig, err := sk.LatestValidSelfCertification(zeroTime, &packet.Config{})
if err != nil {
t.Fatal("expected a self signature to be found")
}
Expand Down Expand Up @@ -1084,7 +1085,8 @@ func TestAddSubkey(t *testing.T) {

for _, sk := range entity.Subkeys {
var zeroTime time.Time
selfSig, err := sk.LatestValidBindingSignature(zeroTime)
var config *packet.Config
selfSig, err := sk.LatestValidBindingSignature(zeroTime, config)
if err != nil {
t.Fatal("expected a self signature to be found")
}
Expand Down Expand Up @@ -1137,7 +1139,8 @@ func TestAddSubkeySerialized(t *testing.T) {

for _, sk := range entity.Subkeys {
var zeroTime time.Time
selfSig, err := sk.LatestValidBindingSignature(zeroTime)
var config *packet.Config
selfSig, err := sk.LatestValidBindingSignature(zeroTime, config)
if err != nil {
t.Fatal("expected a self signature to be found")
}
Expand Down Expand Up @@ -1183,7 +1186,8 @@ func TestAddSubkeyWithConfig(t *testing.T) {
}

var zeroTime time.Time
selfSig1, err := entity.Subkeys[1].LatestValidBindingSignature(zeroTime)
var config *packet.Config
selfSig1, err := entity.Subkeys[1].LatestValidBindingSignature(zeroTime, config)
if err != nil {
t.Fatal("expected a self signature to be found")
}
Expand All @@ -1200,7 +1204,7 @@ func TestAddSubkeyWithConfig(t *testing.T) {
t.Errorf("Invalid subkey signature: %v", err)
}

selfSig2, err := entity.Subkeys[2].LatestValidBindingSignature(zeroTime)
selfSig2, err := entity.Subkeys[2].LatestValidBindingSignature(zeroTime, config)
if err != nil {
t.Fatal("expected a self signature to be found")
}
Expand Down Expand Up @@ -1259,7 +1263,8 @@ func TestAddSubkeyWithConfigSerialized(t *testing.T) {
}

var zeroTime time.Time
selfSig1, err := entity.Subkeys[1].LatestValidBindingSignature(zeroTime)
var config *packet.Config
selfSig1, err := entity.Subkeys[1].LatestValidBindingSignature(zeroTime, config)
if err != nil {
t.Fatal("expected a self signature to be found")
}
Expand All @@ -1283,7 +1288,7 @@ func TestAddSubkeyWithConfigSerialized(t *testing.T) {
selfSig1.EmbeddedSignature.Hash)
}

selfSig2, err := entity.Subkeys[2].LatestValidBindingSignature(zeroTime)
selfSig2, err := entity.Subkeys[2].LatestValidBindingSignature(zeroTime, config)
if err != nil {
t.Fatal("expected a self signature to be found")
}
Expand Down Expand Up @@ -1904,7 +1909,7 @@ zaXZE2aAMQ==
t.Fatal(err)
}
var config *packet.Config
sig, _ := key[0].PrimaryIdentity(config.Now())
sig, _ := key[0].PrimaryIdentity(config.Now(), &packet.Config{})
if err != nil {
t.Fatal(err)
}
Expand Down
Loading

0 comments on commit ad60d74

Please sign in to comment.