From 44016a15c4e092b0306846f99b55799903251122 Mon Sep 17 00:00:00 2001 From: Aron Wussler Date: Tue, 27 Jul 2021 12:51:54 +0200 Subject: [PATCH 1/4] Release version 2.2.1 --- CHANGELOG.md | 5 ++--- constants/armor.go | 2 +- constants/version.go | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c7800a..37ab976 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,15 +4,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## Unreleased - +## [2.2.1] 2021-07-27 ### Changed - - Changed the returned `SignatureVerificationError.Status` when trying to verify a message with no embedded signature. It used to return `constants.SIGNATURE_NO_VERIFIER` and now returns `constants.SIGNATURE_NOT_SIGNED`. This change impacts : - `func (sk *SessionKey) DecryptAndVerify(...)` - `func (msg *PlainMessageReader) VerifySignature(...)` - `func (keyRing *KeyRing) Decrypt(...)` +- Improved error messages for failures in password protected message decryption ### Added - Helper to access the SignatureVerificationError explicitly when decrypting streams in mobile apps: diff --git a/constants/armor.go b/constants/armor.go index 1f33704..7196b46 100644 --- a/constants/armor.go +++ b/constants/armor.go @@ -3,7 +3,7 @@ package constants // Constants for armored data. const ( - ArmorHeaderVersion = "GopenPGP 2.2.0" + ArmorHeaderVersion = "GopenPGP 2.2.1" ArmorHeaderComment = "https://gopenpgp.org" PGPMessageHeader = "PGP MESSAGE" PGPSignatureHeader = "PGP SIGNATURE" diff --git a/constants/version.go b/constants/version.go index 0df2485..26c0693 100644 --- a/constants/version.go +++ b/constants/version.go @@ -1,3 +1,3 @@ package constants -const Version = "2.2.0" +const Version = "2.2.1" From b50a051c7e6b1678e8833b714f69982d30d22edb Mon Sep 17 00:00:00 2001 From: Aron Wussler Date: Wed, 4 Nov 2020 16:42:28 +0100 Subject: [PATCH 2/4] Deprecate Key#Check() --- CHANGELOG.md | 7 +++++++ README.md | 22 ---------------------- crypto/key.go | 31 +------------------------------ crypto/key_test.go | 30 ++++++++++-------------------- 4 files changed, 18 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37ab976..a0a79ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased +### Security +- All keys are now checked on parsing from the underlying library + +### Deprecated +- `(key *Key) Check() (bool, error)` is now deprecated, all keys are now checked upon import from x/crypto + ## [2.2.1] 2021-07-27 ### Changed - Changed the returned `SignatureVerificationError.Status` when trying to verify a message with no embedded signature. It used to return `constants.SIGNATURE_NO_VERIFIER` and now returns `constants.SIGNATURE_NOT_SIGNED`. diff --git a/README.md b/README.md index 54f5e86..8238358 100644 --- a/README.md +++ b/README.md @@ -371,25 +371,3 @@ newPGPSplitMessage, err := pgpMessage.SeparateKeyAndData() // Key Packet is in newPGPSplitMessage.GetBinaryKeyPacket() // Data Packet is in newPGPSplitMessage.GetBinaryDataPacket() ``` - -### Checking keys -In order to check that the primary key is valid the `Key#Check` function can be used. -This operation is as of 2.0.0 fairly expensive, as it requires a signature operation. -It will be improved in the future versions, and possibly expanded to the subkeys, that are -for now assumed to be correct thanks to the binding signature. -```go -const privkey = `-----BEGIN PGP PRIVATE KEY BLOCK----- -... ------END PGP PRIVATE KEY BLOCK-----` // Encrypted private key -const passphrase = []byte("LongSecret") // Private key passphrase - -privateKeyObj, err := crypto.NewKeyFromArmored(privkey) -unlockedKeyObj = privateKeyObj.Unlock(passphrase) - -isVerified, _ := unlockedKeyObj.Check(); -if !isVerified { - // Handle broken keys -} -``` -This function runs on unlocked private keys, and it will return an error if called with public keys -or locked keys. diff --git a/crypto/key.go b/crypto/key.go index 49af11e..25af16e 100644 --- a/crypto/key.go +++ b/crypto/key.go @@ -306,37 +306,8 @@ func (key *Key) IsUnlocked() (bool, error) { // Check verifies if the public keys match the private key parameters by // signing and verifying. +// Deprecated: all keys are now checked on parsing. func (key *Key) Check() (bool, error) { - var err error - testSign := bytes.Repeat([]byte{0x01}, 64) - testReader := bytes.NewReader(testSign) - - if !key.IsPrivate() { - return false, errors.New("gopenpgp: can check only private key") - } - - unlocked, err := key.IsUnlocked() - if err != nil { - return false, err - } - - if !unlocked { - return false, errors.New("gopenpgp: key is not unlocked") - } - - var signBuf bytes.Buffer - - if err = openpgp.DetachSign(&signBuf, key.entity, testReader, nil); err != nil { - return false, errors.New("gopenpgp: unable to sign with key") - } - - testReader = bytes.NewReader(testSign) - signer, err := openpgp.CheckDetachedSignature(openpgp.EntityList{key.entity}, testReader, &signBuf, nil) - - if signer == nil || err != nil { - return false, nil - } - return true, nil } diff --git a/crypto/key_test.go b/crypto/key_test.go index 9eb35f5..e2c2808 100644 --- a/crypto/key_test.go +++ b/crypto/key_test.go @@ -241,33 +241,23 @@ func TestGenerateKeyWithPrimes(t *testing.T) { assert.Exactly(t, prime2, pk.Primes[1].Bytes()) } -func TestCheckIntegrity(t *testing.T) { - isVerified, err := keyTestRSA.Check() - if err != nil { - t.Fatal("Expected no error while checking correct passphrase, got:", err) - } - - assert.Exactly(t, true, isVerified) +func TestFailCheckIntegrity25519(t *testing.T) { + failCheckIntegrity(t, "x25519", 0) } -func TestFailCheckIntegrity(t *testing.T) { - // This test is done with ECC because in an RSA key we would need to replace the primes, but maintaining the moduli, - // that is a private struct element. - k1, _ := GenerateKey(keyTestName, keyTestDomain, "x25519", 256) - k2, _ := GenerateKey(keyTestName, keyTestDomain, "x25519", 256) +func TestFailCheckIntegrityRSA(t *testing.T) { + failCheckIntegrity(t, "rsa", 2048) +} + +func failCheckIntegrity(t *testing.T, keyType string, bits int) { + k1, _ := GenerateKey(keyTestName, keyTestDomain, keyType, bits) + k2, _ := GenerateKey(keyTestName, keyTestDomain, keyType, bits) k1.entity.PrivateKey.PrivateKey = k2.entity.PrivateKey.PrivateKey // Swap private keys - isVerified, err := k1.Check() - if err != nil { - t.Fatal("Expected no error while checking key, got:", err) - } - - assert.Exactly(t, false, isVerified) - serialized, err := k1.Serialize() if err != nil { - t.Fatal("Expected no error while serializing keyring kr3, got:", err) + t.Fatal("Expected no error while serializing keyring, got:", err) } _, err = NewKey(serialized) From 48d4852e6af81346bc15c530d85407b7fdbfe65b Mon Sep 17 00:00:00 2001 From: Aron Wussler Date: Fri, 30 Jul 2021 12:28:03 +0200 Subject: [PATCH 3/4] Improve readme --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index 8238358..04b71b1 100644 --- a/README.md +++ b/README.md @@ -371,3 +371,6 @@ newPGPSplitMessage, err := pgpMessage.SeparateKeyAndData() // Key Packet is in newPGPSplitMessage.GetBinaryKeyPacket() // Data Packet is in newPGPSplitMessage.GetBinaryDataPacket() ``` + +### Checking keys +Keys are now checked on import and the explicit check via `Key#Check()` is deprecated and no longer necessary. \ No newline at end of file From 5904ff3d703b8461fc7e06a90f5c264210cbc8b3 Mon Sep 17 00:00:00 2001 From: Aron Wussler Date: Fri, 30 Jul 2021 12:55:45 +0200 Subject: [PATCH 4/4] Add static malformed key test --- crypto/key_test.go | 11 +++++++++++ crypto/testdata/key_mismatching_eddsa_key | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 crypto/testdata/key_mismatching_eddsa_key diff --git a/crypto/key_test.go b/crypto/key_test.go index e2c2808..1130e7f 100644 --- a/crypto/key_test.go +++ b/crypto/key_test.go @@ -415,3 +415,14 @@ func TestKeyCapabilities(t *testing.T) { assert.True(t, publicKey.CanVerify()) assert.True(t, publicKey.CanEncrypt()) } + +func TestUnlockMismatchingKey(t *testing.T) { + privateKey, err := NewKeyFromArmored(readTestFile("key_mismatching_eddsa_key", false)) + if err != nil { + t.Fatal("Expected no error while unarmoring private key, got:", err) + } + + if _, err = privateKey.Unlock([]byte("123")); err == nil { + t.Fatalf("Mismatching private key was not detected") + } +} diff --git a/crypto/testdata/key_mismatching_eddsa_key b/crypto/testdata/key_mismatching_eddsa_key new file mode 100644 index 0000000..e931d2a --- /dev/null +++ b/crypto/testdata/key_mismatching_eddsa_key @@ -0,0 +1,18 @@ +-----BEGIN PGP PRIVATE KEY BLOCK----- +Version: OpenPGP.js v4.10.4 +Comment: https://openpgpjs.org + +xYYEX6FzhhYJKwYBBAHaRw8BAQdA0Yeb41B5BRI25Dq0z7oRCcImRQfT+WcJ +HLaHpd6baGT+CQMI/6uWP8YxbIIAJDKhBYD+D8yTJW6JBtnOISKzPSbTNxOP +aQEQz1YPMhZhTBIWZFNXo614n3C7Ak1Q73bv8zsJq0EmRJPNkFXmY1S/B0Lm +GM0SQm9iIDxpbmZvQGJvYi5jb20+wngEEBYKACAFAl+hc4YGCwkHCAMCBBUI +CgIEFgIBAAIZAQIbAwIeAQAKCRBeNKXxFWm5mDjwAQC6vYacL9/oBZ3Ev3DR +l9a//92L8hYrx3Le3pRZhJmDqAEAoz5fPGxQEXdgzI14i7ZdNsRRzyGQh3nL +jnGeUJx7aQnHiwRfoXOGEgorBgEEAZdVAQUBAQdA2yQoAO2pqLJe48Wazz6+ +PSxyh5EXGQZ9yy/ZO2y8Rl8DAQgH/gkDCJE4VlUHrTStAIYCzED5f7BXphR0 +jqbMnwTxXxFsI7H9kUykmOPjzYayTLkp/Pw5OMzxjwVKD6+zO4YKtd9EuhEH +pD32k72LbNPYE/j3p77CYQQYFggACQUCX6FzhgIbDAAKCRBeNKXxFWm5mJa3 +AQDZA/yx5SotHacjZmJir/ly7aPdPUv4krqx86BHbl/2HAD/YSPzjDxBYUDA +kWK2+FnnoMBGL6PIKIHlxONRrDVknwQ= +=IsER +-----END PGP PRIVATE KEY BLOCK----- \ No newline at end of file