From 06ee86f3752f56add4d6e9deab1c460640ced21c Mon Sep 17 00:00:00 2001 From: Justus Winter Date: Fri, 28 Sep 2018 15:43:53 +0200 Subject: openpgp: Introduce and use a type holding passwords. - Fixes #108. --- ffi/src/openpgp.rs | 10 +++++---- openpgp/src/autocrypt.rs | 37 ++++++++++++++++++--------------- openpgp/src/key.rs | 7 ++++--- openpgp/src/lib.rs | 46 +++++++++++++++++++++++++++++++++++++++++ openpgp/src/message/mod.rs | 2 +- openpgp/src/parse/parse.rs | 7 ++++--- openpgp/src/parse/stream.rs | 7 +++---- openpgp/src/s2k.rs | 23 +++++++++++---------- openpgp/src/serialize/stream.rs | 14 +++++++------ openpgp/src/skesk.rs | 5 +++-- tool/src/commands/decrypt.rs | 7 ++++--- tool/src/commands/mod.rs | 6 +++--- 12 files changed, 114 insertions(+), 57 deletions(-) diff --git a/ffi/src/openpgp.rs b/ffi/src/openpgp.rs index a5fe6d46..19525422 100644 --- a/ffi/src/openpgp.rs +++ b/ffi/src/openpgp.rs @@ -11,7 +11,9 @@ use libc::{self, uint8_t, uint64_t, c_char, c_int, size_t, ssize_t}; extern crate openpgp; -use self::openpgp::{armor, Fingerprint, KeyID, PacketPile, TPK, TSK, Packet}; +use self::openpgp::{ + armor, Fingerprint, KeyID, PacketPile, TPK, TSK, Packet, Password, +}; use self::openpgp::tpk::{CipherSuite, TPKBuilder}; use self::openpgp::parse::{PacketParserResult, PacketParser, PacketParserEOF}; use self::openpgp::serialize::Serialize; @@ -1123,7 +1125,7 @@ pub extern "system" fn sq_skesk_decrypt(ctx: Option<&mut Context>, let key_len = key_len.expect("Key length is NULL"); if let &Packet::SKESK(ref skesk) = skesk { - match skesk.decrypt(password) { + match skesk.decrypt(&password.to_owned().into()) { Ok((a, k)) => { *algo = a.into(); if !key.is_null() && *key_len >= k.len() { @@ -1733,7 +1735,7 @@ pub extern "system" fn sq_encryptor_new for password in passwords { passwords_.push(unsafe { CStr::from_ptr(*password) - }.to_bytes()); + }.to_bytes().to_owned().into()); } } let recipients = if recipients_len > 0 { @@ -1750,7 +1752,7 @@ pub extern "system" fn sq_encryptor_new _ => panic!("Bad encryption mode: {}", encryption_mode), }; fry_box!(ctx, Encryptor::new(*inner, - &passwords_, + &passwords_.iter().collect::>(), &recipients, encryption_mode)) } diff --git a/openpgp/src/autocrypt.rs b/openpgp/src/autocrypt.rs index 4aed6934..c4bfe162 100644 --- a/openpgp/src/autocrypt.rs +++ b/openpgp/src/autocrypt.rs @@ -34,6 +34,7 @@ use serialize::stream::{ wrap, LiteralWriter, Encryptor, EncryptionMode, }; use constants::DataFormat; +use Password; /// Version of Autocrypt to use. `Autocrypt::default()` always returns the /// latest version. @@ -245,7 +246,7 @@ impl AutocryptHeaders { pub struct AutocryptSetupMessage { prefer_encrypt: Option, passcode_format: Option, - passcode: Option>, + passcode: Option, // We only emit a "Passcode-Begin" header if this is set. Note: // we don't check if this actually matches the start of the // passcode. @@ -300,8 +301,8 @@ impl AutocryptSetupMessage { /// Sets the passcode. - pub fn set_passcode(mut self, passcode: &[u8]) -> Self { - self.passcode = Some(passcode.into()); + pub fn set_passcode(mut self, passcode: Password) -> Self { + self.passcode = Some(passcode); self } @@ -309,8 +310,8 @@ impl AutocryptSetupMessage { /// /// If the passcode has not yet been generated, this returns /// `None`. - pub fn passcode(&self) -> Option<&[u8]> { - self.passcode.as_ref().map(|p| &p[..]) + pub fn passcode(&self) -> Option<&Password> { + self.passcode.as_ref() } @@ -327,7 +328,7 @@ impl AutocryptSetupMessage { // Generates a new passcode in "numeric9x4" format. - fn passcode_gen() -> Vec { + fn passcode_gen() -> Password { use nettle::Yarrow; // Generate a random passcode. @@ -339,11 +340,12 @@ impl AutocryptSetupMessage { let mut p_as_vec = vec![0; 15]; rng.random(&mut p_as_vec[..]); + let p = Password::from(p_as_vec); // Turn it into a 128-bit number. let mut p_as_u128 = 0u128; - for v in p_as_vec.into_iter() { - p_as_u128 = (p_as_u128 << 8) + v as u128; + for v in p.iter() { + p_as_u128 = (p_as_u128 << 8) + *v as u128; } // Turn it into ASCII. @@ -357,7 +359,7 @@ impl AutocryptSetupMessage { p_as_u128 = p_as_u128 / 10; } - p + p.into() } /// If there is no passcode, generates one. @@ -542,7 +544,7 @@ pub struct AutocryptSetupMessageParser<'a> { passcode_begin: Option, skesk: SKESK, pp: PacketParser<'a>, - passcode: Option>, + passcode: Option, } impl<'a> AutocryptSetupMessageParser<'a> { @@ -561,7 +563,7 @@ impl<'a> AutocryptSetupMessageParser<'a> { /// On success, follow up with /// `AutocryptSetupMessageParser::parse()` to extract the /// `AutocryptSetupMessage`. - pub fn decrypt(&mut self, passcode: &[u8]) -> Result<()> { + pub fn decrypt(&mut self, passcode: &Password) -> Result<()> { if self.pp.decrypted() { return Err( Error::InvalidOperation("Already decrypted".into()).into()); @@ -570,7 +572,7 @@ impl<'a> AutocryptSetupMessageParser<'a> { let (algo, key) = self.skesk.decrypt(passcode)?; self.pp.decrypt(algo, &key)?; - self.passcode = Some(passcode.into()); + self.passcode = Some(passcode.clone()); Ok(()) } @@ -912,10 +914,10 @@ In the light of the Efail vulnerability I am asking myself if it's let p = AutocryptSetupMessage::passcode_gen(); assert_eq!(p.len(), passcode_len); - for c in p.into_iter() { - match c as char { + for c in p.iter() { + match *c as char { '0'|'1'|'2'|'3'|'4'|'5'|'6'|'7'|'8'|'9' => { - let i = c as usize - ('0' as usize); + let i = *c as usize - ('0' as usize); dist[i] = dist[i] + 1 }, '-' => (), @@ -968,10 +970,11 @@ In the light of the Efail vulnerability I am asking myself if it's bytes!("autocrypt/setup-message.txt")).unwrap(); // A bad passcode. - assert!(asm.decrypt(&b"123"[..]).is_err()); + assert!(asm.decrypt(&"123".into()).is_err()); // Now the right one. assert!(asm.decrypt( - &b"1742-0185-6197-1303-7016-8412-3581-4441-0597"[..]).is_ok()); + &"1742-0185-6197-1303-7016-8412-3581-4441-0597".into() + ).is_ok()); let asm = asm.parse().unwrap(); // A basic check to make sure we got the key. diff --git a/openpgp/src/key.rs b/openpgp/src/key.rs index 2fb69029..216d6b21 100644 --- a/openpgp/src/key.rs +++ b/openpgp/src/key.rs @@ -11,6 +11,7 @@ use SymmetricAlgorithm; use s2k::S2K; use Result; use conversions::Time; +use Password; /// Holds a public key, public subkey, private key or private subkey packet. /// @@ -250,7 +251,7 @@ impl SecretKey { /// /// The SecretKey type does not know what kind of key it is, so /// `pk_algo` is needed to parse the correct number of MPIs. - pub fn decrypt(&self, pk_algo: PublicKeyAlgorithm, password: &[u8]) + pub fn decrypt(&self, pk_algo: PublicKeyAlgorithm, password: &Password) -> Result { use std::io::{Cursor, Read}; use symmetric::Decryptor; @@ -275,7 +276,7 @@ impl SecretKey { /// The SecretKey type does not know what kind of key it is, so /// `pk_algo` is needed to parse the correct number of MPIs. pub fn decrypt_in_place(&mut self, pk_algo: PublicKeyAlgorithm, - password: &[u8]) + password: &Password) -> Result<()> { if self.is_encrypted() { *self = SecretKey::Unencrypted { @@ -318,7 +319,7 @@ mod tests { let secret = pair.secret.as_mut().unwrap(); assert!(secret.is_encrypted()); - secret.decrypt_in_place(pair.pk_algo, &b"123"[..]).unwrap(); + secret.decrypt_in_place(pair.pk_algo, &"123".into()).unwrap(); assert!(!secret.is_encrypted()); match secret { diff --git a/openpgp/src/lib.rs b/openpgp/src/lib.rs index 8cf34296..fe1ff8e4 100644 --- a/openpgp/src/lib.rs +++ b/openpgp/src/lib.rs @@ -554,6 +554,52 @@ impl Drop for SessionKey { } } } + +/// Holds a password. +/// +/// The password is cleared when dropped. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Password(Box<[u8]>); + +impl Deref for Password { + type Target = [u8]; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl From> for Password { + fn from(v: Vec) -> Self { + Password(v.into_boxed_slice()) + } +} + +impl From> for Password { + fn from(v: Box<[u8]>) -> Self { + Password(v) + } +} + +impl From for Password { + fn from(v: String) -> Self { + v.into_bytes().into() + } +} + +impl<'a> From<&'a str> for Password { + fn from(v: &'a str) -> Self { + v.to_owned().into() + } +} + +impl Drop for Password { + fn drop(&mut self) { + unsafe { + memsec::memzero(self.0.as_mut_ptr(), self.0.len()); + } + } +} use std::io::Read; use nettle::Hash; diff --git a/openpgp/src/message/mod.rs b/openpgp/src/message/mod.rs index 572a812e..8b7f96a5 100644 --- a/openpgp/src/message/mod.rs +++ b/openpgp/src/message/mod.rs @@ -893,7 +893,7 @@ mod tests { packets.push(SKESK::new(SymmetricAlgorithm::AES256, S2K::Simple { hash: HashAlgorithm::SHA256 }, &sk, - &b"12345678"[..]).unwrap().to_packet()); + &"12345678".into()).unwrap().to_packet()); let message = Message::from_packets(packets.clone()); assert!(message.is_err(), "{:?}", message); diff --git a/openpgp/src/parse/parse.rs b/openpgp/src/parse/parse.rs index a8029e9c..8f3d51a5 100644 --- a/openpgp/src/parse/parse.rs +++ b/openpgp/src/parse/parse.rs @@ -1723,11 +1723,12 @@ impl MDC { #[test] fn skesk_parser_test() { + use Password; struct Test<'a> { filename: &'a str, s2k: S2K, cipher_algo: SymmetricAlgorithm, - password: &'a [u8], + password: Password, key_hex: &'a str, }; @@ -1740,7 +1741,7 @@ fn skesk_parser_test() { salt: [0x82, 0x59, 0xa0, 0x6e, 0x98, 0xda, 0x94, 0x1c], iterations: S2K::decode_count(238), }, - password: &b"bgtyhn"[..], + password: "bgtyhn".into(), key_hex: "474E5C373BA18AF0A499FCAFE6093F131DF636F6A3812B9A8AE707F1F0214AE9", }, ]; @@ -1754,7 +1755,7 @@ fn skesk_parser_test() { assert_eq!(skesk.symm_algo, test.cipher_algo); assert_eq!(skesk.s2k, test.s2k); - match skesk.decrypt(test.password) { + match skesk.decrypt(&test.password) { Ok((_symm_algo, key)) => { let key = ::conversions::to_hex(&key[..], false); assert_eq!(&key[..], &test.key_hex[..]); diff --git a/openpgp/src/parse/stream.rs b/openpgp/src/parse/stream.rs index 0b4238f5..b9b54ad8 100644 --- a/openpgp/src/parse/stream.rs +++ b/openpgp/src/parse/stream.rs @@ -23,6 +23,7 @@ use { packet::Signature, TPK, mpis, + Password, SessionKey, }; use parse::{ @@ -584,7 +585,7 @@ pub enum Secret { /// A password for symmetric decryption. Symmetric { /// The password. - password: String, + password: Password, }, /// A cached session key. @@ -703,10 +704,8 @@ impl<'a, H: VerificationHelper + DecryptionHelper> Decryptor<'a, H> { }, Secret::Symmetric { ref password } => { - let pass = password.as_bytes(); - for skesk in skesks.iter() { - let (algo, key) = skesk.decrypt(pass)?; + let (algo, key) = skesk.decrypt(password)?; if pp.decrypt(algo, &key).is_ok() { decrypted = true; diff --git a/openpgp/src/s2k.rs b/openpgp/src/s2k.rs index e1d32211..4a6e8da3 100644 --- a/openpgp/src/s2k.rs +++ b/openpgp/src/s2k.rs @@ -9,6 +9,7 @@ use Error; use Result; use HashAlgorithm; +use Password; use SessionKey; use std::fmt; @@ -68,7 +69,7 @@ impl Default for S2K { impl S2K { /// Convert the string to a key using the S2K's paramters. - pub fn derive_key(&self, string: &[u8], key_size: usize) + pub fn derive_key(&self, string: &Password, key_size: usize) -> Result { match self { &S2K::Simple { hash } | &S2K::Salted { hash, .. } @@ -276,7 +277,7 @@ mod tests { filename: &'a str, s2k: S2K, cipher_algo: SymmetricAlgorithm, - password: &'a [u8], + password: Password, key_hex: &'a str, }; @@ -291,7 +292,7 @@ mod tests { filename: "mode-0-password-1234.gpg", cipher_algo: SymmetricAlgorithm::AES256, s2k: S2K::Simple{ hash: HashAlgorithm::SHA1, }, - password: &b"1234"[..], + password: "1234".into(), key_hex: "7110EDA4D09E062AA5E4A390B0A572AC0D2C0220F352B0D292B65164C2A67301", }, Test { @@ -301,7 +302,7 @@ mod tests { hash: HashAlgorithm::SHA1, salt: [0xa8, 0x42, 0xa7, 0xa9, 0x59, 0xfa, 0x42, 0x2a], }, - password: &b"123456"[..], + password: "123456".into(), key_hex: "8B79077CA448F6FB3D3AD2A264D3B938D357C9FB3E41219FD962DF960A9AFA08", }, Test { @@ -311,7 +312,7 @@ mod tests { hash: HashAlgorithm::SHA1, salt: [0xbc, 0x95, 0x58, 0x45, 0x81, 0x3c, 0x7c, 0x37], }, - password: &b"foobar"[..], + password: "foobar".into(), key_hex: "B7D48AAE9B943B22A4D390083E8460B5EDFA118FE1688BF0C473B8094D1A8D10", }, Test { @@ -322,7 +323,7 @@ mod tests { salt: [0x78, 0x45, 0xf0, 0x5b, 0x55, 0xf7, 0xb4, 0x9e], iterations: S2K::decode_count(241), }, - password: &b"qwerty"[..], + password: "qwerty".into(), key_hex: "575AD156187A3F8CEC11108309236EB499F1E682F0D1AFADFAC4ECF97613108A", }, Test { @@ -333,7 +334,7 @@ mod tests { salt: [0xb9, 0x67, 0xea, 0x96, 0x53, 0xdb, 0x6a, 0xc8], iterations: S2K::decode_count(43), }, - password: &b"9876"[..], + password: "9876".into(), key_hex: "736C226B8C64E4E6D0325C6C552EF7C0738F98F48FED65FD8C93265103EFA23A", }, Test { @@ -344,7 +345,7 @@ mod tests { salt: [0x8f, 0x81, 0x74, 0xc5, 0xd9, 0x61, 0xc7, 0x79], iterations: S2K::decode_count(238), }, - password: &b"123"[..], + password: "123".into(), key_hex: "915E96FC694E7F90A6850B740125EA005199C725F3BD27E3", }, Test { @@ -355,7 +356,7 @@ mod tests { salt: [0x51, 0xed, 0xfc, 0x15, 0x45, 0x40, 0x65, 0xac], iterations: S2K::decode_count(238), }, - password: &b"0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"[..], + password: "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789".into(), key_hex: "EA264FADA5A859C40D88A159B344ECF1F51FF327FDB3C558B0A7DC299777173E", }, Test { @@ -366,7 +367,7 @@ mod tests { salt: [0x06, 0xe4, 0x61, 0x5c, 0xa4, 0x48, 0xf9, 0xdd], iterations: S2K::decode_count(238), }, - password: &b"0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"[..], + password: "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789".into(), key_hex: "F3D0CE52ED6143637443E3399437FD0F", }, ]; @@ -379,7 +380,7 @@ mod tests { assert_eq!(skesk.s2k, test.s2k); let key = skesk.s2k.derive_key( - test.password, + &test.password, skesk.symm_algo.key_size().unwrap()); if let Ok(key) = key { let key = to_hex(&key[..], false); diff --git a/openpgp/src/serialize/stream.rs b/openpgp/src/serialize/stream.rs index a0f0eb6d..2a946879 100644 --- a/openpgp/src/serialize/stream.rs +++ b/openpgp/src/serialize/stream.rs @@ -16,6 +16,7 @@ use { packet::OnePassSig, packet::PKESK, Result, + Password, SecretKey, SessionKey, packet::SKESK, @@ -838,7 +839,7 @@ impl<'a> Encryptor<'a> { /// )).unwrap(); /// let mut o = vec![]; /// let encryptor = Encryptor::new(wrap(&mut o), - /// &["совершенно секретно".as_bytes()], + /// &[&"совершенно секретно".into()], /// &[&tpk], /// EncryptionMode::AtRest) /// .expect("Failed to create encryptor"); @@ -848,7 +849,7 @@ impl<'a> Encryptor<'a> { /// # } /// ``` pub fn new(mut inner: writer::Stack<'a, Cookie>, - passwords: &[&[u8]], tpks: &[&TPK], + passwords: &[&Password], tpks: &[&TPK], encryption_mode: EncryptionMode) -> Result> { let mut rng = Yarrow::default(); @@ -1267,15 +1268,16 @@ mod test { #[test] fn encryptor() { - let passwords = ["streng geheim".as_bytes(), - "top secret".as_bytes()]; + let passwords: [Password; 2] = ["streng geheim".into(), + "top secret".into()]; let message = b"Hello world."; // Write a simple encrypted message... let mut o = vec![]; { - let encryptor = Encryptor::new(wrap(&mut o), &passwords, &[], - EncryptionMode::ForTransport) + let encryptor = Encryptor::new( + wrap(&mut o), &passwords.iter().collect::>(), + &[], EncryptionMode::ForTransport) .unwrap(); let mut literal = LiteralWriter::new(encryptor, DataFormat::Binary, None, None) diff --git a/openpgp/src/skesk.rs b/openpgp/src/skesk.rs index c38727c1..cee5e9c6 100644 --- a/openpgp/src/skesk.rs +++ b/openpgp/src/skesk.rs @@ -4,6 +4,7 @@ use Error; use SymmetricAlgorithm; use packet; use Packet; +use Password; use SessionKey; /// Holds an symmetrically encrypted session key. @@ -34,7 +35,7 @@ impl SKESK { /// used to encrypt the payload, and is also used to encrypt the /// given session key. pub fn new(algo: SymmetricAlgorithm, s2k: S2K, - session_key: &SessionKey, password: &[u8]) + session_key: &SessionKey, password: &Password) -> Result { // Derive key and make a cipher. let key = s2k.derive_key(password, algo.key_size()?)?; @@ -105,7 +106,7 @@ impl SKESK { /// Derives the key inside this SKESK from `password`. Returns a /// tuple of the symmetric cipher to use with the key and the key /// itself. - pub fn decrypt(&self, password: &[u8]) + pub fn decrypt(&self, password: &Password) -> Result<(SymmetricAlgorithm, SessionKey)> { let key = self.s2k.derive_key(password, self.symm_algo.key_size()?)?; diff --git a/tool/src/commands/decrypt.rs b/tool/src/commands/decrypt.rs index 28a0f259..90303fbc 100644 --- a/tool/src/commands/decrypt.rs +++ b/tool/src/commands/decrypt.rs @@ -159,11 +159,12 @@ impl<'a> DecryptionHelper for Helper<'a> { let p = rpassword::prompt_password_stderr( &format!( "Enter password to decrypt key {}: ", - self.key_hints.get(keyid).unwrap()))?; + self.key_hints.get(keyid).unwrap()))? + .into(); if let Ok(mpis) = key.secret().unwrap() - .decrypt(key.pk_algo(), p.as_bytes()) + .decrypt(key.pk_algo(), &p) { return Ok(Some(Secret::Asymmetric { key: key.clone(), @@ -182,7 +183,7 @@ impl<'a> DecryptionHelper for Helper<'a> { Pass::Passwords => return Ok(Some(Secret::Symmetric { password: rpassword::prompt_password_stderr( - "Enter password to decrypt message: ")?, + "Enter password to decrypt message: ")?.into(), })), } } diff --git a/tool/src/commands/mod.rs b/tool/src/commands/mod.rs index e236d713..b7475e6a 100644 --- a/tool/src/commands/mod.rs +++ b/tool/src/commands/mod.rs @@ -53,13 +53,13 @@ pub fn encrypt(store: &mut store::Store, &nprompt } else { "Enter password: " - })?); + })?.into()); } // Build a vector of references to hand to Encryptor. let recipients: Vec<&openpgp::TPK> = tpks.iter().collect(); - let passwords_: Vec<&[u8]> = - passwords.iter().map(|p| p.as_bytes()).collect(); + let passwords_: Vec<&openpgp::Password> = + passwords.iter().collect(); // We want to encrypt a literal data packet. let encryptor = Encryptor::new(wrap(output), -- cgit v1.2.3