From 5e3e39db239450fc8ab5457cd1047a74b0dbc8d9 Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter <10532077+lubux@users.noreply.github.com> Date: Mon, 18 Nov 2024 09:29:10 +0100 Subject: [PATCH] Improve error message for decryption with a session key (#237) * feat: Improve error message for decryption with a session key * chore: Add explicit error message strings to tests * feat: Update SEIPD test vectors * chore: Move expected error to the old position * feat: Unify mdc integrity error * feat: Add aead test for missing last auth tag * docs: Add comments when handling parsing errors * feat: Unify parsing errors in SEIPDv1 decryption --- openpgp/errors/errors.go | 43 +++++- openpgp/packet/aead_crypter.go | 8 +- .../symmetric_key_encrypted_data_test.go | 28 ++-- .../packet/symmetric_key_encrypted_test.go | 131 +++++++++++++++--- openpgp/packet/symmetrically_encrypted_mdc.go | 6 +- openpgp/read.go | 12 +- openpgp/read_test.go | 4 +- openpgp/v2/read.go | 65 ++++----- openpgp/v2/read_test.go | 4 +- 9 files changed, 216 insertions(+), 85 deletions(-) diff --git a/openpgp/errors/errors.go b/openpgp/errors/errors.go index c42b01cb..0eb3937b 100644 --- a/openpgp/errors/errors.go +++ b/openpgp/errors/errors.go @@ -9,6 +9,18 @@ import ( "strconv" ) +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 + ErrMDCHashMismatch error = SignatureError("MDC hash mismatch") + // ErrMDCMissing + ErrMDCMissing error = SignatureError("MDC packet not found") +) + // A StructuralError is returned when OpenPGP data is found to be syntactically // invalid. type StructuralError string @@ -17,6 +29,34 @@ 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) +} + +// 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 ErrDecryptSessionKeyParsing +} + // UnsupportedError indicates that, although the OpenPGP data is valid, it // makes use of currently unimplemented features. type UnsupportedError string @@ -41,9 +81,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 7171387f..2eecd062 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_data_test.go b/openpgp/packet/symmetric_key_encrypted_data_test.go index d8cf294f..6aada24d 100644 --- a/openpgp/packet/symmetric_key_encrypted_data_test.go +++ b/openpgp/packet/symmetric_key_encrypted_data_test.go @@ -4,16 +4,17 @@ 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 { 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 +31,18 @@ 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", + // Missing last authentication chunk + faultyDataPacket: "d259020701069ff90e3b321964f3a42913c8dcc6619325015227efb7eaeaa49f04c2e674175d4a3d226ed6afcb9ca9ac122c1470e11c63d4c0ab241c6a938ad48bf99a5a99b90bba8325de61047540258ab7959a95ad051dda96eb", +} + +// 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 6bb1f2a3..c96f13ec 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,110 @@ 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) + + 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) + } + } +} + +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 { + err = data.Close() + } + 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/packet/symmetrically_encrypted_mdc.go b/openpgp/packet/symmetrically_encrypted_mdc.go index 0a3aecad..8b186236 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 } diff --git a/openpgp/read.go b/openpgp/read.go index 8a69b44a..e6dd9b5f 100644 --- a/openpgp/read.go +++ b/openpgp/read.go @@ -233,7 +233,7 @@ FindKey: } mdFinal, sensitiveParsingErr := readSignedMessage(packets, md, keyring, config) if sensitiveParsingErr != nil { - return nil, errors.StructuralError("parsing error") + return nil, errors.HandleSensitiveParsingError(sensitiveParsingErr, md.decrypted != nil) } return mdFinal, nil } @@ -368,7 +368,7 @@ func (cr *checkReader) Read(buf []byte) (int, error) { } if sensitiveParsingError != nil { - return n, errors.StructuralError("parsing error") + return n, errors.HandleSensitiveParsingError(sensitiveParsingError, true) } return n, nil @@ -392,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 @@ -434,16 +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 { - return n, errors.StructuralError("parsing error") + return n, errors.HandleSensitiveParsingError(sensitiveParsingError, readsDecryptedData) } return n, nil diff --git a/openpgp/read_test.go b/openpgp/read_test.go index a9846cbc..23fd4aec 100644 --- a/openpgp/read_test.go +++ b/openpgp/read_test.go @@ -819,7 +819,7 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } - const expectedErr string = "openpgp: invalid data: parsing error" + 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) @@ -833,7 +833,7 @@ func TestCorruptedMessageWrongLength(t *testing.T) { promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } - const expectedErr string = "openpgp: invalid data: parsing error" + 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.go b/openpgp/v2/read.go index 24c8c8f0..5ab9aff5 100644 --- a/openpgp/v2/read.go +++ b/openpgp/v2/read.go @@ -268,7 +268,7 @@ FindKey: } mdFinal, sensitiveParsingErr := readSignedMessage(packets, md, keyring, config) if sensitiveParsingErr != nil { - return nil, errors.StructuralError("parsing error") + return nil, errors.HandleSensitiveParsingError(sensitiveParsingErr, md.decrypted != nil) } return mdFinal, nil } @@ -481,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.StructuralError("parsing error") + return n, errors.HandleSensitiveParsingError(sensitiveParsingError, true) } return n, nil } @@ -513,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("parsing error") + 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) { @@ -533,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 { @@ -625,32 +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("parsing error") - } - - } - - // 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 { - return n, errors.StructuralError("parsing error") + return n, errors.HandleSensitiveParsingError(sensitiveParsingError, readsDecryptedData) } return n, nil @@ -741,12 +730,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 25926d81..24eec2f4 100644 --- a/openpgp/v2/read_test.go +++ b/openpgp/v2/read_test.go @@ -851,7 +851,7 @@ func TestCorruptedMessageInvalidSigHeader(t *testing.T) { promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } - const expectedErr string = "openpgp: invalid data: parsing error" + 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) @@ -865,7 +865,7 @@ func TestCorruptedMessageWrongLength(t *testing.T) { promptFunc := func(keys []Key, symmetric bool) ([]byte, error) { return passphrase, nil } - const expectedErr string = "openpgp: invalid data: parsing error" + 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 {