From 56dba759fd425aa28d4ba8b03db3b2f89ae0b033 Mon Sep 17 00:00:00 2001 From: Justus Winter Date: Mon, 11 Dec 2023 14:37:02 +0100 Subject: [PATCH] Scan emitted data for inadvertent leaks of secret key material. - In debug builds, scan all emitted data for secret key material, and panic if we find something. Manually goodlist all the places where we expect to emit secret key material. --- src/cli/types.rs | 114 ++++++++++++++++++++-- src/commands/key/adopt.rs | 2 +- src/commands/key/attest_certifications.rs | 2 +- src/commands/key/generate.rs | 2 +- src/commands/key/password.rs | 2 +- src/commands/key/subkey.rs | 2 +- src/commands/key/userid.rs | 4 +- src/commands/keyring.rs | 6 +- 8 files changed, 116 insertions(+), 18 deletions(-) diff --git a/src/cli/types.rs b/src/cli/types.rs index 418ff351..a430f875 100644 --- a/src/cli/types.rs +++ b/src/cli/types.rs @@ -1,7 +1,7 @@ use std::fmt::Display; use std::fmt::Formatter; use std::fs::OpenOptions; -use std::io::Write; +use std::io::{self, Write}; use std::io::stdin; use std::io::stdout; use std::path::Path; @@ -250,7 +250,12 @@ impl ClapData for FileOrCertStore { /// } /// ``` #[derive(Clone, Debug, Eq, PartialEq)] -pub struct FileOrStdout(Option); +pub struct FileOrStdout { + path: Option, + + /// If set, secret keys may be written to this sink. + for_secrets: bool, +} impl ClapData for FileOrStdout { const VALUE_NAME: &'static str = "FILE"; @@ -259,12 +264,23 @@ impl ClapData for FileOrStdout { impl FileOrStdout { pub fn new(path: Option) -> Self { - FileOrStdout(path) + FileOrStdout { + path, + ..Default::default() + } + } + + /// Indicates that we will emit secrets. + /// + /// Use this to mark outputs where we intend to emit secret keys. + pub fn for_secrets(mut self) -> Self { + self.for_secrets = true; + self } /// Return a reference to the optional PathBuf pub fn path(&self) -> Option<&PathBuf> { - self.0.as_ref() + self.path.as_ref() } /// Opens the file (or stdout) for writing data that is safe for @@ -294,13 +310,21 @@ impl FileOrStdout { /// Opens the file (or stdout) for writing data that is safe for /// non-interactive use because it is an OpenPGP data stream. - pub fn create_pgp_safe( + /// + /// Emitting armored data with the label `armor::Kind::SecretKey` + /// implicitly configures this output to emit secret keys. + pub fn create_pgp_safe<'a>( &self, force: bool, binary: bool, kind: armor::Kind, - ) -> Result { - let sink = self.create_safe(force)?; + ) -> Result> { + // Allow secrets to be emitted if the armor label says secret + // key. + let mut o = self.clone(); + o.for_secrets |= kind == armor::Kind::SecretKey; + let sink = o.create_safe(force)?; + let mut message = Message::new(sink); if ! binary { message = Armorer::new(message).kind(kind).build()?; @@ -311,6 +335,18 @@ impl FileOrStdout { /// Helper function, do not use directly. Instead, use create_or_stdout_safe /// or create_or_stdout_unsafe. fn create(&self, force: bool) -> Result> { + let sink = self._create_sink(force)?; + if self.for_secrets || ! cfg!(debug_assertions) { + // We either expect secrets, or we are in release mode. + Ok(sink) + } else { + // In debug mode, if we don't expect secrets, scan the + // output for inadvertently leaked secret keys. + Ok(Box::new(SecretLeakDetector::new(sink))) + } + } + fn _create_sink(&self, force: bool) -> Result> + { if let Some(path) = self.path() { if !path.exists() || force { Ok(Box::new( @@ -335,7 +371,10 @@ impl FileOrStdout { impl Default for FileOrStdout { fn default() -> Self { - FileOrStdout(None) + FileOrStdout { + path: None, + for_secrets: false, + } } } @@ -380,6 +419,65 @@ impl Display for FileOrStdout { } } +/// A writer that buffers all data, and scans for secret keys on drop. +/// +/// This is used to assert that we only write secret keys in places +/// where we expect that. As this buffers all data, and has a +/// performance impact, we only do this in debug builds. +struct SecretLeakDetector { + sink: W, + data: Vec, +} + +impl io::Write for SecretLeakDetector { + fn write(&mut self, buf: &[u8]) -> io::Result { + let n = self.sink.write(buf)?; + self.data.extend_from_slice(&buf[..n]); + Ok(n) + } + + fn flush(&mut self) -> io::Result<()> { + self.sink.flush() + } +} + +impl Drop for SecretLeakDetector { + fn drop(&mut self) { + let _ = self.detect_leaks(); + } +} + +impl SecretLeakDetector { + /// Creates a shim around `sink` that scans for inadvertently + /// leaked secret keys. + fn new(sink: W) -> Self { + SecretLeakDetector { + sink, + data: Vec::with_capacity(4096), + } + } + + /// Scans the buffered data for secret keys, panic'ing if one is + /// found. + fn detect_leaks(&self) -> Result<()> { + use openpgp::Packet; + use openpgp::parse::{Parse, PacketParserResult, PacketParser}; + + let mut ppr = PacketParser::from_bytes(&self.data)?; + while let PacketParserResult::Some(pp) = ppr { + match &pp.packet { + Packet::SecretKey(_) | Packet::SecretSubkey(_) => + panic!("Leaked secret key: {:?}", pp.packet), + _ => (), + } + let (_, next_ppr) = pp.recurse()?; + ppr = next_ppr; + } + + Ok(()) + } +} + #[derive(ValueEnum, Debug, Clone)] pub enum ArmorKind { Auto, diff --git a/src/commands/key/adopt.rs b/src/commands/key/adopt.rs index 67fed2ca..3122e652 100644 --- a/src/commands/key/adopt.rs +++ b/src/commands/key/adopt.rs @@ -200,7 +200,7 @@ pub fn adopt(config: Config, command: cli::key::AdoptCommand) -> Result<()> let cert = cert.clone().insert_packets(packets.clone())?; - let mut sink = command.output.create_safe(config.force)?; + let mut sink = command.output.for_secrets().create_safe(config.force)?; if command.binary { cert.as_tsk().serialize(&mut sink)?; } else { diff --git a/src/commands/key/attest_certifications.rs b/src/commands/key/attest_certifications.rs index 76b2b32b..19024dda 100644 --- a/src/commands/key/attest_certifications.rs +++ b/src/commands/key/attest_certifications.rs @@ -62,7 +62,7 @@ pub fn attest_certifications( // Finally, add the new signatures. let key = key.insert_packets(attestation_signatures)?; - let mut sink = command.output.create_safe(config.force)?; + let mut sink = command.output.for_secrets().create_safe(config.force)?; if command.binary { key.as_tsk().serialize(&mut sink)?; } else { diff --git a/src/commands/key/generate.rs b/src/commands/key/generate.rs index 0f4296d0..ef875550 100644 --- a/src/commands/key/generate.rs +++ b/src/commands/key/generate.rs @@ -134,7 +134,7 @@ pub fn generate( .map(|value| ("Comment", value.as_str())) .collect(); - let w = command.output.create_safe(config.force)?; + let w = command.output.for_secrets().create_safe(config.force)?; let mut w = Writer::with_headers(w, Kind::SecretKey, headers)?; cert.as_tsk().serialize(&mut w)?; w.finalize()?; diff --git a/src/commands/key/password.rs b/src/commands/key/password.rs index 15bfadf4..a276af30 100644 --- a/src/commands/key/password.rs +++ b/src/commands/key/password.rs @@ -66,7 +66,7 @@ pub fn password( key = key.insert_packets(encrypted)?; } - let mut output = command.output.create_safe(config.force)?; + let mut output = command.output.for_secrets().create_safe(config.force)?; if command.binary { key.as_tsk().serialize(&mut output)?; } else { diff --git a/src/commands/key/subkey.rs b/src/commands/key/subkey.rs index 63b4c824..c3d57543 100644 --- a/src/commands/key/subkey.rs +++ b/src/commands/key/subkey.rs @@ -316,7 +316,7 @@ fn subkey_add( .set_primary_key_signer(primary_key) .attach_cert()?; - let mut sink = command.output.create_safe(config.force)?; + let mut sink = command.output.for_secrets().create_safe(config.force)?; if command.binary { new_cert.as_tsk().serialize(&mut sink)?; } else { diff --git a/src/commands/key/userid.rs b/src/commands/key/userid.rs index 130b4224..0e68511d 100644 --- a/src/commands/key/userid.rs +++ b/src/commands/key/userid.rs @@ -395,7 +395,7 @@ fn userid_add( // Merge additional User IDs into key let cert = key.insert_packets(add)?; - let mut sink = command.output.create_safe(config.force)?; + let mut sink = command.output.for_secrets().create_safe(config.force)?; if command.binary { cert.as_tsk().serialize(&mut sink)?; } else { @@ -449,7 +449,7 @@ signatures on other User IDs to make the key valid again.", } } - let mut sink = command.output.create_safe(config.force)?; + let mut sink = command.output.for_secrets().create_safe(config.force)?; if command.binary { cert.as_tsk().serialize(&mut sink)?; } else { diff --git a/src/commands/keyring.rs b/src/commands/keyring.rs index 0be3d86b..b1f9874e 100644 --- a/src/commands/keyring.rs +++ b/src/commands/keyring.rs @@ -135,7 +135,7 @@ pub fn dispatch(config: Config, c: keyring::Command) -> Result<()> { // better to use Kind::SecretKey here. However, this // requires buffering all certs, which has its own // problems. - let mut output = command.output.create_pgp_safe( + let mut output = command.output.for_secrets().create_pgp_safe( config.force, command.binary, armor::Kind::PublicKey, @@ -149,7 +149,7 @@ pub fn dispatch(config: Config, c: keyring::Command) -> Result<()> { // better to use Kind::SecretKey here. However, this // requires buffering all certs, which has its own // problems. - let mut output = c.output.create_pgp_safe( + let mut output = c.output.for_secrets().create_pgp_safe( config.force, c.binary, armor::Kind::PublicKey, @@ -158,7 +158,7 @@ pub fn dispatch(config: Config, c: keyring::Command) -> Result<()> { output.finalize() }, Merge(c) => { - let mut output = c.output.create_pgp_safe( + let mut output = c.output.for_secrets().create_pgp_safe( config.force, c.binary, armor::Kind::PublicKey,