feat: Improve error message for decryption with a session key

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 {

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

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,

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

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 {

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())

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 {