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.
This commit is contained in:
Justus Winter 2023-12-11 14:37:02 +01:00
parent 8e1ae6e0a3
commit 56dba759fd
No known key found for this signature in database
GPG Key ID: 686F55B4AB2B3386
8 changed files with 116 additions and 18 deletions

@ -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<PathBuf>);
pub struct FileOrStdout {
path: Option<PathBuf>,
/// 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<PathBuf>) -> 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<Message> {
let sink = self.create_safe(force)?;
) -> Result<Message<'a>> {
// 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<Box<dyn Write + Sync + Send>> {
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<Box<dyn Write + Sync + Send>>
{
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<W: io::Write + Send + Sync> {
sink: W,
data: Vec<u8>,
}
impl<W: io::Write + Send + Sync> io::Write for SecretLeakDetector<W> {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
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<W: io::Write + Send + Sync> Drop for SecretLeakDetector<W> {
fn drop(&mut self) {
let _ = self.detect_leaks();
}
}
impl<W: io::Write + Send + Sync> SecretLeakDetector<W> {
/// 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,

@ -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 {

@ -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 {

@ -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()?;

@ -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 {

@ -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 {

@ -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 {

@ -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,