fix: wipe secrets on every exit of softsign keygen, softsign import and yubihsm setup

Cargo.toml

@@ -55,7 +55,6 @@ zeroize = "1"
 [dev-dependencies]
 abscissa_core = { version = "0.9", features = ["testing"] }
 byteorder = "1"
-rand = "0.8"
 toml = "0.9"
 
 [features]

FORK.md

@@ -43,7 +43,7 @@ liveness/DoS > hygiene.
 | 11  | `tmkms init` will not destroy an existing identity key without `-f`            | **Medium** | Data loss           |
 | 12  | Disk failures are no longer reported as "attempted double sign"                | **Low**    | Observability       |
 | 13  | Bounded TCP connect, Unix-socket timeouts, no peer payloads in logs            | **Low**    | Liveness/DoS        |
-| 14  | Zeroization gaps closed in key load, keygen, `yubihsm keys import` (see below) | **Low**    | Hygiene             |
+| 14  | Zeroization gaps closed in key load, keygen, key import and setup (see below)  | **Low**    | Hygiene             |
 | 15  | Dependencies refreshed, CI hardened, dev image off EOL centos:7                | **Low**    | Supply chain        |
 | 16  | `tmkms ledger init` can succeed at all, and honours the double-sign check      | **Low**    | Upstream bug (dead) |
 | 17  | A `gno` key format prints the public key gno.land accepts                      | **Low**    | gno.land addition   |
@@ -184,8 +184,7 @@ Signing is unaffected either way.
   `fortanixdsm` feature, which the release image does not build, and even there the
   Fortanix DSM client speaks HTTP/1.1, so it never runs. The fix needs hyper 1, which
   `sdkms` does not use.
-- **Key copies held inside two dependencies are not wiped (#14).** tmkms cannot
-  reach them:
+- **Some key copies are out of tmkms's reach, and not wiped (#14):**
   - `iq-cometbft` parses every `priv_validator_key.json`, and its Ed25519 key type is
     a plain byte array that never wipes itself and cannot be wiped from outside. Each
     parse leaves the seed in freed memory: `yubihsm keys import`, `softsign import`,
@@ -194,9 +193,11 @@ Signing is unaffected either way.
     wiped either, whatever the key type.
   - `yubihsm` copies a key into command buffers it frees without wiping, which
     upstream marks as a TODO. So every key `yubihsm keys import` stores, Base64 or
-    JSON, leaves copies in freed memory.
+    JSON, and every key `yubihsm setup` installs leaves copies in freed memory.
+  - The standard library's stdin and stdout buffers, which nothing can wipe, keep a
+    mnemonic typed into `yubihsm setup --restore` and parts of the keys setup prints.
 
-  Revisit when these crates wipe their key buffers.
+  Revisit the first two when these crates wipe their key buffers.
 - **The container runs as root**, matching the previously published image, because
   key and state files arrive on a host-mounted volume whose ownership is managed
   outside the image.
@@ -205,7 +206,7 @@ Signing is unaffected either way.
 
 - `cargo fmt --all -- --check` — clean.
 - `cargo clippy --all-features --all-targets` — clean.
-- `cargo test --all-features -- --test-threads 1` — 70 unit + 31 integration tests
+- `cargo test --all-features -- --test-threads 1` — 75 unit + 40 integration tests
   pass (10 ignored, all hardware-dependent), printing nothing but their results.
 - The published image is pulled and checked on every release: it reports its own
   version, keeps `tmkms` as the entrypoint, and links the USB libraries the

src/commands/softsign/import.rs

@@ -1,11 +1,15 @@
 //! `tmkms softsign import` command
 
-use crate::{config::provider::softsign::KeyFormat, key_utils, prelude::*};
+use crate::{config::provider::softsign::KeyFormat, error::Error, key_utils, prelude::*};
 use abscissa_core::Command;
 use clap::Parser;
 use cometbft::PrivateKey;
 use cometbft_config::PrivValidatorKey;
-use std::{path::PathBuf, process};
+use std::{
+    path::{Path, PathBuf},
+    process,
+};
+use zeroize::Zeroizing;
 
 /// `import` command: import a `priv_validator.json` formatted key and convert
 /// it into the raw format used by the softsign backend (by default)
@@ -54,25 +58,69 @@ impl Runnable for ImportCommand {
             })
             .priv_key;
 
