From 89db31a4121079a54f09b87854e65887325b9e70 Mon Sep 17 00:00:00 2001 From: Lukas Burkhalter Date: Thu, 14 Nov 2024 16:37:24 +0100 Subject: [PATCH] feat: Unify parsing errors in SEIPDv1 decryption --- openpgp/errors/errors.go | 26 +++++++++------- openpgp/read.go | 26 ++++------------ openpgp/v2/read.go | 67 +++++++++++++--------------------------- 3 files changed, 42 insertions(+), 77 deletions(-) diff --git a/openpgp/errors/errors.go b/openpgp/errors/errors.go index c1b21cb4..05a28895 100644 --- a/openpgp/errors/errors.go +++ b/openpgp/errors/errors.go @@ -9,17 +9,14 @@ 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") + ErrMDCHashMismatch error = SignatureError("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. @@ -43,16 +40,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 index 44710ffd..e6dd9b5f 100644 --- 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 index 03730fd7..5ab9aff5 100644 --- 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