Merge pull request #334 from ApprenticeofEnder/zeroize_secrets

Zeroize Secrets (Cursive and Lib)

Cargo.toml

@@ -27,6 +27,7 @@ base64 = "0.21.4"
 sha2 = "0.10.8"
 sha1 = "0.10.6"
 hmac = "0.12.1"
+zeroize = { version = "1.7.0", features = ["zeroize_derive", "alloc"] }
 
 [dependencies.config]
 version = "0.11.0"

cursive/Cargo.toml

@@ -21,6 +21,7 @@ lazy_static = "1.4.0"
 toml = "0.8.2"
 terminal_size = "0.3.0"
 hex = "0.4.3"
+zeroize = { version = "1.7.0", features = ["zeroize_derive", "alloc"] }
 
 [dependencies.config]
 version = "0.11.0"

cursive/src/helpers.rs

@@ -52,7 +52,7 @@ pub fn errorbox(ui: &mut Cursive, err: &pass::Error) {
 }
 
 /// Copies content to the clipboard.
-pub fn set_clipboard(content: String) -> Result<()> {
+pub fn set_clipboard(content: &String) -> Result<()> {
     Ok(CLIPBOARD.lock().unwrap().set_text(content)?)
 }
 

cursive/src/main.rs

@@ -50,6 +50,7 @@ mod helpers;
 mod wizard;
 
 use lazy_static::lazy_static;
+use zeroize::Zeroize;
 
 use crate::helpers::{get_value_from_input, is_checkbox_checked, is_radio_button_selected};
 
@@ -141,7 +142,9 @@ fn copy(ui: &mut Cursive, store: PasswordStoreType) {
         return;
     }
     if let Err(err) = || -> pass::Result<()> {
-        helpers::set_clipboard(sel.unwrap().secret(&*store.lock()?.lock()?)?)?;
+        let mut secret: String = sel.unwrap().secret(&*store.lock()?.lock()?)?;
+        helpers::set_clipboard(&secret)?;
+        secret.zeroize();
         Ok(())
     }() {
         helpers::errorbox(ui, &err);
@@ -150,7 +153,7 @@ fn copy(ui: &mut Cursive, store: PasswordStoreType) {
 
     thread::spawn(|| {
         thread::sleep(time::Duration::from_secs(40));
-        helpers::set_clipboard(String::new()).unwrap();
+        helpers::set_clipboard(&String::new()).unwrap();
     });
     ui.call_on_name("status_bar", |l: &mut TextView| {
         l.set_content(CATALOG.gettext("Copied password to copy buffer for 40 seconds"));
@@ -167,7 +170,10 @@ fn copy_first_line(ui: &mut Cursive, store: PasswordStoreType) {
         return;
     }
     if let Err(err) = || -> pass::Result<()> {
-        helpers::set_clipboard(sel.unwrap().secret(&*store.lock()?.lock()?)?)?;
+        //Wait, isn't this supposed to be a call to password()?
+        let mut secret = sel.unwrap().secret(&*store.lock()?.lock()?)?;
+        helpers::set_clipboard(&secret)?;
+        secret.zeroize();
         Ok(())
     }() {
         helpers::errorbox(ui, &err);
@@ -176,7 +182,7 @@ fn copy_first_line(ui: &mut Cursive, store: PasswordStoreType) {
 
     thread::spawn(|| {
         thread::sleep(time::Duration::from_secs(40));
-        helpers::set_clipboard(String::new()).unwrap();
+        helpers::set_clipboard(&String::new()).unwrap();
     });
     ui.call_on_name("status_bar", |l: &mut TextView| {
         l.set_content(CATALOG.gettext("Copied password to copy buffer for 40 seconds"));
@@ -193,7 +199,9 @@ fn copy_mfa(ui: &mut Cursive, store: PasswordStoreType) {
         return;
     }
     if let Err(err) = || -> pass::Result<()> {
-        helpers::set_clipboard(sel.unwrap().mfa(&*store.lock()?.lock()?)?)?;
+        let mut secret = sel.unwrap().mfa(&*store.lock()?.lock()?)?;
+        helpers::set_clipboard(&secret)?;
+        secret.zeroize();
         Ok(())
     }() {
         helpers::errorbox(ui, &err);
@@ -218,7 +226,7 @@ fn copy_name(ui: &mut Cursive) {
 
     if let Err(err) = || -> pass::Result<()> {
         let name = sel.name.split('/').next_back();
-        helpers::set_clipboard(name.unwrap_or("").to_string())?;
+        helpers::set_clipboard(&name.unwrap_or("").to_string())?;
         Ok(())
     }() {
         helpers::errorbox(ui, &err);
@@ -395,7 +403,7 @@ fn open(ui: &mut Cursive, store: PasswordStoreType) -> Result<()> {
 
     let password_entry = password_entry_opt.unwrap();
 
-    let password = {
+    let mut password = {
         match password_entry.secret(&*store.lock()?.lock()?) {
             Ok(p) => p,
             Err(err) => {
@@ -404,35 +412,40 @@ fn open(ui: &mut Cursive, store: PasswordStoreType) -> Result<()> {
             }
         }
     };
-    let d = Dialog::around(TextArea::new().content(password).with_name("editbox"))
+    let d = Dialog::around(TextArea::new().content(&password).with_name("editbox"))
         .button(CATALOG.gettext("Save"), move |s| {
-            let new_password = s
+            let mut new_secret = s
                 .call_on_name("editbox", |e: &mut TextArea| e.get_content().to_string())
                 .unwrap();
 
-            if new_password.contains("otpauth://") {
+            if new_secret.contains("otpauth://") {
                 let store = store.clone();
                 let d = Dialog::around(TextView::new(CATALOG.gettext("It seems like you are trying to save a TOTP code to the password store. This will reduce your 2FA solution to just 1FA, do you want to proceed?")))
                     .button(CATALOG.gettext("Save"), move |s| {
-                        do_password_save(s, &new_password, store.clone(), true);
+                        let mut confirmed_new_secret = s
+                            .call_on_name("editbox", |e: &mut TextArea| e.get_content().to_string())
+                            .unwrap();
+                        do_password_save(s, &confirmed_new_secret, store.clone(), true);
+                        confirmed_new_secret.zeroize();
                     })
                     .dismiss_button(CATALOG.gettext("Close"));
 
                 let ev = OnEventView::new(d).on_event(Key::Esc, |s| {
                     s.pop_layer();
                 });
-
                 s.add_layer(ev);
             } else {
-                do_password_save(s, &new_password, store.clone(), false);
+                do_password_save(s, &new_secret, store.clone(), false);
             };
+            new_secret.zeroize();
 
         })
         .button(CATALOG.gettext("Generate"), move |s| {
-            let new_password = ripasso::words::generate_password(6);
+            let mut new_password = ripasso::words::generate_password(6);
             s.call_on_name("editbox", |e: &mut TextArea| {
-                e.set_content(new_password);
+                e.set_content(&new_password);
             });
+            new_password.zeroize();
         })
         .dismiss_button(CATALOG.gettext("Close"));
 
@@ -441,7 +454,7 @@ fn open(ui: &mut Cursive, store: PasswordStoreType) -> Result<()> {
     });
 
     ui.add_layer(ev);
-
+    password.zeroize();
     Ok(())
 }
 

cursive/src/tests/main.rs

@@ -193,6 +193,8 @@ fn substr_overlong() {
 
 #[test]
 fn create_label_basic() {
+    // TODO: Fix this test so that time zones don't mess with it.
+
     let p = pass::PasswordEntry::new(
         &PathBuf::from("/tmp/"),
         &PathBuf::from("file.gpg"),

src/crypto.rs

@@ -26,6 +26,7 @@ use sequoia_openpgp::{
     types::{RevocationStatus, SymmetricAlgorithm},
     Cert, KeyHandle,
 };
+use zeroize::Zeroize;
 
 pub use crate::error::{Error, Result};
 use crate::{
@@ -227,7 +228,9 @@ impl Crypto for GpgMe {
         let mut ctx = gpgme::Context::from_protocol(gpgme::Protocol::OpenPgp)?;
         let mut output = Vec::new();
         ctx.decrypt(ciphertext, &mut output)?;
-        Ok(String::from_utf8(output)?)
+        let result = String::from_utf8(output.to_vec())?;
+        output.zeroize();
+        Ok(result)
     }
 
     fn encrypt_string(&self, plaintext: &str, recipients: &[Recipient]) -> Result<Vec<u8>> {
@@ -249,7 +252,6 @@ impl Crypto for GpgMe {
             &mut output,
             gpgme::EncryptFlags::NO_COMPRESS,
         )?;
-
         Ok(output)
     }
 
@@ -756,8 +758,9 @@ impl Crypto for Sequoia {
 
             // Decrypt the data.
             std::io::copy(&mut decryptor, &mut sink).unwrap();
-
-            Ok(std::str::from_utf8(&sink).unwrap().to_owned())
+            let result = std::str::from_utf8(&sink).unwrap().to_owned();
+            sink.zeroize();
+            Ok(result)
         } else {
             // Make a helper that that feeds the recipient's secret key to the
             // decryptor.
@@ -776,8 +779,9 @@ impl Crypto for Sequoia {
 
             // Decrypt the data.
             std::io::copy(&mut decryptor, &mut sink)?;
-
-            Ok(std::str::from_utf8(&sink)?.to_owned())
+            let result = std::str::from_utf8(&sink)?.to_owned();
+            sink.zeroize();
+            Ok(result)
         }
     }
 

src/pass.rs

@@ -26,6 +26,7 @@ use std::{
 
 use chrono::prelude::*;
 use totp_rs::TOTP;
+use zeroize::Zeroize;
 
 use crate::{
     crypto::{Crypto, CryptoImpl, GpgMe, Sequoia, VerificationError},
@@ -692,7 +693,9 @@ impl PasswordStore {
     fn reencrypt_all_password_entries(&self) -> Result<()> {
         let mut names: Vec<PathBuf> = Vec::new();
         for entry in self.all_passwords()? {
-            entry.update_internal(&entry.secret(self)?, self)?;
+            let mut secret = entry.secret(self)?;
+            entry.update_internal(&secret, self)?;
+            secret.zeroize();
             names.push(append_extension(PathBuf::from(&entry.name), ".gpg"));
         }
         names.push(PathBuf::from(".gpg-id"));
@@ -785,12 +788,13 @@ impl PasswordStore {
         fs::create_dir_all(&new_path_dir)?;
 
         let mut file = std::fs::File::create(&new_path)?;
-        let secret = self.crypto.decrypt_string(&std::fs::read(&old_path)?)?;
+        let mut secret = self.crypto.decrypt_string(&std::fs::read(&old_path)?)?;
         let new_recipients = Recipient::all_recipients(
             &self.recipients_file_for_dir(&new_path)?,
             self.crypto.as_ref(),
         )?;
         file.write_all(&self.crypto.encrypt_string(&secret, &new_recipients)?)?;
+        secret.zeroize();
         std::fs::remove_file(&old_path)?;
 
         if self.repo().is_ok() {
@@ -1014,14 +1018,17 @@ impl PasswordEntry {
     /// # Errors
     /// Returns an `Err` if the decryption fails
     pub fn password(&self, store: &PasswordStore) -> Result<String> {
-        Ok(self.secret(store)?.split('\n').take(1).collect())
+        let mut secret = self.secret(store)?;
+        let password: String = secret.split('\n').take(1).collect();
+        secret.zeroize();
+        Ok(password)
     }
 
     /// decrypts and returns a TOTP code if the entry contains a otpauth:// url
     /// # Errors
     /// Returns an `Err` if the code generation fails
     pub fn mfa(&self, store: &PasswordStore) -> Result<String> {
-        let secret = self.secret(store)?;
+        let mut secret = self.secret(store)?;
 
         if let Some(start_pos) = secret.find("otpauth://") {
             let end_pos = {
@@ -1035,12 +1042,15 @@ impl PasswordEntry {
                 end_pos
             };
             let totp = TOTP::from_url(&secret[start_pos..end_pos])?;
+            secret.zeroize();
             Ok(totp.generate_current()?)
         } else {
+            secret.zeroize();
             Err(Error::Generic("No otpauth:// url in secret"))
         }
     }
 
+    /// All calls to this function must be followed by secret.zeroize()
     fn update_internal(&self, secret: &str, store: &PasswordStore) -> Result<()> {
         if !store.valid_gpg_signing_keys.is_empty() {
             store.verify_gpg_id_files()?;
@@ -1058,8 +1068,9 @@ impl PasswordEntry {
     /// is supplied.
     /// # Errors
     /// Returns an `Err` if the update fails.
-    pub fn update(&self, secret: String, store: &PasswordStore) -> Result<()> {
+    pub fn update(&self, mut secret: String, store: &PasswordStore) -> Result<()> {
         self.update_internal(&secret, store)?;
+        secret.zeroize();
 
         if store.repo().is_err() {
             return Ok(());

src/tests/pass.rs

@@ -1184,9 +1184,10 @@ fn decrypt_password_multiline() -> Result<()> {
         user_home: None,
     };
 
-    let res = pe.password(&store).unwrap();
+    let mut res = pe.password(&store).unwrap();
 
     assert_eq!("row one", res);
+    res.zeroize();
 
     Ok(())
 }