From b11dc8b1c467826bcaf33b2e94598c395272c933 Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Mon, 28 Oct 2024 12:33:17 +0100 Subject: [PATCH 1/8] feat: Improve error message for decryption with a session key --- openpgp/errors/errors.go | 39 +++++- openpgp/packet/aead_crypter.go | 8 +- .../packet/symmetric_key_encrypted_test.go | 118 +++++++++++++++--- openpgp/read.go | 12 +- openpgp/read_test.go | 4 +- openpgp/v2/read.go | 28 +++-- openpgp/v2/read_test.go | 4 +- 7 files changed, 170 insertions(+), 43 deletions(-) diff --git a/openpgp/errors/errors.go b/openpgp/errors/errors.go index c42b01cb0..b00892412 100644 --- a/openpgp/errors/errors.go +++ b/openpgp/errors/errors.go @@ -9,6 +9,21 @@ import ( "strconv" ) +const ( + // GenericParsingErrorMessage is a generic error message for parsing errors + // to reduce the risk of oracle attacks. + GenericParsingErrorMessage = "parsing error" +) + +var ( + // ErrAEADTagVerification is returned if one of the tag verifications in SEIPDv2 fails + ErrAEADTagVerification error = DecryptWithSessionKeyError("AEAD tag verification failed") + // ErrMDCHashMismatch is returned if the final tag verification in SEIPDv1 fails + ErrMDCHashMismatch error = DecryptWithSessionKeyError("MDC hash mismatch") + // ErrMDCMissing is returned if the final tag in SEIPDv1 is missing + ErrMDCMissing error = DecryptWithSessionKeyError("MDC packet not found") +) + // A StructuralError is returned when OpenPGP data is found to be syntactically // invalid. type StructuralError string @@ -17,6 +32,27 @@ func (s StructuralError) Error() string { return "openpgp: invalid data: " + string(s) } +// A DecryptWithSessionKeyError is returned when a failure occurs when reading from symmetrically decrypted data or +// an authentication tag verification fails. +// Such an error indicates that the supplied session key is likely wrong or the data got corrupted. +type DecryptWithSessionKeyError string + +func (s DecryptWithSessionKeyError) Error() string { + return "openpgp: decryption with session key failed: " + string(s) +} + +// HandleDecryptionSensitiveParsingError handles symmetric decryption errors. +// The function makes parsing errors generic to reduce the risk of oracle attacks. +func HandleDecryptionSensitiveParsingError(err error) error { + if decError, ok := err.(*DecryptWithSessionKeyError); ok { + return decError + } + if decError, ok := err.(DecryptWithSessionKeyError); ok { + return decError + } + return DecryptWithSessionKeyError(GenericParsingErrorMessage) +} + // UnsupportedError indicates that, although the OpenPGP data is valid, it // makes use of currently unimplemented features. type UnsupportedError string @@ -41,9 +77,6 @@ func (b SignatureError) Error() string { return "openpgp: invalid signature: " + string(b) } -var ErrMDCHashMismatch error = SignatureError("MDC hash mismatch") -var ErrMDCMissing error = SignatureError("MDC packet not found") - type signatureExpiredError int func (se signatureExpiredError) Error() string { diff --git a/openpgp/packet/aead_crypter.go b/openpgp/packet/aead_crypter.go index 7171387f9..2eecd062f 100644 --- a/openpgp/packet/aead_crypter.go +++ b/openpgp/packet/aead_crypter.go @@ -147,7 +147,7 @@ func (ar *aeadDecrypter) openChunk(data []byte) ([]byte, error) { nonce := ar.computeNextNonce() plainChunk, err := ar.aead.Open(nil, nonce, chunk, adata) if err != nil { - return nil, err + return nil, errors.ErrAEADTagVerification } ar.bytesProcessed += len(plainChunk) if err = ar.aeadCrypter.incrementIndex(); err != nil { @@ -172,8 +172,10 @@ func (ar *aeadDecrypter) validateFinalTag(tag []byte) error { // ... and total number of encrypted octets adata = append(adata, amountBytes...) nonce := ar.computeNextNonce() - _, err := ar.aead.Open(nil, nonce, tag, adata) - return err + if _, err := ar.aead.Open(nil, nonce, tag, adata); err != nil { + return errors.ErrAEADTagVerification + } + return nil } // aeadEncrypter encrypts and writes bytes. It encrypts when necessary according diff --git a/openpgp/packet/symmetric_key_encrypted_test.go b/openpgp/packet/symmetric_key_encrypted_test.go index 6bb1f2a34..dd0c1b0a9 100644 --- a/openpgp/packet/symmetric_key_encrypted_test.go +++ b/openpgp/packet/symmetric_key_encrypted_test.go @@ -12,6 +12,7 @@ import ( mathrand "math/rand" "testing" + "github.com/ProtonMail/go-crypto/openpgp/errors" "github.com/ProtonMail/go-crypto/openpgp/s2k" ) @@ -20,25 +21,12 @@ const maxPassLen = 64 // Tests against RFC vectors func TestDecryptSymmetricKeyAndEncryptedDataPacket(t *testing.T) { for _, testCase := range keyAndIpePackets() { - // Key - buf := readerFromHex(testCase.packets) - packet, err := Read(buf) - if err != nil { - t.Fatalf("failed to read SymmetricKeyEncrypted: %s", err) - } - ske, ok := packet.(*SymmetricKeyEncrypted) - if !ok { - t.Fatal("didn't find SymmetricKeyEncrypted packet") - } - // Decrypt key - key, cipherFunc, err := ske.Decrypt([]byte(testCase.password)) - if err != nil { - t.Fatal(err) - } - packet, err = Read(buf) - if err != nil { - t.Fatalf("failed to read SymmetricallyEncrypted: %s", err) - } + // Read and verify the key packet + ske, dataPacket := readSymmetricKeyEncrypted(t, testCase.packets) + key, cipherFunc := decryptSymmetricKey(t, ske, []byte(testCase.password)) + + packet := readSymmetricallyEncrypted(t, dataPacket) + // Decrypt contents var edp EncryptedDataPacket switch p := packet.(type) { @@ -49,6 +37,7 @@ func TestDecryptSymmetricKeyAndEncryptedDataPacket(t *testing.T) { default: t.Fatal("no integrity protected packet") } + r, err := edp.Decrypt(cipherFunc, key) if err != nil { t.Fatal(err) @@ -66,6 +55,97 @@ func TestDecryptSymmetricKeyAndEncryptedDataPacket(t *testing.T) { } } +func TestTagVerificationError(t *testing.T) { + for _, testCase := range keyAndIpePackets() { + ske, dataPacket := readSymmetricKeyEncrypted(t, testCase.packets) + key, cipherFunc := decryptSymmetricKey(t, ske, []byte(testCase.password)) + + // Corrupt chunk + tmp := make([]byte, len(dataPacket)) + copy(tmp, dataPacket) + tmp[38] += 1 + packet := readSymmetricallyEncrypted(t, tmp) + // Decrypt contents and check integrity + checkIntegrityError(t, packet, cipherFunc, key) + + // Corrupt final tag or mdc + dataPacket[len(dataPacket)-1] += 1 + packet = readSymmetricallyEncrypted(t, dataPacket) + // Decrypt contents and check integrity + checkIntegrityError(t, packet, cipherFunc, key) + } +} + +func readSymmetricKeyEncrypted(t *testing.T, packetHex string) (*SymmetricKeyEncrypted, []byte) { + t.Helper() + + buf := readerFromHex(packetHex) + packet, err := Read(buf) + if err != nil { + t.Fatalf("failed to read SymmetricKeyEncrypted: %s", err) + } + + ske, ok := packet.(*SymmetricKeyEncrypted) + if !ok { + t.Fatal("didn't find SymmetricKeyEncrypted packet") + } + + dataPacket, err := io.ReadAll(buf) + if err != nil { + t.Fatalf("failed to read data packet: %s", err) + } + return ske, dataPacket +} + +func decryptSymmetricKey(t *testing.T, ske *SymmetricKeyEncrypted, password []byte) ([]byte, CipherFunction) { + t.Helper() + + key, cipherFunc, err := ske.Decrypt(password) + if err != nil { + t.Fatalf("failed to decrypt symmetric key: %s", err) + } + + return key, cipherFunc +} + +func readSymmetricallyEncrypted(t *testing.T, dataPacket []byte) Packet { + t.Helper() + packet, err := Read(bytes.NewReader(dataPacket)) + if err != nil { + t.Fatalf("failed to read SymmetricallyEncrypted: %s", err) + } + return packet +} + +func checkIntegrityError(t *testing.T, packet Packet, cipherFunc CipherFunction, key []byte) { + t.Helper() + + switch p := packet.(type) { + case *SymmetricallyEncrypted: + edp := p + data, err := edp.Decrypt(cipherFunc, key) + if err != nil { + t.Fatal(err) + } + + _, err = io.ReadAll(data) + if err != nil { + if edp.Version == 1 && err != errors.ErrMDCHashMismatch { + t.Fatalf("no integrity error (expected MDC hash mismatch)") + } + if edp.Version == 2 && err != errors.ErrAEADTagVerification { + t.Fatalf("no integrity error (expected AEAD tag verification failure)") + } + } else { + t.Fatalf("no error (expected integrity check failure)") + } + case *AEADEncrypted: + return + default: + t.Fatal("no integrity protected packet found") + } +} + func TestSerializeSymmetricKeyEncryptedV6RandomizeSlow(t *testing.T) { ciphers := map[string]CipherFunction{ "AES128": CipherAES128, diff --git a/openpgp/read.go b/openpgp/read.go index 8a69b44a5..629a0b741 100644 --- a/openpgp/read.go +++ b/openpgp/read.go @@ -233,7 +233,10 @@ FindKey: } mdFinal, sensitiveParsingErr := readSignedMessage(packets, md, keyring, config) if sensitiveParsingErr != nil { - return nil, errors.StructuralError("parsing error") + if md.decrypted != nil { + return nil, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingErr) + } + return nil, errors.StructuralError(errors.GenericParsingErrorMessage) } return mdFinal, nil } @@ -368,7 +371,7 @@ func (cr *checkReader) Read(buf []byte) (int, error) { } if sensitiveParsingError != nil { - return n, errors.StructuralError("parsing error") + return n, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingError) } return n, nil @@ -443,7 +446,10 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { } if sensitiveParsingError != nil { - return n, errors.StructuralError("parsing error") + if scr.md.decrypted != nil { + return n, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingError) + } + return n, errors.StructuralError(errors.GenericParsingErrorMessage) } return n, nil diff --git a/openpgp/read_test.go b/openpgp/read_test.go index a9846cbc2..e8867a3f6 100644 --- a/openpgp/read_test.go +++ b/openpgp/read_test.go @@ -800,6 +800,7 @@ func TestSymmetricAeadEaxOpenPGPJsMessage(t *testing.T) { func TestCorruptedMessageInvalidSigHeader(t *testing.T) { // Decrypt message with corrupted MDC and invalid one-pass-signature header // Expect parsing errors over unverified decrypted data to be opaque + var expectedErr string = errors.DecryptWithSessionKeyError(errors.GenericParsingErrorMessage).Error() passphrase := []byte("password") file, err := os.Open("test_data/sym-corrupted-message-invalid-sig-header.asc") if err != nil { @@ -819,7 +820,6 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } - const expectedErr string = "openpgp: invalid data: parsing error" _, observedErr := ReadMessage(raw.Body, nil, promptFunc, nil) if observedErr.Error() != expectedErr { t.Errorf("Expected error '%s', but got error '%s'", expectedErr, observedErr) @@ -829,11 +829,11 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { func TestCorruptedMessageWrongLength(t *testing.T) { // Decrypt message with wrong length in Literal packet header (length too long) // Expect parsing errors over unverified decrypted data to be opaque + var expectedErr string = errors.DecryptWithSessionKeyError(errors.GenericParsingErrorMessage).Error() passphrase := []byte("password") promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } - const expectedErr string = "openpgp: invalid data: parsing error" file, err := os.Open("test_data/sym-corrupted-message-long-length.asc") if err != nil { diff --git a/openpgp/v2/read.go b/openpgp/v2/read.go index 24c8c8f0d..bfa049fef 100644 --- a/openpgp/v2/read.go +++ b/openpgp/v2/read.go @@ -268,7 +268,10 @@ FindKey: } mdFinal, sensitiveParsingErr := readSignedMessage(packets, md, keyring, config) if sensitiveParsingErr != nil { - return nil, errors.StructuralError("parsing error") + if md.decrypted != nil { + return nil, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingErr) + } + return nil, errors.StructuralError(errors.GenericParsingErrorMessage) } return mdFinal, nil } @@ -489,7 +492,7 @@ func (cr *checkReader) Read(buf []byte) (int, error) { return n, io.EOF } if sensitiveParsingError != nil { - return n, errors.StructuralError("parsing error") + return n, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingError) } return n, nil } @@ -524,7 +527,7 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { break } if err != nil { - return n, errors.StructuralError("parsing error") + return n, errors.StructuralError(errors.GenericParsingErrorMessage) } if sig, ok := p.(*packet.Signature); ok { if sig.Version == 5 && scr.md.LiteralData != nil && (sig.SigType == 0x00 || sig.SigType == 0x01) { @@ -632,7 +635,7 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { break } if err != nil { - return 0, errors.StructuralError("parsing error") + return 0, errors.StructuralError(errors.GenericParsingErrorMessage) } } @@ -650,7 +653,10 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { } if sensitiveParsingError != nil { - return n, errors.StructuralError("parsing error") + if scr.md.decrypted != nil { + return n, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingError) + } + return n, errors.StructuralError(errors.GenericParsingErrorMessage) } return n, nil @@ -741,12 +747,12 @@ func verifyDetachedSignatureReader(keyring KeyRing, signed, signature io.Reader, // checkSignatureDetails verifies the metadata of the signature. // It checks the following: -// - Hash function should not be invalid according to -// config.RejectHashAlgorithms. -// - Verification key must be older than the signature creation time. -// - Check signature notations. -// - Signature is not expired (unless a zero time is passed to -// explicitly ignore expiration). +// - Hash function should not be invalid according to +// config.RejectHashAlgorithms. +// - Verification key must be older than the signature creation time. +// - Check signature notations. +// - Signature is not expired (unless a zero time is passed to +// explicitly ignore expiration). func checkSignatureDetails(pk *packet.PublicKey, signature *packet.Signature, now time.Time, config *packet.Config) error { if config.RejectHashAlgorithm(signature.Hash) { return errors.SignatureError("insecure hash algorithm: " + signature.Hash.String()) diff --git a/openpgp/v2/read_test.go b/openpgp/v2/read_test.go index 25926d810..9c8b6cff4 100644 --- a/openpgp/v2/read_test.go +++ b/openpgp/v2/read_test.go @@ -832,6 +832,7 @@ func TestSymmetricAeadEaxOpenPGPJsMessage(t *testing.T) { func TestCorruptedMessageInvalidSigHeader(t *testing.T) { // Decrypt message with corrupted MDC and invalid one-pass-signature header // Expect parsing errors over unverified decrypted data to be opaque + var expectedErr string = errors.DecryptWithSessionKeyError(errors.GenericParsingErrorMessage).Error() passphrase := []byte("password") file, err := os.Open("../test_data/sym-corrupted-message-invalid-sig-header.asc") if err != nil { @@ -851,7 +852,6 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } - const expectedErr string = "openpgp: invalid data: parsing error" _, observedErr := ReadMessage(raw.Body, nil, promptFunc, nil) if observedErr.Error() != expectedErr { t.Errorf("Expected error '%s', but got error '%s'", expectedErr, observedErr) @@ -861,11 +861,11 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { func TestCorruptedMessageWrongLength(t *testing.T) { // Decrypt message with wrong length in Literal packet header (length too long) // Expect parsing errors over unverified decrypted data to be opaque + var expectedErr string = errors.DecryptWithSessionKeyError(errors.GenericParsingErrorMessage).Error() passphrase := []byte("password") promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } - const expectedErr string = "openpgp: invalid data: parsing error" file, err := os.Open("../test_data/sym-corrupted-message-long-length.asc") if err != nil { From 3e80b6826b2c79b28753fd0e1d2b9472763ca96b Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Mon, 11 Nov 2024 15:09:59 +0100 Subject: [PATCH 2/8] chore: Add explicit error message strings to tests --- openpgp/read_test.go | 4 ++-- openpgp/v2/read_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/openpgp/read_test.go b/openpgp/read_test.go index e8867a3f6..bf6040043 100644 --- a/openpgp/read_test.go +++ b/openpgp/read_test.go @@ -800,7 +800,7 @@ func TestSymmetricAeadEaxOpenPGPJsMessage(t *testing.T) { func TestCorruptedMessageInvalidSigHeader(t *testing.T) { // Decrypt message with corrupted MDC and invalid one-pass-signature header // Expect parsing errors over unverified decrypted data to be opaque - var expectedErr string = errors.DecryptWithSessionKeyError(errors.GenericParsingErrorMessage).Error() + var expectedErr string = "openpgp: decryption with session key failed: parsing error" passphrase := []byte("password") file, err := os.Open("test_data/sym-corrupted-message-invalid-sig-header.asc") if err != nil { @@ -829,7 +829,7 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { func TestCorruptedMessageWrongLength(t *testing.T) { // Decrypt message with wrong length in Literal packet header (length too long) // Expect parsing errors over unverified decrypted data to be opaque - var expectedErr string = errors.DecryptWithSessionKeyError(errors.GenericParsingErrorMessage).Error() + var expectedErr string = "openpgp: decryption with session key failed: parsing error" passphrase := []byte("password") promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil diff --git a/openpgp/v2/read_test.go b/openpgp/v2/read_test.go index 9c8b6cff4..b5f31860f 100644 --- a/openpgp/v2/read_test.go +++ b/openpgp/v2/read_test.go @@ -832,7 +832,7 @@ func TestSymmetricAeadEaxOpenPGPJsMessage(t *testing.T) { func TestCorruptedMessageInvalidSigHeader(t *testing.T) { // Decrypt message with corrupted MDC and invalid one-pass-signature header // Expect parsing errors over unverified decrypted data to be opaque - var expectedErr string = errors.DecryptWithSessionKeyError(errors.GenericParsingErrorMessage).Error() + var expectedErr string = "openpgp: decryption with session key failed: parsing error" passphrase := []byte("password") file, err := os.Open("../test_data/sym-corrupted-message-invalid-sig-header.asc") if err != nil { @@ -861,7 +861,7 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { func TestCorruptedMessageWrongLength(t *testing.T) { // Decrypt message with wrong length in Literal packet header (length too long) // Expect parsing errors over unverified decrypted data to be opaque - var expectedErr string = errors.DecryptWithSessionKeyError(errors.GenericParsingErrorMessage).Error() + var expectedErr string = "openpgp: decryption with session key failed: parsing error" passphrase := []byte("password") promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil From d6044b8c077fbfaf72f0929267c073a28f82825d Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Mon, 11 Nov 2024 16:12:23 +0100 Subject: [PATCH 3/8] feat: Update SEIPD test vectors --- .../symmetric_key_encrypted_data_test.go | 19 +++++++++++++------ .../packet/symmetric_key_encrypted_test.go | 3 +++ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/openpgp/packet/symmetric_key_encrypted_data_test.go b/openpgp/packet/symmetric_key_encrypted_data_test.go index d8cf294f2..04a5ff890 100644 --- a/openpgp/packet/symmetric_key_encrypted_data_test.go +++ b/openpgp/packet/symmetric_key_encrypted_data_test.go @@ -11,9 +11,9 @@ type packetSequence struct { func keyAndIpePackets() []*packetSequence { if V5Disabled { - return []*packetSequence{symEncTestv6} + return []*packetSequence{symEncRFC9580, symEncRFC4880} } - return []*packetSequence{symEncTestv6, aeadEaxRFC, aeadOcbRFC} + return []*packetSequence{symEncRFC9580, symEncRFC4880, aeadEaxRFC, aeadOcbRFC} } // https://www.ietf.org/archive/id/draft-koch-openpgp-2015-rfc4880bis-00.html#name-complete-aead-eax-encrypted- @@ -30,9 +30,16 @@ var aeadOcbRFC = &packetSequence{ contents: "cb1462000000000048656c6c6f2c20776f726c64210a", } -// OpenPGP crypto refresh A.7.1. -var symEncTestv6 = &packetSequence{ +// From OpenPGP RFC9580 A.9 https://www.rfc-editor.org/rfc/rfc9580.html#name-sample-aead-eax-encryption- +var symEncRFC9580 = &packetSequence{ password: "password", - packets: "c33c061a07030b0308e9d39785b2070008ffb42e7c483ef4884457cb3726b9b3db9ff776e5f4d9a40952e2447298851abfff7526df2dd554417579a7799fd26902070306fcb94490bcb98bbdc9d106c6090266940f72e89edc21b5596b1576b101ed0f9ffc6fc6d65bbfd24dcd0790966e6d1e85a30053784cb1d8b6a0699ef12155a7b2ad6258531b57651fd7777912fa95e35d9b40216f69a4c248db28ff4331f1632907399e6ff9", - contents: "cb1362000000000048656c6c6f2c20776f726c6421d50e1ce2269a9eddef81032172b7ed7c", + packets: "c340061e07010b0308a5ae579d1fc5d82bff69224f919993b3506fa3b59a6a73cff8c5efc5f41c57fb54e1c226815d7828f5f92c454eb65ebe00ab5986c68e6e7c55d269020701069ff90e3b321964f3a42913c8dcc6619325015227efb7eaeaa49f04c2e674175d4a3d226ed6afcb9ca9ac122c1470e11c63d4c0ab241c6a938ad48bf99a5a99b90bba8325de61047540258ab7959a95ad051dda96eb15431dfef5f5e2255ca78261546e339a", + contents: "cb1362000000000048656c6c6f2c20776f726c6421d50eae5bf0cd6705500355816cb0c8ff", +} + +// From the OpenPGP interoperability test suite (Test: S2K mechanisms, iterated min + esk) +var symEncRFC4880 = &packetSequence{ + password: "password", + packets: "c32e0409030873616c7a6967657200080674a0d96a4a6e122b1d5bbaa3fac117b9cbb46c7e38f12967386b57e2f79d11d23f01cee77ceed8544e6d52c78bd33c81bd366c8673b68955ddbd1ade98fe6a9b4e27ae54cd10dda7cd3a4637f44e0ead895ebebdcf0c679f1342745628f104e7", + contents: "cb1462000000000048656c6c6f20576f726c64203a29", } diff --git a/openpgp/packet/symmetric_key_encrypted_test.go b/openpgp/packet/symmetric_key_encrypted_test.go index dd0c1b0a9..8271b0c24 100644 --- a/openpgp/packet/symmetric_key_encrypted_test.go +++ b/openpgp/packet/symmetric_key_encrypted_test.go @@ -129,6 +129,9 @@ func checkIntegrityError(t *testing.T, packet Packet, cipherFunc CipherFunction, } _, err = io.ReadAll(data) + if err == nil { + err = data.Close() + } if err != nil { if edp.Version == 1 && err != errors.ErrMDCHashMismatch { t.Fatalf("no integrity error (expected MDC hash mismatch)") From 015d6b09f83657261cf6dc51bdf4bbfbd926e2d8 Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Mon, 11 Nov 2024 16:17:34 +0100 Subject: [PATCH 4/8] chore: Move expected error to the old position --- openpgp/read_test.go | 4 ++-- openpgp/v2/read_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/openpgp/read_test.go b/openpgp/read_test.go index bf6040043..23fd4aec1 100644 --- a/openpgp/read_test.go +++ b/openpgp/read_test.go @@ -800,7 +800,6 @@ func TestSymmetricAeadEaxOpenPGPJsMessage(t *testing.T) { func TestCorruptedMessageInvalidSigHeader(t *testing.T) { // Decrypt message with corrupted MDC and invalid one-pass-signature header // Expect parsing errors over unverified decrypted data to be opaque - var expectedErr string = "openpgp: decryption with session key failed: parsing error" passphrase := []byte("password") file, err := os.Open("test_data/sym-corrupted-message-invalid-sig-header.asc") if err != nil { @@ -820,6 +819,7 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } + const expectedErr string = "openpgp: decryption with session key failed: parsing error" _, observedErr := ReadMessage(raw.Body, nil, promptFunc, nil) if observedErr.Error() != expectedErr { t.Errorf("Expected error '%s', but got error '%s'", expectedErr, observedErr) @@ -829,11 +829,11 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { func TestCorruptedMessageWrongLength(t *testing.T) { // Decrypt message with wrong length in Literal packet header (length too long) // Expect parsing errors over unverified decrypted data to be opaque - var expectedErr string = "openpgp: decryption with session key failed: parsing error" passphrase := []byte("password") promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } + const expectedErr string = "openpgp: decryption with session key failed: parsing error" file, err := os.Open("test_data/sym-corrupted-message-long-length.asc") if err != nil { diff --git a/openpgp/v2/read_test.go b/openpgp/v2/read_test.go index b5f31860f..24eec2f49 100644 --- a/openpgp/v2/read_test.go +++ b/openpgp/v2/read_test.go @@ -832,7 +832,6 @@ func TestSymmetricAeadEaxOpenPGPJsMessage(t *testing.T) { func TestCorruptedMessageInvalidSigHeader(t *testing.T) { // Decrypt message with corrupted MDC and invalid one-pass-signature header // Expect parsing errors over unverified decrypted data to be opaque - var expectedErr string = "openpgp: decryption with session key failed: parsing error" passphrase := []byte("password") file, err := os.Open("../test_data/sym-corrupted-message-invalid-sig-header.asc") if err != nil { @@ -852,6 +851,7 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } + const expectedErr string = "openpgp: decryption with session key failed: parsing error" _, observedErr := ReadMessage(raw.Body, nil, promptFunc, nil) if observedErr.Error() != expectedErr { t.Errorf("Expected error '%s', but got error '%s'", expectedErr, observedErr) @@ -861,11 +861,11 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { func TestCorruptedMessageWrongLength(t *testing.T) { // Decrypt message with wrong length in Literal packet header (length too long) // Expect parsing errors over unverified decrypted data to be opaque - var expectedErr string = "openpgp: decryption with session key failed: parsing error" passphrase := []byte("password") promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } + const expectedErr string = "openpgp: decryption with session key failed: parsing error" file, err := os.Open("../test_data/sym-corrupted-message-long-length.asc") if err != nil { From e24a6a1b992cde4c720b5c273662f4cc017f5396 Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Tue, 12 Nov 2024 12:32:16 +0100 Subject: [PATCH 5/8] feat: Unify mdc integrity error --- openpgp/errors/errors.go | 6 ++++-- openpgp/packet/symmetrically_encrypted_mdc.go | 6 +++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/openpgp/errors/errors.go b/openpgp/errors/errors.go index b00892412..c1b21cb46 100644 --- a/openpgp/errors/errors.go +++ b/openpgp/errors/errors.go @@ -20,8 +20,10 @@ var ( ErrAEADTagVerification error = DecryptWithSessionKeyError("AEAD tag verification failed") // ErrMDCHashMismatch is returned if the final tag verification in SEIPDv1 fails ErrMDCHashMismatch error = DecryptWithSessionKeyError("MDC hash mismatch") - // ErrMDCMissing is returned if the final tag in SEIPDv1 is missing - ErrMDCMissing error = DecryptWithSessionKeyError("MDC packet not found") + // ErrMDCMissing is deprecated and is no longer returned. + // Instead, if the MDC tag is missing, an ErrMDCHashMismatch error will be returned. + // Reduces the risk of decryption oracles. + ErrMDCMissing error = SignatureError("MDC packet not found") ) // A StructuralError is returned when OpenPGP data is found to be syntactically diff --git a/openpgp/packet/symmetrically_encrypted_mdc.go b/openpgp/packet/symmetrically_encrypted_mdc.go index 0a3aecadf..8b1862368 100644 --- a/openpgp/packet/symmetrically_encrypted_mdc.go +++ b/openpgp/packet/symmetrically_encrypted_mdc.go @@ -148,7 +148,7 @@ const mdcPacketTagByte = byte(0x80) | 0x40 | 19 func (ser *seMDCReader) Close() error { if ser.error { - return errors.ErrMDCMissing + return errors.ErrMDCHashMismatch } for !ser.eof { @@ -159,7 +159,7 @@ func (ser *seMDCReader) Close() error { break } if err != nil { - return errors.ErrMDCMissing + return errors.ErrMDCHashMismatch } } @@ -172,7 +172,7 @@ func (ser *seMDCReader) Close() error { // The hash already includes the MDC header, but we still check its value // to confirm encryption correctness if ser.trailer[0] != mdcPacketTagByte || ser.trailer[1] != sha1.Size { - return errors.ErrMDCMissing + return errors.ErrMDCHashMismatch } return nil } From 8fb82f9de3f043ee220f698da06e64446dff3ccf Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Tue, 12 Nov 2024 15:31:57 +0100 Subject: [PATCH 6/8] feat: Add aead test for missing last auth tag --- openpgp/packet/symmetric_key_encrypted_data_test.go | 9 ++++++--- openpgp/packet/symmetric_key_encrypted_test.go | 10 ++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/openpgp/packet/symmetric_key_encrypted_data_test.go b/openpgp/packet/symmetric_key_encrypted_data_test.go index 04a5ff890..6aada24db 100644 --- a/openpgp/packet/symmetric_key_encrypted_data_test.go +++ b/openpgp/packet/symmetric_key_encrypted_data_test.go @@ -4,9 +4,10 @@ package packet // by an integrity protected packet (SEIPD v1 or v2). type packetSequence struct { - password string - packets string - contents string + password string + packets string + contents string + faultyDataPacket string } func keyAndIpePackets() []*packetSequence { @@ -35,6 +36,8 @@ var symEncRFC9580 = &packetSequence{ password: "password", packets: "c340061e07010b0308a5ae579d1fc5d82bff69224f919993b3506fa3b59a6a73cff8c5efc5f41c57fb54e1c226815d7828f5f92c454eb65ebe00ab5986c68e6e7c55d269020701069ff90e3b321964f3a42913c8dcc6619325015227efb7eaeaa49f04c2e674175d4a3d226ed6afcb9ca9ac122c1470e11c63d4c0ab241c6a938ad48bf99a5a99b90bba8325de61047540258ab7959a95ad051dda96eb15431dfef5f5e2255ca78261546e339a", contents: "cb1362000000000048656c6c6f2c20776f726c6421d50eae5bf0cd6705500355816cb0c8ff", + // Missing last authentication chunk + faultyDataPacket: "d259020701069ff90e3b321964f3a42913c8dcc6619325015227efb7eaeaa49f04c2e674175d4a3d226ed6afcb9ca9ac122c1470e11c63d4c0ab241c6a938ad48bf99a5a99b90bba8325de61047540258ab7959a95ad051dda96eb", } // From the OpenPGP interoperability test suite (Test: S2K mechanisms, iterated min + esk) diff --git a/openpgp/packet/symmetric_key_encrypted_test.go b/openpgp/packet/symmetric_key_encrypted_test.go index 8271b0c24..c96f13ec5 100644 --- a/openpgp/packet/symmetric_key_encrypted_test.go +++ b/openpgp/packet/symmetric_key_encrypted_test.go @@ -73,6 +73,16 @@ func TestTagVerificationError(t *testing.T) { packet = readSymmetricallyEncrypted(t, dataPacket) // Decrypt contents and check integrity checkIntegrityError(t, packet, cipherFunc, key) + + if len(testCase.faultyDataPacket) > 0 { + dataPacket, err := hex.DecodeString(testCase.faultyDataPacket) + if err != nil { + t.Fatal(err) + } + packet = readSymmetricallyEncrypted(t, dataPacket) + // Decrypt contents and check integrity + checkIntegrityError(t, packet, cipherFunc, key) + } } } From 9e86d7795ac3ec44d3e820235dfc9293da3e19ee Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Thu, 14 Nov 2024 14:15:56 +0100 Subject: [PATCH 7/8] docs: Add comments when handling parsing errors --- openpgp/read.go | 8 ++++++++ openpgp/v2/read.go | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/openpgp/read.go b/openpgp/read.go index 629a0b741..44710ffd5 100644 --- a/openpgp/read.go +++ b/openpgp/read.go @@ -234,8 +234,12 @@ FindKey: mdFinal, sensitiveParsingErr := readSignedMessage(packets, md, keyring, config) if sensitiveParsingErr != nil { if md.decrypted != nil { + // The data is read from a stream that decrypts using a session key; + // therefore, we need to handle parsing errors appropriately. + // It's essential to mitigate the risk of oracle attacks. return nil, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingErr) } + // Data was not encrypted and is directly read in plaintext. return nil, errors.StructuralError(errors.GenericParsingErrorMessage) } return mdFinal, nil @@ -447,8 +451,12 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { if sensitiveParsingError != nil { if scr.md.decrypted != nil { + // The data is read from a stream that decrypts using a session key; + // therefore, we need to handle parsing errors appropriately. + // This is essential to mitigate the risk of oracle attacks. return n, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingError) } + // Data was not encrypted and is directly read in plaintext. return n, errors.StructuralError(errors.GenericParsingErrorMessage) } diff --git a/openpgp/v2/read.go b/openpgp/v2/read.go index bfa049fef..03730fd7f 100644 --- a/openpgp/v2/read.go +++ b/openpgp/v2/read.go @@ -269,8 +269,12 @@ FindKey: mdFinal, sensitiveParsingErr := readSignedMessage(packets, md, keyring, config) if sensitiveParsingErr != nil { if md.decrypted != nil { + // The data is read from a stream that decrypts using a session key; + // therefore, we need to handle parsing errors appropriately. + // This is essential to mitigate the risk of oracle attacks. return nil, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingErr) } + // Data was not encrypted and is directly read in plaintext. return nil, errors.StructuralError(errors.GenericParsingErrorMessage) } return mdFinal, nil @@ -654,8 +658,12 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { if sensitiveParsingError != nil { if scr.md.decrypted != nil { + // The data is read from a stream that decrypts using a session key; + // therefore, we need to handle parsing errors appropriately. + // This is essential to mitigate the risk of oracle attacks. return n, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingError) } + // Data was not encrypted and is directly read in plaintext. return n, errors.StructuralError(errors.GenericParsingErrorMessage) } From c895e57f0f9d81b6117e005d6c9ba873d9996cd4 Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Thu, 14 Nov 2024 16:37:24 +0100 Subject: [PATCH 8/8] feat: Unify parsing errors in SEIPDv1 decryption --- openpgp/errors/errors.go | 32 ++++++++++--------- openpgp/read.go | 26 ++++------------ openpgp/v2/read.go | 67 +++++++++++++--------------------------- 3 files changed, 44 insertions(+), 81 deletions(-) diff --git a/openpgp/errors/errors.go b/openpgp/errors/errors.go index c1b21cb46..0eb3937b3 100644 --- a/openpgp/errors/errors.go +++ b/openpgp/errors/errors.go @@ -9,20 +9,15 @@ import ( "strconv" ) -const ( - // GenericParsingErrorMessage is a generic error message for parsing errors - // to reduce the risk of oracle attacks. - GenericParsingErrorMessage = "parsing error" -) - var ( + // ErrDecryptSessionKeyParsing is a generic error message for parsing errors in decrypted data + // to reduce the risk of oracle attacks. + ErrDecryptSessionKeyParsing = DecryptWithSessionKeyError("parsing error") // ErrAEADTagVerification is returned if one of the tag verifications in SEIPDv2 fails ErrAEADTagVerification error = DecryptWithSessionKeyError("AEAD tag verification failed") - // ErrMDCHashMismatch is returned if the final tag verification in SEIPDv1 fails - ErrMDCHashMismatch error = DecryptWithSessionKeyError("MDC hash mismatch") - // ErrMDCMissing is deprecated and is no longer returned. - // Instead, if the MDC tag is missing, an ErrMDCHashMismatch error will be returned. - // Reduces the risk of decryption oracles. + // ErrMDCHashMismatch + ErrMDCHashMismatch error = SignatureError("MDC hash mismatch") + // ErrMDCMissing ErrMDCMissing error = SignatureError("MDC packet not found") ) @@ -43,16 +38,23 @@ func (s DecryptWithSessionKeyError) Error() string { return "openpgp: decryption with session key failed: " + string(s) } -// HandleDecryptionSensitiveParsingError handles symmetric decryption errors. -// The function makes parsing errors generic to reduce the risk of oracle attacks. -func HandleDecryptionSensitiveParsingError(err error) error { +// HandleSensitiveParsingError handles parsing errors when reading data from potentially decrypted data. +// The function makes parsing errors generic to reduce the risk of oracle attacks in SEIPDv1. +func HandleSensitiveParsingError(err error, decrypted bool) error { + if !decrypted { + // Data was not encrypted so we return the inner error. + return err + } + // The data is read from a stream that decrypts using a session key; + // therefore, we need to handle parsing errors appropriately. + // This is essential to mitigate the risk of oracle attacks. if decError, ok := err.(*DecryptWithSessionKeyError); ok { return decError } if decError, ok := err.(DecryptWithSessionKeyError); ok { return decError } - return DecryptWithSessionKeyError(GenericParsingErrorMessage) + return ErrDecryptSessionKeyParsing } // UnsupportedError indicates that, although the OpenPGP data is valid, it diff --git a/openpgp/read.go b/openpgp/read.go index 44710ffd5..e6dd9b5fd 100644 --- a/openpgp/read.go +++ b/openpgp/read.go @@ -233,14 +233,7 @@ FindKey: } mdFinal, sensitiveParsingErr := readSignedMessage(packets, md, keyring, config) if sensitiveParsingErr != nil { - if md.decrypted != nil { - // The data is read from a stream that decrypts using a session key; - // therefore, we need to handle parsing errors appropriately. - // It's essential to mitigate the risk of oracle attacks. - return nil, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingErr) - } - // Data was not encrypted and is directly read in plaintext. - return nil, errors.StructuralError(errors.GenericParsingErrorMessage) + return nil, errors.HandleSensitiveParsingError(sensitiveParsingErr, md.decrypted != nil) } return mdFinal, nil } @@ -375,7 +368,7 @@ func (cr *checkReader) Read(buf []byte) (int, error) { } if sensitiveParsingError != nil { - return n, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingError) + return n, errors.HandleSensitiveParsingError(sensitiveParsingError, true) } return n, nil @@ -399,6 +392,7 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { scr.wrappedHash.Write(buf[:n]) } + readsDecryptedData := scr.md.decrypted != nil if sensitiveParsingError == io.EOF { var p packet.Packet var readError error @@ -441,23 +435,15 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { // unsigned hash of its own. In order to check this we need to // close that Reader. if scr.md.decrypted != nil { - mdcErr := scr.md.decrypted.Close() - if mdcErr != nil { - return n, mdcErr + if sensitiveParsingError := scr.md.decrypted.Close(); sensitiveParsingError != nil { + return n, errors.HandleSensitiveParsingError(sensitiveParsingError, true) } } return n, io.EOF } if sensitiveParsingError != nil { - if scr.md.decrypted != nil { - // The data is read from a stream that decrypts using a session key; - // therefore, we need to handle parsing errors appropriately. - // This is essential to mitigate the risk of oracle attacks. - return n, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingError) - } - // Data was not encrypted and is directly read in plaintext. - return n, errors.StructuralError(errors.GenericParsingErrorMessage) + return n, errors.HandleSensitiveParsingError(sensitiveParsingError, readsDecryptedData) } return n, nil diff --git a/openpgp/v2/read.go b/openpgp/v2/read.go index 03730fd7f..5ab9aff53 100644 --- a/openpgp/v2/read.go +++ b/openpgp/v2/read.go @@ -268,14 +268,7 @@ FindKey: } mdFinal, sensitiveParsingErr := readSignedMessage(packets, md, keyring, config) if sensitiveParsingErr != nil { - if md.decrypted != nil { - // The data is read from a stream that decrypts using a session key; - // therefore, we need to handle parsing errors appropriately. - // This is essential to mitigate the risk of oracle attacks. - return nil, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingErr) - } - // Data was not encrypted and is directly read in plaintext. - return nil, errors.StructuralError(errors.GenericParsingErrorMessage) + return nil, errors.HandleSensitiveParsingError(sensitiveParsingErr, md.decrypted != nil) } return mdFinal, nil } @@ -488,15 +481,15 @@ func (cr *checkReader) Read(buf []byte) (int, error) { } } if cr.md.decrypted != nil { - if mdcErr := cr.md.decrypted.Close(); mdcErr != nil { - return n, mdcErr + if sensitiveParsingError := cr.md.decrypted.Close(); sensitiveParsingError != nil { + return n, errors.HandleSensitiveParsingError(sensitiveParsingError, true) } } cr.checked = true return n, io.EOF } if sensitiveParsingError != nil { - return n, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingError) + return n, errors.HandleSensitiveParsingError(sensitiveParsingError, true) } return n, nil } @@ -520,18 +513,19 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { } } + readsDecryptedData := scr.md.decrypted != nil if sensitiveParsingError == io.EOF { - var signatures []*packet.Signature - // Read all signature packets. + var signatures []*packet.Signature + // Read all signature packets and discard others. for { - p, err := scr.packets.Next() - if err == io.EOF { + p, sensitiveParsingErrorPacket := scr.packets.Next() + if sensitiveParsingErrorPacket == io.EOF { break } - if err != nil { - return n, errors.StructuralError(errors.GenericParsingErrorMessage) + if sensitiveParsingErrorPacket != nil { + return n, errors.HandleSensitiveParsingError(sensitiveParsingErrorPacket, readsDecryptedData) } if sig, ok := p.(*packet.Signature); ok { if sig.Version == 5 && scr.md.LiteralData != nil && (sig.SigType == 0x00 || sig.SigType == 0x01) { @@ -540,6 +534,15 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { signatures = append(signatures, sig) } } + + // Verify the integrity of the decrypted data before verifying the signatures. + if scr.md.decrypted != nil { + sensitiveParsingErrorPacket := scr.md.decrypted.Close() + if sensitiveParsingErrorPacket != nil { + return n, errors.HandleSensitiveParsingError(sensitiveParsingErrorPacket, true) + } + } + numberOfOpsSignatures := 0 for _, candidate := range scr.md.SignatureCandidates { if candidate.CorrespondingSig == nil { @@ -632,39 +635,11 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) { } scr.md.IsVerified = true - - for { - _, err := scr.packets.Next() - if err == io.EOF { - break - } - if err != nil { - return 0, errors.StructuralError(errors.GenericParsingErrorMessage) - } - - } - - // The SymmetricallyEncrypted packet, if any, might have an - // unsigned hash of its own. In order to check this we need to - // close that Reader. - if scr.md.decrypted != nil { - mdcErr := scr.md.decrypted.Close() - if mdcErr != nil { - return n, mdcErr - } - } return n, io.EOF } if sensitiveParsingError != nil { - if scr.md.decrypted != nil { - // The data is read from a stream that decrypts using a session key; - // therefore, we need to handle parsing errors appropriately. - // This is essential to mitigate the risk of oracle attacks. - return n, errors.HandleDecryptionSensitiveParsingError(sensitiveParsingError) - } - // Data was not encrypted and is directly read in plaintext. - return n, errors.StructuralError(errors.GenericParsingErrorMessage) + return n, errors.HandleSensitiveParsingError(sensitiveParsingError, readsDecryptedData) } return n, nil