feat: Unify parsing errors in SEIPDv1 decryption

ProtonMail · Nov 18, 2024 · b13d2fd · b13d2fd
1 parent 6e34c69
commit b13d2fd
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 81 deletions.
diff --git 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
@@ -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
@@ -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