-        match secret_key {
-            PrivateKey::Ed25519(sk) => {
-                key_utils::write_base64_secret(output_path, sk.as_bytes()).unwrap_or_else(|e| {
-                    status_err!("{}", e);
-                    process::exit(1);
-                });
-                info!("Imported Ed25519 private key to {}", output_path.display());
-            }
-            PrivateKey::Secp256k1(sk) => {
-                key_utils::write_base64_secret(output_path, &sk.to_bytes()).unwrap_or_else(|e| {
-                    status_err!("{}", e);
-                    process::exit(1);
-                });
-                info!(
-                    "Imported Secp256k1 private key to {}",
-                    output_path.display()
-                );
+        // Exit only here, once `write_secret_key` has returned and dropped the key:
+        // `process::exit` runs no destructors, so nothing would wipe it
+        match write_secret_key(secret_key, output_path) {
+            Ok(algorithm) => info!(
+                "Imported {} private key to {}",
+                algorithm,
+                output_path.display()
+            ),
+            Err(e) => {
+                status_err!("{}", e);
+                process::exit(1);
             }
-            _ => unreachable!("unsupported priv_validator.json algorithm"),
+        }
+    }
+}
+
+/// Write `secret_key` to `output_path` in the softsign format, and return the name of
+/// its algorithm
+fn write_secret_key(secret_key: PrivateKey, output_path: &Path) -> Result<&'static str, Error> {
+    match secret_key {
+        PrivateKey::Ed25519(sk) => {
+            key_utils::write_base64_secret(output_path, sk.as_bytes())?;
+            Ok("Ed25519")
+        }
+        PrivateKey::Secp256k1(sk) => {
+            // `to_bytes` hands back a copy of the private scalar
+            let secret_bytes = Zeroizing::new(sk.to_bytes());
+            key_utils::write_base64_secret(output_path, &secret_bytes)?;
+            Ok("Secp256k1")
+        }
+        _ => unreachable!("unsupported priv_validator.json algorithm"),
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// `process::exit` runs no destructors, so a write failure that ended the process
+    /// where it happened would skip wiping the imported key. It must come back to `run`
+    /// instead, after the key has been dropped.
+    #[test]
+    fn a_failed_write_comes_back_to_run() {
+        let dir = tempfile::tempdir().unwrap();
+        let unwritable = dir.path().join("missing/key");
+
+        for key_file in [
+            "tests/support/priv_validator_mock.json",
+            "tests/support/priv_validator_secp256k1_mock.json",
+        ] {
+            let secret_key = PrivValidatorKey::load_json_file(&key_file)
+                .unwrap()
+                .priv_key;
+
+            assert_eq!(
+                write_secret_key(secret_key, &unwritable)
+                    .unwrap_err()
+                    .to_string(),
+                format!(
+                    "I/O error: couldn't write `{}`: No such file or directory (os error 2)",
+                    unwritable.display()
+                )
+            );
         }
     }
 }

src/commands/softsign/keygen.rs

@@ -1,6 +1,6 @@
 //! `tmkms softsign keygen` subcommand
 
-use crate::{key_utils, prelude::*};
+use crate::{error::Error, key_utils, prelude::*};
 use abscissa_core::Command;
 use clap::Parser;
 use k256::ecdsa;
@@ -32,7 +32,7 @@ impl Runnable for KeygenCommand {
 
         let output_path = &self.output_paths[0];
 
-        match self
+        let generated = match self
             .key_type
             .as_ref()
             .map(AsRef::as_ref)
@@ -47,30 +47,35 @@ impl Runnable for KeygenCommand {
                 );
                 process::exit(1);
             }
+        };
+
+        // Exit only here, once the generator has returned and dropped the key:
+        // `process::exit` runs no destructors, so nothing would wipe it
+        if let Err(e) = generated {
+            status_err!("{}", e);
+            process::exit(1);
         }
     }
 }
 
 /// Randomly generate a Base64-encoded secp256k1 key and store it at the given path
-fn generate_secp256k1_key(output_path: &Path) {
+fn generate_secp256k1_key(output_path: &Path) -> Result<(), Error> {
     let signing_key = ecdsa::SigningKey::random(&mut OsRng);
     // `to_bytes` hands back a copy of the private scalar
     let secret_bytes = Zeroizing::new(signing_key.to_bytes());
 
-    key_utils::write_base64_secret(output_path, &secret_bytes).unwrap_or_else(|e| {
-        status_err!("{}", e);
-        process::exit(1);
-    });
+    key_utils::write_base64_secret(output_path, &secret_bytes)?;
 
     status_ok!(
         "Generated",
         "account (secp256k1) private key at: {}",
         output_path.display()
     );
+    Ok(())
 }
 
 /// Randomly generate a Base64-encoded Ed25519 key and store it at the given path
-fn generate_ed25519_key(output_path: &Path) {
+fn generate_ed25519_key(output_path: &Path) -> Result<(), Error> {
     // `Zeroizing` wipes the seed when this scope ends; `SigningKey` zeroizes
     // itself on drop, but the buffer it was built from would otherwise be left
     // in place
@@ -80,14 +85,36 @@ fn generate_ed25519_key(output_path: &Path) {
     // the stack and leave that copy unwiped
     let sk = ed25519_dalek::SigningKey::from_bytes(&sk_bytes);
 
-    key_utils::write_base64_secret(output_path, sk.as_bytes()).unwrap_or_else(|e| {
-        status_err!("{}", e);
-        process::exit(1);
-    });
+    key_utils::write_base64_secret(output_path, sk.as_bytes())?;
 
     status_ok!(
         "Generated",
         "consensus (Ed25519) private key at: {}",
         output_path.display()
     );
+    Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// `process::exit` runs no destructors, so a write failure that ended the process
+    /// where it happened would skip wiping the key just generated. It must come back to
+    /// `run` instead, after the key has been dropped.
+    #[test]
+    fn a_failed_write_comes_back_to_run() {
+        let dir = tempfile::tempdir().unwrap();
+        let unwritable = dir.path().join("missing/key");
+
+        for generate in [generate_secp256k1_key, generate_ed25519_key] {
+            assert_eq!(
+                generate(&unwritable).unwrap_err().to_string(),
+                format!(
+                    "I/O error: couldn't write `{}`: No such file or directory (os error 2)",
+                    unwritable.display()
+                )
+            );
+        }
+    }
 }

src/commands/yubihsm.rs

@@ -2,7 +2,7 @@
 
 mod detect;
 pub mod keys;
-mod setup;
+pub mod setup;
 mod test;
 
 pub use self::{detect::DetectCommand, keys::KeysCommand, setup::SetupCommand, test::TestCommand};