Reject signatures using insecure hash algorithms (#52)

crypto/message_test.go

@@ -143,6 +143,35 @@ func TestSignedMessageDecryption(t *testing.T) {
 	assert.Exactly(t, readTestFile("message_plaintext", true), decrypted.GetString())
 }
 
+func TestSHA256SignedMessageDecryption(t *testing.T) {
+	pgpMessage, err := NewPGPMessageFromArmored(readTestFile("message_sha256_signed", false))
+	if err != nil {
+		t.Fatal("Expected no error when unarmoring, got:", err)
+	}
+
+	decrypted, err := keyRingTestPrivate.Decrypt(pgpMessage, keyRingTestPrivate, 0)
+	if err != nil {
+		t.Fatal("Expected no error when decrypting, got:", err)
+	}
+	assert.Exactly(t, readTestFile("message_plaintext", true), decrypted.GetString())
+}
+
+func TestSHA1SignedMessageDecryption(t *testing.T) {
+	pgpMessage, err := NewPGPMessageFromArmored(readTestFile("message_sha1_signed", false))
+	if err != nil {
+		t.Fatal("Expected no error when unarmoring, got:", err)
+	}
+
+	decrypted, err := keyRingTestPrivate.Decrypt(pgpMessage, keyRingTestPrivate, 0)
+	if err == nil {
+		t.Fatal("Expected verification error when decrypting")
+	}
+	if err.Error() != "Signature Verification Error: Insecure signature" {
+		t.Fatal("Expected verification error when decrypting, got:", err)
+	}
+	assert.Exactly(t, readTestFile("message_plaintext", true), decrypted.GetString())
+}
+
 func TestMultipleKeyMessageEncryption(t *testing.T) {
 	var message = NewPlainMessageFromString("plain text")
 	assert.Exactly(t, 3, len(keyRingTestMultiple.entities))

crypto/signature.go

@@ -2,6 +2,7 @@ package crypto
 
 import (
 	"bytes"
+	"crypto"
 	"fmt"
 	"io"
 	"math"
@@ -15,6 +16,13 @@ import (
 	"github.com/ProtonMail/gopenpgp/v2/internal"
 )
 
+var allowedHashes = []crypto.Hash{
+	crypto.SHA224,
+	crypto.SHA256,
+	crypto.SHA384,
+	crypto.SHA512,
+}
+
 // SignatureVerificationError is returned from Decrypt and VerifyDetached
 // functions when signature verification fails.
 type SignatureVerificationError struct {
@@ -40,6 +48,15 @@ func newSignatureFailed() SignatureVerificationError {
 	}
 }
 
+// newSignatureInsecure creates a new SignatureVerificationError, type
+// SignatureFailed, with a message describing the signature as insecure.
+func newSignatureInsecure() SignatureVerificationError {
+	return SignatureVerificationError{
+		constants.SIGNATURE_FAILED,
+		"Insecure signature",
+	}
+}
+
 // newSignatureNotSigned creates a new SignatureVerificationError, type
 // SignatureNotSigned.
 func newSignatureNotSigned() SignatureVerificationError {
@@ -81,20 +98,21 @@ func processSignatureExpiration(md *openpgp.MessageDetails, verifyTime int64) {
 
 // verifyDetailsSignature verifies signature from message details.
 func verifyDetailsSignature(md *openpgp.MessageDetails, verifierKey *KeyRing) error {
-	if md.IsSigned {
+	if !md.IsSigned ||
-		if md.SignedBy == nil || len(verifierKey.entities) == 0 {
+		md.SignedBy == nil ||
+		len(verifierKey.entities) == 0 ||
+		len(verifierKey.entities.KeysById(md.SignedByKeyId)) == 0 {
 		return newSignatureNoVerifier()
 	}
-		matches := verifierKey.entities.KeysById(md.SignedByKeyId)
+	if md.SignatureError != nil {
-		if len(matches) > 0 {
-			if md.SignatureError == nil {
-				return nil
-			}
 		return newSignatureFailed()
 	}
+	if md.Signature == nil ||
+		md.Signature.Hash < allowedHashes[0] ||
+		md.Signature.Hash > allowedHashes[len(allowedHashes)-1] {
+		return newSignatureInsecure()
 	}
-
+	return nil
-	return newSignatureNoVerifier()
 }
 
 // verifySignature verifies if a signature is valid with the entity list.
@@ -111,7 +129,7 @@ func verifySignature(pubKeyEntries openpgp.EntityList, origText io.Reader, signa
 	}
 	signatureReader := bytes.NewReader(signature)
 
-	signer, err := openpgp.CheckDetachedSignature(pubKeyEntries, origText, signatureReader, config)
+	signer, err := openpgp.CheckDetachedSignatureAndHash(pubKeyEntries, origText, signatureReader, allowedHashes, config)
 
 	if err == pgpErrors.ErrSignatureExpired && signer != nil && verifyTime > 0 {
 		// if verifyTime = 0: time check disabled, everything is okay
@@ -126,7 +144,7 @@ func verifySignature(pubKeyEntries openpgp.EntityList, origText io.Reader, signa
 			return newSignatureFailed()
 		}
 
-		signer, err = openpgp.CheckDetachedSignature(pubKeyEntries, origText, signatureReader, config)
+		signer, err = openpgp.CheckDetachedSignatureAndHash(pubKeyEntries, origText, signatureReader, allowedHashes, config)
 		if err != nil {
 			return newSignatureFailed()
 		}

crypto/testdata/message_sha1_signed

@@ -0,0 +1,25 @@
+-----BEGIN PGP MESSAGE-----
+Version: OpenPGP.js v4.10.4
+Comment: https://openpgpjs.org
+
+wcBMA0fcZ7XLgmf2AQf/SRGI1T68u5cqeSbznX2T8mpHliVIXj1Oifa1cqaA
+spPxpuFMYctAS1J3yOVaFMtNSlFaGR3ftQb09d1Lx9JXW31tshWO+7EAZ44j
+yqS3y4KUqJPm9W73UIEUk0z49fiIDAnOTfMkSai1Y1KtSUtD+4ZRdkYRqEV0
+WfUlExkjDu98aBEzr4GPMefl8+VC17n6jOpOikT1rUJbelOIhpW2T9sEy0mw
+X200eaiVY1Ge9OTOEpLxY+pmFfdVZpmoOzSCn3C3uxNPT5uDzwBqFBUOAJHA
+oZNrZx67Nl/swa+Y7Mcr4z1iIMKbwn37Zk29zzat9tfJRhDTAOk/6Z5KjOPB
+qNLBeQFoUgbkNmoLDEbi6EYU9POQxkovnIfBGhJI5XkJlh/Dqe2iaysrNIHv
+JIhtqZ1Zq3NNsKdOsfCTkkQFZwJc8gsK72aRXPDMn+qXm9O6Tv3qXZL+Eewz
++RT+OI2xtc44xpY6tLPTmNYlVyqGC2yAUIbFeqct3A/98g1uZn617RGtHBiY
+ql1L4YeB/3mJpY4pK4Syota95vbNhJo5Sai9qnh+KTB8UlEbjmg6ULdH2tVG
+F7ET/IbjCs1D2wgdjs2fn/TIpO5sbVjo4VmxxnYdWWvf36r7QTmHXh1mAkKt
+EqSxQg/VUaeW/ew41zfO+xYJLGnpkvgcfyrzgHqrq0wEMe/+V+r8s1lRJ1NC
+8jv6vEyhH/cS/qTK7zhs8rS7JB0ywefuJKyu3XcBUSceQOmwG7GqhXkYYaAT
+yscUK82tOxw562ccxxgXtQ11EXrJw4+lOe2hVkp6y+MRYGFQY4VntpnxbrpY
+gPikwUOpDXbluzzn5pXgKXsMfdhxcYLL8NaBq6xMLRR+MNoO+fQ8WVR7HWcE
+BdYLl4vRXdwCBW1Bg1ndmF050d6otcPHZ54xaEqZDugtv3schPrqXaNSGMg3
+DTceof/a4KUaAfJxpP7BfCTrFu+CyLhNMS5RUeKNVEXxmXnlF6g8x6gEKbJA
+ave6M75z/Pluqpz6ZtYxKJPsutX4WPoGahY/Gu6aRkp1RcE7VFsSlv2sZq4A
+t7zowuG+7Y+PdQzOhNkxs17D5vhjJYqF+rmFwQ/TU9V6
+=nhYM
+-----END PGP MESSAGE-----

crypto/testdata/message_sha256_signed

@@ -0,0 +1,25 @@
+-----BEGIN PGP MESSAGE-----
+Version: OpenPGP.js v4.10.4
+Comment: https://openpgpjs.org
+
+wcBMA0fcZ7XLgmf2AQf/adxZ10qXLL0qVcYq7iDTpGNEKbR8UWcWqN6S6GnG
+XJwVretWIGLjJiPdh7irZcG993rCJr43P6uZg8AD/+7G/ceVFSRw2+dYHZmp
+bq3e89py+zGO/m3o+41RF+3ekH3LHZsb/x4Ob+ZZM3UqOX9cvFiKKN1MjyH2
+JkHWhQLtZVxDKwa77QBvs5fu8kzEdVFjA1RQhRbQpkuTMkfOEfcsuqHrWJrG
+T2eliNU5VzIBxfojLOSAF1ueMP3QMrB/vMimEnO140MJHf4mwj92+a9BxyXs
+UAbgKC79BsrF5CsND7+ANsoTJTv2eDxf6bVEfCk8HwT5X/C4hOgJ9ILOw8K2
+Y9LBeQH9RMVQej1Az9glJY7S3FlTL9IUndFPD5Mb/+zNjNv6Mg5UquxmeZZx
+S8RJDjAOTC2Rcv9jlELh6leS3meY+lpXar/ao+FL/1eB8zKkGsZqstfhmA14
+S5I5R4EgLmjkt+Ibg3Kj2sswWchB9eDdfvW/VwrhXU45Hv+EeKS/Pn+5qf05
+E5j7PQ3+EtdMTLIT3cSJe1a7zHlLR49hAzJ3Z5wPSmIzFYNE7RvnRA/7MMv/
+amlHAdIjDejDFIHR4LsT5e0GyMcAaDAOyGZRxM9fPo7PmZhz7mvoY4MGaBZ/
+icrv91IunxWo7QjLzubx4lVJM6xi2375Mntk4ZI6JNTblodOXEchTCmzNQxE
+jJkzLnlMU8/gTNLJh43pgy2F8+QVYjqZT61mnScBIUidQVNeqSOiHyLCsAgS
+Q2XAzIgf7HO+UBqHfrDG9wtNtUKVRrGhMwNiVXIO8VvdrKsYrWdmCgvKXZn8
+cdS9991BJQSavQRjf6rZRit+4iBzx3elYOtdN6zu3JrF1Mjm7pgDfij8Z1o1
+2hpUDNWPCa1idijPp9wvy03lXuVhB1U+rFb+ASslYoXyOASiZuKGAyxXaJwR
+5VOY1EKExsSzh74w2bvtFyO/BjF5OFqtL2j3CEu3CWPiYlFPZzF+/jf8v/QQ
+jfB1gs9u/Yz9ao9BU00eooLud3cS5mr8jeO/ugKh3rAqQEShsmoillyuNWlr
+Dusky1Y9txR4llJOdcdIyhMhnUdMd8yVF4FfhlGgVirY
+=oTYf
+-----END PGP MESSAGE-----