Randomize v4 and v5 signatures via custom notation (#209)

* Randomize v4 and v5 signatures via custom notation with the name from openpgpjs
ProtonMail · Jul 12, 2024 · 4333135 · 4333135
1 parent 196c8f5
commit 4333135
Show file tree

Hide file tree

Showing 7 changed files with 115 additions and 9 deletions.
diff --git a/openpgp/keys_test.go b/openpgp/keys_test.go
@@ -966,6 +966,9 @@ func TestNewEntityPrivateSerialization(t *testing.T) {
 }
 
 func TestNotationPacket(t *testing.T) {
+	config := &packet.Config{
+		NonDeterministicSignaturesViaNotation: packet.BoolPointer(false),
+	}
 	keys, err := ReadArmoredKeyRing(bytes.NewBufferString(keyWithNotation))
 	if err != nil {
 		t.Fatal(err)
@@ -974,7 +977,7 @@ func TestNotationPacket(t *testing.T) {
 	assertNotationPackets(t, keys)
 
 	serializedEntity := bytes.NewBuffer(nil)
-	err = keys[0].SerializePrivate(serializedEntity, nil)
+	err = keys[0].SerializePrivate(serializedEntity, config)
 	if err != nil {
 		t.Fatal(err)
 	}

diff --git a/openpgp/packet/config.go b/openpgp/packet/config.go
@@ -155,6 +155,14 @@ type Config struct {
 	// that the packet sequence conforms with the grammar mandated by rfc4880.
 	// The default behavior, when the config or flag is nil, is to check the packet sequence.
 	CheckPacketSequence *bool
+	// NonDeterministicSignaturesViaNotation is a flag to enable randomization of signatures.
+	// If true, a salt notation is used to randomize signatures generated by v4 and v5 keys
+	// (v6 signatures are always non-deterministic, by design).
+	// This protects EdDSA signatures from potentially leaking the secret key in case of faults (i.e. bitflips) which, in principle, could occur
+	// during the signing computation. It is added to signatures of any algo for simplicity, and as it may also serve as protection in case of
+	// weaknesses in the hash algo, potentially hindering e.g. some chosen-prefix attacks.
+	// The default behavior, when the config or flag is nil, is to enable the feature.
+	NonDeterministicSignaturesViaNotation *bool
 }
 
 func (c *Config) Random() io.Reader {
@@ -359,3 +367,16 @@ func (c *Config) StrictPacketSequence() bool {
 	}
 	return *c.CheckPacketSequence
 }
+
+func (c *Config) RandomizeSignaturesViaNotation() bool {
+	if c == nil || c.NonDeterministicSignaturesViaNotation == nil {
+		return true
+	}
+	return *c.NonDeterministicSignaturesViaNotation
+}
+
+// BoolPointer is a helper function to set a boolean pointer in the Config.
+// e.g., config.CheckPacketSequence = BoolPointer(true)
+func BoolPointer(value bool) *bool {
+	return &value
+}
diff --git a/openpgp/packet/private_key_test.go b/openpgp/packet/private_key_test.go
@@ -566,3 +566,43 @@ func TestElGamalValidation(t *testing.T) {
 	}
 	priv.Y = &y
 }
+
+func TestECDSASignerRandomizedNotation(t *testing.T) {
+	ecdsaPriv, err := ecdsa.GenerateKey(rand.Reader, ecc.NewGenericCurve(elliptic.P256()))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	priv := NewSignerPrivateKey(time.Now(), ecdsaPriv)
+	sig := &Signature{
+		Version:    4,
+		PubKeyAlgo: PubKeyAlgoECDSA,
+		Hash:       crypto.SHA256,
+	}
+	msg := make([]byte, mathrand.Intn(maxMessageLength))
+	rand.Read(msg)
+
+	h, err := populateHash(sig.Hash, msg)
+	if err != nil {
+		t.Fatal(err)
+	}
+	config := Config{
+		NonDeterministicSignaturesViaNotation: BoolPointer(true),
+	}
+	if err := sig.Sign(h, priv, &config); err != nil {
+		t.Fatal(err)
+	}
+
+	if h, err = populateHash(sig.Hash, msg); err != nil {
+		t.Fatal(err)
+	}
+	if err := priv.VerifySignature(h, sig); err != nil {
+		t.Fatal(err)
+	}
+	if len(sig.Notations) == 0 {
+		t.Fatalf("failed to find randomized notation")
+	}
+	if sig.Notations[0].Name != SaltNotationName {
+		t.Fatalf("failed to find randomized notation")
+	}
+}
diff --git a/openpgp/packet/signature.go b/openpgp/packet/signature.go
@@ -35,6 +35,8 @@ const (
 	KeyFlagGroupKey
 )
 
+const SaltNotationName = "salt@notations.openpgpjs.org"
+
 // Signature represents a signature. See RFC 4880, section 5.2.
 type Signature struct {
 	Version    int
@@ -882,6 +884,20 @@ func (sig *Signature) Sign(h hash.Hash, priv *PrivateKey, config *Config) (err e
 	}
 	sig.Version = priv.PublicKey.Version
 	sig.IssuerFingerprint = priv.PublicKey.Fingerprint
+	if sig.Version < 6 && config.RandomizeSignaturesViaNotation() {
+		sig.removeNotationsWithName(SaltNotationName)
+		salt, err := SignatureSaltForHash(sig.Hash, config.Random())
+		if err != nil {
+			return err
+		}
+		notation := Notation{
+			Name:            SaltNotationName,
+			Value:           salt,
+			IsCritical:      false,
+			IsHumanReadable: false,
+		}
+		sig.Notations = append(sig.Notations, &notation)
+	}
 	sig.outSubpackets, err = sig.buildSubpackets(priv.PublicKey)
 	if err != nil {
 		return err
@@ -1409,3 +1425,17 @@ func SignatureSaltForHash(hash crypto.Hash, randReader io.Reader) ([]byte, error
 	}
 	return salt, nil
 }
+
+// removeNotationsWithName removes all notations in this signature with the given name.
+func (sig *Signature) removeNotationsWithName(name string) {
+	if sig == nil || sig.Notations == nil {
+		return
+	}
+	updatedNotations := make([]*Notation, 0, len(sig.Notations))
+	for _, notation := range sig.Notations {
+		if notation.Name != name {
+			updatedNotations = append(updatedNotations, notation)
+		}
+	}
+	sig.Notations = updatedNotations
+}
diff --git a/openpgp/v2/keys_test.go b/openpgp/v2/keys_test.go
@@ -930,6 +930,9 @@ func TestNewEntityPrivateSerialization(t *testing.T) {
 }
 
 func TestNotationPacket(t *testing.T) {
+	keySerializeConfig := &packet.Config{
+		NonDeterministicSignaturesViaNotation: packet.BoolPointer(false),
+	}
 	keys, err := ReadArmoredKeyRing(bytes.NewBufferString(keyWithNotation))
 	if err != nil {
 		t.Fatal(err)
@@ -938,7 +941,7 @@ func TestNotationPacket(t *testing.T) {
 	assertNotationPackets(t, keys)
 
 	serializedEntity := bytes.NewBuffer(nil)
-	err = keys[0].SerializePrivate(serializedEntity, nil)
+	err = keys[0].SerializePrivate(serializedEntity, keySerializeConfig)
 	if err != nil {
 		t.Fatal(err)
 	}

diff --git a/openpgp/v2/write_test.go b/openpgp/v2/write_test.go
@@ -89,6 +89,7 @@ func TestSignDetachedWithNotation(t *testing.T) {
 			IsHumanReadable: true,
 		},
 	}
+	config.NonDeterministicSignaturesViaNotation = packet.BoolPointer(false)
 	err := DetachSign(signature, kring[:1], message, &config)
 	if err != nil {
 		t.Error(err)
@@ -138,6 +139,7 @@ func TestSignDetachedWithCriticalNotation(t *testing.T) {
 			IsCritical:      true,
 		},
 	}
+	config.NonDeterministicSignaturesViaNotation = packet.BoolPointer(false)
 	err := DetachSign(signature, kring[:1], message, &config)
 	if err != nil {
 		t.Error(err)
@@ -209,8 +211,11 @@ func TestNewEntity(t *testing.T) {
 		t.Errorf("BitLength %v, expected %v", bl, cfg.RSABits)
 	}
 
+	keySerializeConfig := &packet.Config{
+		NonDeterministicSignaturesViaNotation: packet.BoolPointer(false),
+	}
 	w := bytes.NewBuffer(nil)
-	if err := e.SerializePrivate(w, nil); err != nil {
+	if err := e.SerializePrivate(w, keySerializeConfig); err != nil {
 		t.Errorf("failed to serialize entity: %s", err)
 		return
 	}
@@ -227,7 +232,7 @@ func TestNewEntity(t *testing.T) {
 	}
 
 	w = bytes.NewBuffer(nil)
-	if err := e.SerializePrivate(w, nil); err != nil {
+	if err := e.SerializePrivate(w, keySerializeConfig); err != nil {
 		t.Errorf("failed to serialize entity second time: %s", err)
 		return
 	}
@@ -245,7 +250,7 @@ func TestNewEntity(t *testing.T) {
 	}
 
 	w = bytes.NewBuffer(nil)
-	if err := e.SerializePrivate(w, nil); err != nil {
+	if err := e.SerializePrivate(w, keySerializeConfig); err != nil {
 		t.Errorf("failed to serialize after encryption round: %s", err)
 		return
 	}

diff --git a/openpgp/write_test.go b/openpgp/write_test.go
@@ -86,6 +86,7 @@ func TestSignDetachedWithNotation(t *testing.T) {
 				IsHumanReadable: true,
 			},
 		},
+		NonDeterministicSignaturesViaNotation: packet.BoolPointer(false),
 	}
 	err := DetachSign(signature, kring[0], message, config)
 	if err != nil {
@@ -137,6 +138,7 @@ func TestSignDetachedWithCriticalNotation(t *testing.T) {
 				IsCritical:      true,
 			},
 		},
+		NonDeterministicSignaturesViaNotation: packet.BoolPointer(false),
 	}
 	err := DetachSign(signature, kring[0], message, config)
 	if err != nil {
@@ -180,7 +182,6 @@ func TestSignDetachedWithCriticalNotation(t *testing.T) {
 }
 
 func TestNewEntity(t *testing.T) {
-
 	// Check bit-length with no config.
 	e, err := NewEntity("Test User", "test", "test@example.com", nil)
 	if err != nil {
@@ -211,8 +212,11 @@ func TestNewEntity(t *testing.T) {
 		t.Errorf("BitLength %v, expected %v", bl, cfg.RSABits)
 	}
 
+	keySerializeConfig := &packet.Config{
+		NonDeterministicSignaturesViaNotation: packet.BoolPointer(false),
+	}
 	w := bytes.NewBuffer(nil)
-	if err := e.SerializePrivate(w, nil); err != nil {
+	if err := e.SerializePrivate(w, keySerializeConfig); err != nil {
 		t.Errorf("failed to serialize entity: %s", err)
 		return
 	}
@@ -229,7 +233,7 @@ func TestNewEntity(t *testing.T) {
 	}
 
 	w = bytes.NewBuffer(nil)
-	if err := e.SerializePrivate(w, nil); err != nil {
+	if err := e.SerializePrivate(w, keySerializeConfig); err != nil {
 		t.Errorf("failed to serialize entity second time: %s", err)
 		return
 	}
@@ -247,7 +251,7 @@ func TestNewEntity(t *testing.T) {
 	}
 
 	w = bytes.NewBuffer(nil)
-	if err := e.SerializePrivate(w, nil); err != nil {
+	if err := e.SerializePrivate(w, keySerializeConfig); err != nil {
 		t.Errorf("failed to serialize after encryption round: %s", err)
 		return
 	}