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

Author: lubux

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 {

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_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",
 }

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,

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
 }

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

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 {