diff options
author | Justus Winter <justus@sequoia-pgp.org> | 2021-03-16 15:11:25 +0100 |
---|---|---|
committer | Justus Winter <justus@sequoia-pgp.org> | 2021-03-17 19:08:44 +0100 |
commit | 5140997e88b191eb7108e47ec1a3bdf7b129cd9b (patch) | |
tree | c1bb5a86361760920eaf3e676197ee8f84df3f5d /openpgp | |
parent | 711d29ebff65aa08c06898d91a2db4ecbcdbee67 (diff) |
openpgp: Fix handling of malformed MDC packets.
- Tampering with MDC packets can be used to create decryption
oracles. To defend against that, we need to respond with uniform
error messages.
- Thanks to Lara Bruseghini for bringing this to our attention.
- Fixes #693.
Diffstat (limited to 'openpgp')
-rw-r--r-- | openpgp/src/parse/stream.rs | 172 |
1 files changed, 161 insertions, 11 deletions
diff --git a/openpgp/src/parse/stream.rs b/openpgp/src/parse/stream.rs index 31029f08..ad1318c5 100644 --- a/openpgp/src/parse/stream.rs +++ b/openpgp/src/parse/stream.rs @@ -606,15 +606,39 @@ impl IMessageStructure { }); } - fn new_encryption_layer(&mut self, sym_algo: SymmetricAlgorithm, + fn new_encryption_layer(&mut self, + depth: isize, + expect_mdc: bool, + sym_algo: SymmetricAlgorithm, aead_algo: Option<AEADAlgorithm>) { self.insert_missing_signature_group(); self.layers.push(IMessageLayer::Encryption { + depth, + expect_mdc, sym_algo: sym_algo, aead_algo: aead_algo, }); } + /// Returns whether or not we expect an MDC packet in an + /// encryption container at this recursion depth. + /// + /// Handling MDC packets has to be done carefully, otherwise, we + /// may create a decryption oracle. + fn expect_mdc_at(&self, at: isize) -> bool { + for l in &self.layers { + match l { + IMessageLayer::Encryption { + depth, + expect_mdc, + .. + } if *depth == at && *expect_mdc => return true, + _ => (), + } + } + false + } + /// Makes sure that we insert a signature group even if the /// previous OPS packet had the last flag set to false. fn insert_missing_signature_group(&mut self) { @@ -687,6 +711,12 @@ enum IMessageLayer { algo: CompressionAlgorithm, }, Encryption { + /// Recursion depth of this container. + depth: isize, + /// Do we expect an MDC packet? + /// + /// I.e. is this a SEIP container? + expect_mdc: bool, sym_algo: SymmetricAlgorithm, aead_algo: Option<AEADAlgorithm>, }, @@ -2396,6 +2426,8 @@ impl<'a, H: VerificationHelper + DecryptionHelper> Decryptor<'a, H> { } v.structure.new_encryption_layer( + pp.recursion_depth(), + pp.packet.tag() == packet::Tag::SEIP, sym_algo, if let Packet::AED(ref p) = pp.packet { Some(p.aead()) @@ -2568,21 +2600,55 @@ impl<'a, H: VerificationHelper + DecryptionHelper> Decryptor<'a, H> { self.helper.inspect(&pp)?; } - if let Err(err) = pp.possible_message() { - return Err(err.context( - "Malformed OpenPGP message").into()); - } + let possible_message = pp.possible_message(); + + // If we are ascending, and the packet was the + // last packet in a SEIP container, we need to be + // extra careful with reporting errors to avoid + // creating a decryption oracle. + + let last_recursion_depth = pp.recursion_depth(); + let (p, ppr_tmp) = match pp.recurse() { + Ok(v) => v, + Err(e) => { + // Assuming we just tried to ascend, + // should there have been a MDC packet? + // If so, this may be an attack. + if self.structure.expect_mdc_at( + last_recursion_depth - 1) + { + return Err(Error::ManipulatedMessage.into()); + } else { + return Err(e); + } + }, + }; + ppr = ppr_tmp; + let recursion_depth = ppr.as_ref() + .map(|pp| pp.recursion_depth()).unwrap_or(0); - match pp.packet { - Packet::MDC(ref mdc) => if ! mdc.valid() { + // Did we just ascend? + if recursion_depth + 1 == last_recursion_depth + && self.structure.expect_mdc_at(recursion_depth) + { + match &p { + Packet::MDC(mdc) if mdc.valid() => + (), // Good. + _ => // Bad. + return Err(Error::ManipulatedMessage.into()), + } + + if possible_message.is_err() { return Err(Error::ManipulatedMessage.into()); } - _ => (), } - let (p, ppr_tmp) = pp.recurse()?; + if let Err(err) = possible_message { + return Err(err.context( + "Malformed OpenPGP message").into()); + } + self.push_sig(p)?; - ppr = ppr_tmp; } self.verify_signatures() @@ -2608,7 +2674,7 @@ impl<'a, H: VerificationHelper + DecryptionHelper> Decryptor<'a, H> { match layer { IMessageLayer::Compression { algo } => results.new_compression_layer(*algo), - IMessageLayer::Encryption { sym_algo, aead_algo } => + IMessageLayer::Encryption { sym_algo, aead_algo, .. } => results.new_encryption_layer(*sym_algo, *aead_algo), IMessageLayer::SignatureGroup { sigs, .. } => { results.new_signature_group(); @@ -3451,4 +3517,88 @@ mod test { assert!(v.helper_ref().error == 0); Ok(()) } + + /// Checks that tampering with the MDC yields a uniform error + /// response. + #[test] + fn issue_693() -> Result<()> { + struct H(); + impl VerificationHelper for H { + fn get_certs(&mut self, _ids: &[crate::KeyHandle]) + -> Result<Vec<Cert>> { + Ok(Vec::new()) + } + + fn check(&mut self, _: MessageStructure) + -> Result<()> { + Ok(()) + } + } + impl DecryptionHelper for H { + fn decrypt<D>(&mut self, _: &[PKESK], s: &[SKESK], + _: Option<SymmetricAlgorithm>, mut decrypt: D) + -> Result<Option<Fingerprint>> + where D: FnMut(SymmetricAlgorithm, &SessionKey) -> bool + { + let (algo, sk) = s[0].decrypt(&"123".into()).unwrap(); + let r = decrypt(algo, &sk); + assert!(r); + Ok(None) + } + } + + fn check(m: &str) -> Result<()> { + let doit = || -> Result<()> { + let p = &P::new(); + let mut decryptor = DecryptorBuilder::from_bytes(m.as_bytes())? + .with_policy(p, None, H())?; + let mut b = Vec::new(); + decryptor.read_to_end(&mut b)?; + Ok(()) + }; + + let e = doit().unwrap_err(); + match e.downcast::<io::Error>() { + Ok(e) => + assert_eq!(e.into_inner().unwrap().downcast().unwrap(), + Box::new(Error::ManipulatedMessage)), + Err(e) => + assert_eq!(e.downcast::<Error>().unwrap(), + Error::ManipulatedMessage), + }; + Ok(()) + } + + // Bad hash. + check("-----BEGIN PGP MESSAGE----- + +wx4EBwMI7dKRUiOYGCUAWmzhiYGS8Pn/16QkyTous6vSOgFMcilte26C7kej +rKhvjj6uYNT+mt+L2Yg/FHFvpgVF3KfP0fb+9jZwgt4qpDkTMY7AWPTK6wXX +Jo8= +=LS8u +-----END PGP MESSAGE----- +")?; + + // Bad header. + check("-----BEGIN PGP MESSAGE----- + +wx4EBwMI7sPTdlgQwd8AogIcbF/hLVrYbvVbgj4EC6/SOgGNaCyffrR4Fuwl +Ft2w56/hB/gTaGEhCgDGXg8NiFGIURqF3eIwxxdKWghUutYmsGwqOZmdJ49a +9gE= +=DzKF +-----END PGP MESSAGE----- +")?; + + // Bad header matching other packet type. + check("-----BEGIN PGP MESSAGE----- + +wx4EBwMIhpEGBh3v0oMAYgGcj+4CG1mcWQwmyGIDRdvSOgFSHlL2GZ1ZKeXS +29kScqGg2U8N6ZF9vmj/9Sn7CFtO5PGXn2owQVsopeUSTofV3BNUBpxaBDCO +EK8= +=TgeJ +-----END PGP MESSAGE----- +")?; + + Ok(()) + } } |