diff --git a/FORK.md b/FORK.md index 077e1582..6752d74f 100644 --- a/FORK.md +++ b/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 and `yubihsm keys import` | **Low** | Hygiene | +| 14 | Zeroization gaps closed in key load, keygen, `yubihsm keys import` (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,6 +184,19 @@ 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: + - `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`, + and loading a `[[providers.softsign]]` key with `key_format = "json"`. The last two + also have `iq-cometbft-config` read the file, and that copy of its text is not + 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. + + Revisit 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. @@ -192,7 +205,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` — 68 unit + 28 integration tests +- `cargo test --all-features -- --test-threads 1` — 70 unit + 31 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 diff --git a/src/commands/yubihsm.rs b/src/commands/yubihsm.rs index 28ea01b6..d1ef4573 100644 --- a/src/commands/yubihsm.rs +++ b/src/commands/yubihsm.rs @@ -1,7 +1,7 @@ //! `tmkms yubihsm` CLI (sub)commands mod detect; -mod keys; +pub mod keys; mod setup; mod test; diff --git a/src/commands/yubihsm/keys.rs b/src/commands/yubihsm/keys.rs index 541b7627..0ed1c8c1 100644 --- a/src/commands/yubihsm/keys.rs +++ b/src/commands/yubihsm/keys.rs @@ -2,7 +2,7 @@ mod export; mod generate; -mod import; +pub mod import; mod list; use self::{ diff --git a/src/commands/yubihsm/keys/import.rs b/src/commands/yubihsm/keys/import.rs index 6adf7976..c5a2a60d 100644 --- a/src/commands/yubihsm/keys/import.rs +++ b/src/commands/yubihsm/keys/import.rs @@ -1,14 +1,14 @@ //! Import keys either from encrypted backups or existing plaintext keys use super::{DEFAULT_DOMAINS, DEFAULT_WRAP_KEY}; -use crate::{keyring::ed25519, prelude::*}; +use crate::{key_utils, keyring::ed25519, prelude::*}; use abscissa_core::Command; use clap::Parser; use cometbft::{PrivateKey, PublicKey}; use cometbft_config::PrivValidatorKey; use std::{fs, path::PathBuf, process}; use subtle_encoding::base64; -use yubihsm::object; +use yubihsm::{Client, object}; use zeroize::Zeroizing; /// The `yubihsm keys import` subcommand @@ -38,36 +38,51 @@ pub struct ImportCommand { pub path: PathBuf, } +/// What `yubihsm keys import` reports when the key's import needs a key ID it wasn't given +const NO_KEY_ID: &str = + "no key ID specified (use e.g. tmkms yubihsm keys import -i 1 priv_validator.json)"; + impl Runnable for ImportCommand { fn run(&self) { - // The file holds key material, so wipe the buffer when this scope ends - let contents = Zeroizing::new(fs::read_to_string(&self.path).unwrap_or_else(|e| { - status_err!("couldn't import file {}: {}", self.path.display(), e); + // Connecting to the HSM ends the process when it fails, so connect before any + // secret is in memory + let hsm = crate::yubihsm::client(); + + // Exit only here, once `import` has returned and dropped every secret it held: + // `process::exit` runs no destructors, so nothing would wipe them + if let Err(message) = self.import(&hsm) { + status_err!("{}", message); process::exit(1); - })); + } + } +} + +impl ImportCommand { + /// Import the key file, returning what went wrong as the message the operator reads + pub fn import(&self, hsm: &Client) -> Result<(), String> { + // The file holds key material, so wipe the buffer when this scope ends + let contents = Zeroizing::new( + fs::read_to_string(&self.path) + .map_err(|e| format!("couldn't import file {}: {}", self.path.display(), e))?, + ); match self.key_type.as_deref() { - Some("wrap") => self.import_wrapped(&contents), - Some("json") => self.import_priv_validator_json(&contents), - Some("base64") => self.import_base64(&contents), - Some(other) => { - status_err!("invalid key type: {}", other); - process::exit(1); - } + Some("wrap") => self.import_wrapped(hsm, &contents), + Some("json") => self.import_priv_validator_json(hsm, &contents), + Some("base64") => self.import_base64(hsm, &contents), + Some(other) => Err(format!("invalid key type: {other}")), None => { if self.path.ends_with("priv_validator_key.json") { - self.import_priv_validator_json(&contents) + self.import_priv_validator_json(hsm, &contents) } else { - self.import_wrapped(&contents) + self.import_wrapped(hsm, &contents) } } } } -} -impl ImportCommand { /// Import a wrapped object into the HSM - fn import_wrapped(&self, wrapped_key_base64: &str) { + fn import_wrapped(&self, hsm: &Client, wrapped_key_base64: &str) -> Result<(), String> { if let Some(id) = self.key_id { status_warn!("ignoring key ID: {} (wrapped keys use original key ID)", id); } @@ -82,38 +97,33 @@ impl ImportCommand { let wrap_key_id = self.wrap_key_id.unwrap_or(DEFAULT_WRAP_KEY); let wrapped_key_ciphertext = - base64::decode(wrapped_key_base64.as_bytes()).unwrap_or_else(|e| { - status_err!( + base64::decode(wrapped_key_base64.as_bytes()).map_err(|e| { + format!( "couldn't decode Base64-encoded wrapped key from {}: {}", self.path.display(), e - ); - process::exit(1); - }); + ) + })?; - let wrapped_message = yubihsm::wrap::Message::from_vec(wrapped_key_ciphertext) - .unwrap_or_else(|e| { - status_err!( + let wrapped_message = + yubihsm::wrap::Message::from_vec(wrapped_key_ciphertext).map_err(|e| { + format!( "couldn't parse wrapped key from {}: {}", self.path.display(), e - ); - process::exit(1); - }); - - let hsm = crate::yubihsm::client(); + ) + })?; let obj = hsm .import_wrapped(wrap_key_id, wrapped_message) - .unwrap_or_else(|e| { - status_err!( + .map_err(|e| { + format!( "error importing encrypted key from {} (using wrapkey 0x{:04x}): {}", self.path.display(), wrap_key_id, e - ); - process::exit(1); - }); + ) + })?; if obj.object_type != object::Type::AsymmetricKey { // We mainly care about importing asymmetric keys, but can handle other types. @@ -124,17 +134,15 @@ impl ImportCommand { obj.object_id, obj.object_type ); - process::exit(0); + return Ok(()); } - let public_key = hsm.get_public_key(obj.object_id).unwrap_or_else(|e| { - status_err!( + let public_key = hsm.get_public_key(obj.object_id).map_err(|e| { + format!( "couldn't get public key for asymmetric key #{}: {}", - obj.object_id, - e - ); - process::exit(1); - }); + obj.object_id, e + ) + })?; // TODO: display non hex format when listing/displaying keys let key_info = match public_key.algorithm { @@ -147,63 +155,49 @@ impl ImportCommand { }; status_ok!("Imported", "key 0x{:04x}: {}", obj.object_id, key_info); + Ok(()) } /// Import an existing priv_validator file into the HSM - fn import_priv_validator_json(&self, json_data: &str) { - if let Some(id) = self.wrap_key_id { - status_warn!( - "ignoring wrapkey ID: {} (not applicable to priv_validator.json files)", - id - ); - } - - let key_id = self.key_id.unwrap_or_else(|| { - status_err!( - "no key ID specified (use e.g. tmkms yubihsm keys import -i 1 priv_validator.json)" - ); - process::exit(1); - }); + fn import_priv_validator_json(&self, hsm: &Client, json_data: &str) -> Result<(), String> { + let key_id = self.ed25519_key_id()?; let private_key = PrivValidatorKey::parse_json(json_data) - .unwrap_or_else(|e| { - status_err!("couldn't parse priv_validator key: {}", e); - process::exit(1); - }) + .map_err(|e| format!("couldn't parse priv_validator key: {e}"))? .priv_key; // `PrivValidatorKey` also accepts secp256k1, so this is reachable with a // legitimate key file - let seed = match private_key { - PrivateKey::Ed25519(pk) => pk, - _ => { - status_err!( - "unsupported key type in `{}`: an Ed25519 consensus key is required", - self.path.display() - ); - process::exit(1); - } + let PrivateKey::Ed25519(seed) = private_key else { + return Err(format!( + "unsupported key type in `{}`: an Ed25519 consensus key is required", + self.path.display() + )); }; - let label = object::Label::from(self.label.as_ref().map(|l| l.as_ref()).unwrap_or("")); + self.put_ed25519_seed(hsm, key_id, seed.as_bytes()) + } - if let Err(e) = crate::yubihsm::client().put_asymmetric_key( - key_id, - label, - DEFAULT_DOMAINS, - yubihsm::Capability::SIGN_EDDSA | yubihsm::Capability::EXPORTABLE_UNDER_WRAP, - yubihsm::asymmetric::Algorithm::Ed25519, - seed.as_bytes(), - ) { - status_err!("couldn't import key #{}: {}", self.key_id.unwrap(), e); - process::exit(1); - } + /// Import a Base64-encoded private key into the HSM + fn import_base64(&self, hsm: &Client, base64_data: &str) -> Result<(), String> { + let key_id = self.ed25519_key_id()?; - status_ok!("Imported", "key 0x{:04x}", key_id); + let key_bytes = key_utils::decode_base64_secret(base64_data) + .map_err(|e| format!("can't decode Ed25519 key: {e}"))?; + + let secret = ed25519::SigningKey::try_from(key_bytes.as_ref()) + .map_err(|e| format!("invalid Ed25519 key: {e}"))?; + + let seed_bytes = secret + .as_bytes() + .ok_or("Ed25519 key must be provided as a 32-byte seed")?; + + self.put_ed25519_seed(hsm, key_id, seed_bytes) } - /// Import a Base64-encoded private key into the HSM - fn import_base64(&self, base64_data: &str) { + /// The key ID to import an Ed25519 key as, warning that a wrap key ID applies only + /// to wrapped keys + fn ed25519_key_id(&self) -> Result { if let Some(id) = self.wrap_key_id { status_warn!( "ignoring wrapkey ID: {} (not applicable to priv_validator.json files)", @@ -211,44 +205,173 @@ impl ImportCommand { ); } - let key_id = self.key_id.unwrap_or_else(|| { - status_err!( - "no key ID specified (use e.g. tmkms yubihsm keys import -i 1 priv_validator.json)" - ); - process::exit(1); - }); - - // TODO(tarcieri): constant-time string trimming - let key_bytes = - Zeroizing::new(base64::decode(base64_data.trim_end()).unwrap_or_else(|e| { - status_err!("can't decode Ed25519 key: {}", e); - process::exit(1); - })); - - let secret = ed25519::SigningKey::try_from(key_bytes.as_ref()).unwrap_or_else(|e| { - status_err!("invalid Ed25519 key: {}", e); - process::exit(1); - }); - - let seed_bytes = secret.as_bytes().unwrap_or_else(|| { - status_err!("Ed25519 key must be provided as a 32-byte seed"); - process::exit(1); - }); + self.key_id.ok_or_else(|| NO_KEY_ID.to_owned()) + } - let label = object::Label::from(self.label.as_ref().map(|l| l.as_ref()).unwrap_or("")); + /// Put an Ed25519 seed into the HSM as key `key_id` + fn put_ed25519_seed(&self, hsm: &Client, key_id: u16, seed: &[u8]) -> Result<(), String> { + let label = object::Label::from(self.label.as_deref().unwrap_or("")); - if let Err(e) = crate::yubihsm::client().put_asymmetric_key( + hsm.put_asymmetric_key( key_id, label, DEFAULT_DOMAINS, yubihsm::Capability::SIGN_EDDSA | yubihsm::Capability::EXPORTABLE_UNDER_WRAP, yubihsm::asymmetric::Algorithm::Ed25519, - seed_bytes, - ) { - status_err!("couldn't import key #{}: {}", self.key_id.unwrap(), e); - process::exit(1); - } + seed, + ) + .map_err(|e| format!("couldn't import key #{key_id}: {e}"))?; status_ok!("Imported", "key 0x{:04x}", key_id); + Ok(()) + } +} + +#[cfg(all(test, feature = "yubihsm-mock"))] +mod tests { + use super::*; + use std::path::Path; + use yubihsm::{Connector, Credentials}; + + /// How the HSM behind `failing_hsm` ends its refusal of every command; what comes + /// before names a session number, which varies + const HSM_REFUSAL: &str = "invalid credentials for authentication key #1 (cryptogram mismatch)"; + + /// A mock HSM client with the wrong password, so every command it sends fails + fn failing_hsm() -> Client { + Client::create( + Connector::mockhsm(), + Credentials::from_password(1, b"wrong"), + ) + .unwrap() + } + + /// A command importing the file at `path` as `key_type` + fn import_command(key_type: &str, key_id: Option, path: &Path) -> ImportCommand { + ImportCommand { + key_id, + key_type: Some(key_type.to_owned()), + path: path.to_owned(), + ..Default::default() + } + } + + /// `process::exit` runs no destructors, so a failure that ended the process where it + /// happened would skip wiping the file buffer and any key already decoded. Each one + /// must come back to `run` instead, after those have been dropped. + #[test] + fn failures_come_back_to_run() { + let hsm = failing_hsm(); + let dir = tempfile::tempdir().unwrap(); + let file = |name: &str, contents: &str| { + let path = dir.path().join(name); + fs::write(&path, contents).unwrap(); + path + }; + let not_base64 = file("not_base64", "not base64!"); + let short_wrap = file("short_wrap", "AAAAAAAAAAAAAAAA"); + let empty_json = file("empty.json", "{}"); + let short_key = file("short_key", "AAAAAAAAAAAAAAAAAAAAAA=="); + let expanded_key = file("expanded_key", &format!("{}==", "A".repeat(86))); + let secp256k1_json = Path::new("tests/support/priv_validator_secp256k1_mock.json"); + let seed = Path::new("tests/support/signing_ed25519.key"); + let no_key_id = + "no key ID specified (use e.g. tmkms yubihsm keys import -i 1 priv_validator.json)"; + + let cases = [ + ( + import_command("bogus", None, ¬_base64), + "invalid key type: bogus".to_owned(), + ), + ( + import_command("wrap", None, ¬_base64), + format!( + "couldn't decode Base64-encoded wrapped key from {}: bad encoding", + not_base64.display() + ), + ), + ( + import_command("wrap", None, &short_wrap), + format!( + "couldn't parse wrapped key from {}: \ + invalid message length: message must be at least 13-bytes", + short_wrap.display() + ), + ), + ( + import_command("json", None, secp256k1_json), + no_key_id.to_owned(), + ), + ( + import_command("json", Some(1), &empty_json), + "couldn't parse priv_validator key: \ + StringTracer: serde json error: missing field `address` at line 1 column 2" + .to_owned(), + ), + ( + import_command("json", Some(1), secp256k1_json), + format!( + "unsupported key type in `{}`: an Ed25519 consensus key is required", + secp256k1_json.display() + ), + ), + (import_command("base64", None, seed), no_key_id.to_owned()), + ( + import_command("base64", Some(1), ¬_base64), + "can't decode Ed25519 key: bad encoding".to_owned(), + ), + ( + import_command("base64", Some(1), &short_key), + "invalid Ed25519 key: \ + invalid key: invalid Ed25519 key size: expected 32, 64, or 96, but got 16" + .to_owned(), + ), + ( + import_command("base64", Some(1), &expanded_key), + "Ed25519 key must be provided as a 32-byte seed".to_owned(), + ), + ]; + + for (command, message) in cases { + assert_eq!(command.import(&hsm), Err(message)); + } + } + + /// The HSM's own failures come once the key is decoded, so they must come back to + /// `run` too + #[test] + fn hsm_failures_come_back_to_run() { + let hsm = failing_hsm(); + let dir = tempfile::tempdir().unwrap(); + let wrapped = dir.path().join("wrapped"); + fs::write(&wrapped, format!("{}=", "A".repeat(43))).unwrap(); + let json = Path::new("tests/support/priv_validator_mock.json"); + let seed = Path::new("tests/support/signing_ed25519.key"); + + let cases = [ + ( + import_command("json", Some(1), json), + "couldn't import key #1: ".to_owned(), + ), + ( + import_command("base64", Some(1), seed), + "couldn't import key #1: ".to_owned(), + ), + ( + import_command("wrap", None, &wrapped), + format!( + "error importing encrypted key from {} (using wrapkey 0x0001): ", + wrapped.display() + ), + ), + ]; + + for (command, reason) in cases { + let message = command.import(&hsm).unwrap_err(); + assert!( + message.starts_with(&reason) && message.ends_with(HSM_REFUSAL), + "{message}" + ); + } } } diff --git a/src/key_utils.rs b/src/key_utils.rs index 80e96994..3c2eebd1 100644 --- a/src/key_utils.rs +++ b/src/key_utils.rs @@ -13,7 +13,7 @@ use std::{ os::unix::fs::{OpenOptionsExt, PermissionsExt}, path::Path, }; -use subtle_encoding::base64; +use subtle_encoding::{Base64, Encoding, base64}; use zeroize::Zeroizing; /// File permissions for secret data @@ -47,6 +47,20 @@ fn warn_if_readable_by_others(path: &Path) { } } +/// Decode a Base64-encoded secret, ignoring trailing whitespace such as a final +/// newline. The decoder writes every group before it reports an invalid one, so it +/// decodes into a buffer that is wiped when dropped, whether or not decoding succeeds. +pub fn decode_base64_secret(encoded: &str) -> Result>, subtle_encoding::Error> { + // TODO(tarcieri): constant-time string trimming + let encoded = encoded.trim_end().as_bytes(); + + let mut decoded = Zeroizing::new(vec![0u8; Base64::default().decoded_len(encoded)?]); + let decoded_len = Base64::default().decode_to_slice(encoded, &mut decoded)?; + decoded.truncate(decoded_len); + + Ok(decoded) +} + /// Load Base64-encoded secret data (i.e. key) from the given path pub fn load_base64_secret(path: impl AsRef) -> Result>, Error> { warn_if_readable_by_others(path.as_ref()); @@ -60,17 +74,15 @@ pub fn load_base64_secret(path: impl AsRef) -> Result>, ) })?); - // TODO(tarcieri): constant-time string trimming - let data = Zeroizing::new(base64::decode(base64_data.trim_end()).map_err(|e| { + decode_base64_secret(&base64_data).map_err(|e| { format_err!( IoError, "can't decode key from `{}`: {}", path.as_ref().display(), e ) - })?); - - Ok(data) + .into() + }) } /// Load a Base64-encoded Ed25519 secret key diff --git a/tests/cli/yubihsm/keys/import.rs b/tests/cli/yubihsm/keys/import.rs index 848e6384..8b994b04 100644 --- a/tests/cli/yubihsm/keys/import.rs +++ b/tests/cli/yubihsm/keys/import.rs @@ -1,21 +1,193 @@ //! Integration tests for the `yubihsm keys import` subcommand use crate::cli; -use std::str; +use std::{fs, path::Path, str}; +use subtle_encoding::base64; +use tempfile::TempDir; -/// Arguments to import the given priv_validator key file as key 0x0001 -fn import_json_args(path: &str) -> Vec<&str> { +/// Arguments to run `yubihsm keys import` with `options` +fn import_args<'a>(options: &[&'a str]) -> Vec<&'a str> { #[allow(unused_mut)] let mut args = vec!["yubihsm", "keys", "import"]; #[cfg(feature = "yubihsm-mock")] args.extend_from_slice(&["-c", super::KMS_CONFIG_PATH]); - args.extend_from_slice(&["-t", "json"]); - args.extend_from_slice(&["-i", "1"]); // key ID - args.push(path); + args.extend_from_slice(options); args } +/// Arguments to import the given priv_validator key file as key 0x0001 +fn import_json_args(path: &str) -> Vec<&str> { + import_args(&["-t", "json", "-i", "1", path]) +} + +/// Run `yubihsm keys import` with `options`, and return its exit code and what it +/// wrote to stderr with the colour codes removed +fn import(options: &[&str]) -> (Option, String) { + run_writing_only_stderr(&import_args(options)) +} + +/// Run `tmkms` with `args`, check that it wrote nothing to stdout, and return its exit +/// code and what it wrote to stderr with the colour codes removed +fn run_writing_only_stderr(args: &[&str]) -> (Option, String) { + let out = cli::run(args); + assert_eq!( + str::from_utf8(&out.stdout).unwrap(), + "", + "stdout of {args:?}" + ); + + let stderr = str::from_utf8(&out.stderr).unwrap(); + (out.status.code(), without_colour_codes(stderr)) +} + +/// `text` without the ANSI colour codes `tmkms` writes even when not on a terminal +fn without_colour_codes(text: &str) -> String { + let mut plain = String::new(); + let mut chars = text.chars(); + + while let Some(c) = chars.next() { + if c == '\u{1b}' { + // A colour code runs from the escape character to the next `m` + chars.by_ref().find(|&c| c == 'm'); + } else { + plain.push(c); + } + } + + plain +} + +/// Write `contents` to a file named `name` in `dir` and return its path +fn key_file(dir: &TempDir, name: &str, contents: &str) -> String { + let path = dir.path().join(name); + fs::write(&path, contents).unwrap(); + path.display().to_string() +} + +/// `len` zero bytes, Base64-encoded +fn zeros_base64(len: usize) -> String { + String::from_utf8(base64::encode(vec![0; len])).unwrap() +} + +/// Every outcome of `yubihsm keys import` that needs no real HSM: the exit code and +/// every line the operator reads, for each key type and each way the import fails +#[test] +fn keys_import_reports_every_outcome() { + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("missing").display().to_string(); + let not_base64 = key_file(&dir, "not_base64", "not base64!"); + let short_wrap = key_file(&dir, "short_wrap", &zeros_base64(12)); + let unknown_wrap = key_file(&dir, "unknown_wrap", &zeros_base64(32)); + let empty_json = key_file(&dir, "empty.json", "{}"); + let short_key = key_file(&dir, "short_key", &zeros_base64(16)); + let expanded_key = key_file(&dir, "expanded_key", &zeros_base64(64)); + let json = super::PRIV_VALIDATOR_CONFIG_PATH; + let json_by_name = key_file( + &dir, + "priv_validator_key.json", + &fs::read_to_string(json).unwrap(), + ); + let seed = "tests/support/signing_ed25519.key"; + assert!(Path::new(seed).exists()); + + let no_key_id = "error: no key ID specified (use e.g. tmkms yubihsm keys import -i 1 priv_validator.json)\n"; + let wrapkey_ignored = + "warning: ignoring wrapkey ID: 5 (not applicable to priv_validator.json files)\n"; + let unknown_wrap_error = format!( + "error: error importing encrypted key from {unknown_wrap} (using wrapkey 0x0001): HSM error: HSM error: invalid command\n" + ); + + let cases: Vec<(Vec<&str>, Option, String)> = vec![ + // Any key type + ( + vec!["-t", "json", &missing], + Some(1), + format!( + "error: couldn't import file {missing}: No such file or directory (os error 2)\n" + ), + ), + ( + vec!["-t", "bogus", ¬_base64], + Some(1), + "error: invalid key type: bogus\n".into(), + ), + // Wrapped keys, the default for a file not named priv_validator_key.json + ( + vec![¬_base64], + Some(1), + format!( + "error: couldn't decode Base64-encoded wrapped key from {not_base64}: bad encoding\n" + ), + ), + ( + vec!["-t", "wrap", &short_wrap], + Some(1), + format!( + "error: couldn't parse wrapped key from {short_wrap}: \ + invalid message length: message must be at least 13-bytes\n" + ), + ), + ( + vec!["-t", "wrap", &unknown_wrap], + Some(1), + unknown_wrap_error.clone(), + ), + ( + vec!["-t", "wrap", "-i", "5", "-l", "foo", &unknown_wrap], + Some(1), + format!( + "warning: ignoring key ID: 5 (wrapped keys use original key ID)\n\ + warning: ignoring label: \"foo\" (wrapped keys use original label)\n\ + {unknown_wrap_error}" + ), + ), + // priv_validator JSON keys, the default for a file named priv_validator_key.json + (vec![&json_by_name], Some(1), no_key_id.into()), + (vec!["-t", "json", json], Some(1), no_key_id.into()), + ( + vec!["-t", "json", "-i", "1", &empty_json], + Some(1), + "error: couldn't parse priv_validator key: \ + StringTracer: serde json error: missing field `address` at line 1 column 2\n" + .into(), + ), + ( + vec!["-t", "json", "-i", "1", "-w", "5", json], + Some(0), + format!("{wrapkey_ignored} Imported key 0x0001\n"), + ), + // Base64 keys + (vec!["-t", "base64", seed], Some(1), no_key_id.into()), + ( + vec!["-t", "base64", "-i", "1", ¬_base64], + Some(1), + "error: can't decode Ed25519 key: bad encoding\n".into(), + ), + ( + vec!["-t", "base64", "-i", "1", &short_key], + Some(1), + "error: invalid Ed25519 key: \ + invalid key: invalid Ed25519 key size: expected 32, 64, or 96, but got 16\n" + .into(), + ), + ( + vec!["-t", "base64", "-i", "1", &expanded_key], + Some(1), + "error: Ed25519 key must be provided as a 32-byte seed\n".into(), + ), + ( + vec!["-t", "base64", "-i", "1", "-w", "5", seed], + Some(0), + format!("{wrapkey_ignored} Imported key 0x0001\n"), + ), + ]; + + for (args, code, stderr) in cases { + assert_eq!(import(&args), (code, stderr), "{args:?}"); + } +} + #[test] fn keys_import_priv_validator_test() { let args = import_json_args(super::PRIV_VALIDATOR_CONFIG_PATH); @@ -48,3 +220,33 @@ fn keys_import_rejects_a_secp256k1_priv_validator() { "{message}" ); } + +/// Connecting to the HSM ends the process when it fails, and nothing wipes what is in +/// memory then, so the import connects before it reads the key file +#[test] +fn keys_import_connects_before_reading_the_key_file() { + let dir = tempfile::tempdir().unwrap(); + let config_without_hsm = key_file(&dir, "tmkms.toml", "[providers]\n"); + let missing_key = dir.path().join("missing").display().to_string(); + + let args = [ + "yubihsm", + "keys", + "import", + "-c", + &config_without_hsm, + "-t", + "base64", + "-i", + "1", + &missing_key, + ]; + + assert_eq!( + run_writing_only_stderr(&args), + ( + Some(1), + "error: expected one [[providers.yubihsm]] in config, found: 0\n".to_owned() + ) + ); +} diff --git a/tests/secret_decoding.rs b/tests/secret_decoding.rs new file mode 100644 index 00000000..9a4d8cf1 --- /dev/null +++ b/tests/secret_decoding.rs @@ -0,0 +1,103 @@ +//! A Base64 key must not leave its decoded bytes in freed memory, even when it fails +//! to decode partway. This is its own test binary because checking that needs a +//! global allocator that inspects every buffer as it is freed. + +use std::{ + alloc::{GlobalAlloc, Layout, System}, + fs, slice, + sync::atomic::{AtomicBool, Ordering}, +}; +use subtle_encoding::base64; +use tmkms::key_utils; + +/// The secret the test keys hold. No other buffer in this process holds these bytes. +const SEED: [u8; 32] = [ + 0x5e, 0xed, 0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, + 0xdc, 0xfe, 0x0f, 0x1e, 0x2d, 0x3c, 0x4b, 0x5a, 0x69, 0x78, 0x87, 0x96, 0xa5, 0xb4, 0xc3, 0xd2, +]; + +/// Part of `SEED` that decoding keeps even when it fails on the first character +fn seed_middle() -> &'static [u8] { + &SEED[8..24] +} + +/// Set when a freed buffer still held `seed_middle()` +static FREED_SEED: AtomicBool = AtomicBool::new(false); + +/// The system allocator, recording whether a freed buffer still held the seed +struct SeedScanner; + +unsafe impl GlobalAlloc for SeedScanner { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + unsafe { System.alloc(layout) } + } + + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + let freed = unsafe { slice::from_raw_parts(ptr, layout.size()) }; + if freed + .windows(seed_middle().len()) + .any(|w| w == seed_middle()) + { + FREED_SEED.store(true, Ordering::SeqCst); + } + unsafe { System.dealloc(ptr, layout) } + } +} + +#[global_allocator] +static ALLOCATOR: SeedScanner = SeedScanner; + +/// One test, because the allocator's record is shared by everything in this process. +/// It covers both readers of a Base64 key: key load and `yubihsm keys import`. +#[test] +fn a_base64_key_leaves_no_decoded_bytes_in_freed_memory() { + let dir = tempfile::tempdir().unwrap(); + let encoded = String::from_utf8(base64::encode(SEED)).unwrap(); + // A base64url character where standard Base64 is expected, as in a key file + // written with the wrong alphabet + let misencoded = format!("-{}", &encoded[1..]); + + let key = dir.path().join("key"); + fs::write(&key, &encoded).unwrap(); + drop(key_utils::load_base64_secret(&key).unwrap()); + assert!( + !FREED_SEED.load(Ordering::SeqCst), + "a key that decodes left its bytes in freed memory" + ); + + let misencoded_key = dir.path().join("misencoded_key"); + fs::write(&misencoded_key, &misencoded).unwrap(); + assert!(key_utils::load_base64_secret(&misencoded_key).is_err()); + assert!( + !FREED_SEED.load(Ordering::SeqCst), + "a key that fails to decode left its bytes in freed memory" + ); + + #[cfg(feature = "yubihsm-mock")] + { + use tmkms::commands::yubihsm::keys::import::ImportCommand; + use yubihsm::{Client, Connector, Credentials}; + + // The key fails to decode before the HSM is reached, so its password is moot + let hsm = Client::create( + Connector::mockhsm(), + Credentials::from_password(1, b"wrong"), + ) + .unwrap(); + let import = ImportCommand { + key_id: Some(1), + key_type: Some("base64".to_owned()), + path: misencoded_key, + ..Default::default() + }; + + assert_eq!( + import.import(&hsm), + Err("can't decode Ed25519 key: bad encoding".to_owned()) + ); + assert!( + !FREED_SEED.load(Ordering::SeqCst), + "yubihsm keys import left a key that fails to decode in freed memory" + ); + } +}