diff --git a/openpgp/errors/errors.go b/openpgp/errors/errors.go index c42b01cb..7e33c886 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 +// the final authentication tag verification fails. +// Such an error indicates that the supplied session key is likely wrong. +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 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_test.go b/openpgp/packet/symmetric_key_encrypted_test.go index 6bb1f2a3..ae7ba671 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,98 @@ 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: + // Skip AEADEncrypted packets, as instructed in the original logic. + 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 8a69b44a..629a0b74 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 a9846cbc..e8867a3f 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 24c8c8f0..bfa049fe 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 25926d81..9c8b6cff 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 {