harden: validate PE output and timestamp tokens

src/main.rs

@@ -8,7 +8,9 @@ use ssign_core::{auth, authenticode, card, client, otp, sign, timestamp};
 
 use anyhow::{bail, Context, Result};
 use clap::Parser;
-use std::path::PathBuf;
+use std::fs::{self, File, OpenOptions};
+use std::io::{self, Write};
+use std::path::{Path, PathBuf};
 use std::time::{SystemTime, UNIX_EPOCH};
 
 /// Authenticode-sign Windows binaries (exe/dll/msi/sys) with a Certum SimplySign
@@ -162,18 +164,161 @@ fn run(cli: &Cli, otp: Otp) -> Result<()> {
             .with_context(|| format!("assembling signature for {}", file.display()))?;
 
         let out = output_path(file, cli.output_dir.as_deref())?;
-        if cli.backup && out == *file {
-            std::fs::write(file.with_extension("orig"), &pe).context("writing backup")?;
-        }
-        std::fs::write(&out, &signed).with_context(|| format!("writing {}", out.display()))?;
+        write_signed_file(file, &out, &pe, &signed, cli.backup)
+            .with_context(|| format!("writing {}", out.display()))?;
         println!("signed {}", out.display());
     }
     Ok(())
 }
 
+fn write_signed_file(
+    input: &Path,
+    out: &Path,
+    original: &[u8],
+    signed: &[u8],
+    backup: bool,
+) -> Result<()> {
+    let signed_temp = write_synced_temp(out, "signed", signed)?;
+    let result = (|| -> Result<()> {
+        match fs::metadata(out) {
+            Ok(metadata) => fs::set_permissions(&signed_temp, metadata.permissions())
+                .with_context(|| format!("preserving permissions for {}", out.display()))?,
+            Err(err) if err.kind() == io::ErrorKind::NotFound => {}
+            Err(err) => {
+                return Err(err).with_context(|| format!("reading metadata for {}", out.display()))
+            }
+        }
+
+        if backup && input == out {
+            publish_backup(input, out, original)?;
+        }
+
+        atomic_replace(&signed_temp, out)
+            .with_context(|| format!("atomically replacing {}", out.display()))?;
+        Ok(())
+    })();
+
+    if result.is_err() {
+        let _ = fs::remove_file(&signed_temp);
+    }
+    result
+}
+
+fn publish_backup(input: &Path, out: &Path, original: &[u8]) -> Result<()> {
+    let backup_path = backup_path(input)?;
+    let backup_temp = write_synced_temp(out, "backup", original)?;
+    let result = fs::hard_link(&backup_temp, &backup_path)
+        .with_context(|| format!("creating backup {}", backup_path.display()));
+    let _ = fs::remove_file(&backup_temp);
+    result
+}
+
+fn backup_path(input: &Path) -> Result<PathBuf> {
+    let mut backup_name = input
+        .file_name()
+        .context("input has no file name")?
+        .to_os_string();
+    backup_name.push(".orig");
+    Ok(input.with_file_name(backup_name))
+}
+
+fn write_synced_temp(out: &Path, purpose: &str, contents: &[u8]) -> Result<PathBuf> {
+    let (mut temp, temp_path) = create_temp_file(out, purpose)?;
+    let write_result = (|| -> io::Result<()> {
+        temp.write_all(contents)?;
+        temp.sync_all()
+    })();
+    drop(temp);
+
+    if let Err(err) = write_result {
+        let _ = fs::remove_file(&temp_path);
+        return Err(err).with_context(|| format!("writing temporary file {}", temp_path.display()));
+    }
+
+    Ok(temp_path)
+}
+
+fn create_temp_file(out: &Path, purpose: &str) -> Result<(File, PathBuf)> {
+    const MAX_ATTEMPTS: u32 = 32;
+
+    let directory = out
+        .parent()
+        .filter(|path| !path.as_os_str().is_empty())
+        .unwrap_or_else(|| Path::new("."));
+    let file_name = out.file_name().context("output has no file name")?;
+    let nonce = SystemTime::now()
+        .duration_since(UNIX_EPOCH)
+        .context("system clock before 1970")?
+        .as_nanos();
+
+    for attempt in 0..MAX_ATTEMPTS {
+        let mut temp_name = file_name.to_os_string();
+        temp_name.push(format!(
+            ".{purpose}.{}.{}.{}.tmp",
+            std::process::id(),
+            nonce,
+            attempt
+        ));
+        let temp_path = directory.join(temp_name);
+        match OpenOptions::new()
+            .write(true)
+            .create_new(true)
+            .open(&temp_path)
+        {
+            Ok(file) => return Ok((file, temp_path)),
+            Err(err) if err.kind() == io::ErrorKind::AlreadyExists => continue,
+            Err(err) => {
+                return Err(err)
+                    .with_context(|| format!("creating temporary file {}", temp_path.display()))
+            }
+        }
+    }
+
+    bail!(
+        "could not create a unique temporary file for {}",
+        out.display()
+    )
+}
+
+#[cfg(windows)]
+fn atomic_replace(temp: &Path, out: &Path) -> io::Result<()> {
+    use std::os::windows::ffi::OsStrExt;
+
+    const MOVEFILE_REPLACE_EXISTING: u32 = 0x0000_0001;
+    const MOVEFILE_WRITE_THROUGH: u32 = 0x0000_0008;
+
+    unsafe extern "system" {
+        fn MoveFileExW(
+            existing_file_name: *const u16,
+            new_file_name: *const u16,
+            flags: u32,
+        ) -> i32;
+    }
+
+    let temp_wide: Vec<u16> = temp.as_os_str().encode_wide().chain(Some(0)).collect();
+    let out_wide: Vec<u16> = out.as_os_str().encode_wide().chain(Some(0)).collect();
+    if unsafe {
+        MoveFileExW(
+            temp_wide.as_ptr(),
+            out_wide.as_ptr(),
+            MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH,
+        )
+    } == 0
+    {
+        Err(io::Error::last_os_error())
+    } else {
+        Ok(())
+    }
+}
+
+#[cfg(not(windows))]
+fn atomic_replace(temp: &Path, out: &Path) -> io::Result<()> {
+    fs::rename(temp, out)
+}
+
 /// Where a signed file is written: into `output_dir` (same file name) if given,
 /// otherwise in place.
-fn output_path(file: &std::path::Path, output_dir: Option<&std::path::Path>) -> Result<PathBuf> {
+fn output_path(file: &Path, output_dir: Option<&Path>) -> Result<PathBuf> {
     match output_dir {
         None => Ok(file.to_path_buf()),
         Some(dir) => {
@@ -183,3 +328,34 @@ fn output_path(file: &std::path::Path, output_dir: Option<&std::path::Path>) ->
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn unique_test_dir(label: &str) -> PathBuf {
+        let nonce = SystemTime::now()
+            .duration_since(UNIX_EPOCH)
+            .unwrap()
+            .as_nanos();
+        let dir =
+            std::env::temp_dir().join(format!("ssign-{label}-{}-{nonce}", std::process::id()));
+        std::fs::create_dir_all(&dir).unwrap();
+        dir
+    }
+
+    #[test]
+    fn in_place_backup_is_preserved_and_never_overwritten() {
+        let dir = unique_test_dir("backup");
+        let input = dir.join("app.exe");
+        std::fs::write(&input, b"original").unwrap();
+        write_signed_file(&input, &input, b"original", b"signed", true).unwrap();
+        assert_eq!(std::fs::read(&input).unwrap(), b"signed");
+        assert_eq!(
+            std::fs::read(dir.join("app.exe.orig")).unwrap(),
+            b"original"
+        );
+        assert!(write_signed_file(&input, &input, b"signed", b"new", true).is_err());
+        std::fs::remove_dir_all(dir).unwrap();
+    }
+}

ssign-core/src/authenticode.rs

@@ -85,7 +85,14 @@ pub fn pe_hash(pe: &[u8]) -> Result<[u8; 32]> {
     let (cert_start, cert_end) = if l.cert_table_size == 0 {
         (pe.len(), pe.len())
     } else {
-        (l.cert_table_off, l.cert_table_off + l.cert_table_size)
+        let cert_end = l
+            .cert_table_off
+            .checked_add(l.cert_table_size)
+            .context("certificate table length overflows")?;
+        if l.cert_table_off > pe.len() || cert_end > pe.len() {
+            bail!("certificate table lies outside the PE file");
+        }
+        (l.cert_table_off, cert_end)
     };
     let mut h = Sha256::new();
     h.update(&pe[..l.checksum_off]);
@@ -414,6 +421,30 @@ mod tests {
         assert_eq!(pe_hash(&odd).unwrap(), pe_hash(&padded).unwrap());
     }
 
+    #[test]
+    fn rejects_certificate_table_outside_the_file() {
+        let mut pe = vec![0u8; 0x200];
+        pe[..2].copy_from_slice(b"MZ");
+        pe[0x3c..0x40].copy_from_slice(&(0x80u32).to_le_bytes());
+        pe[0x80..0x84].copy_from_slice(b"PE\0\0");
+        pe[0x98..0x9a].copy_from_slice(&[0x0b, 0x01]);
+        pe[0x118..0x11c].copy_from_slice(&(0xffff_fff0u32).to_le_bytes());
+        pe[0x11c..0x120].copy_from_slice(&(0x20u32).to_le_bytes());
+        assert!(pe_hash(&pe).is_err());
+    }
+
+    #[test]
+    fn ignores_a_stale_certificate_offset_when_the_table_is_empty() {
+        let mut pe = vec![0u8; 0x200];
+        pe[..2].copy_from_slice(b"MZ");
+        pe[0x3c..0x40].copy_from_slice(&(0x80u32).to_le_bytes());
+        pe[0x80..0x84].copy_from_slice(b"PE\0\0");
+        pe[0x98..0x9a].copy_from_slice(&[0x0b, 0x01]);
+        pe[0x118..0x11c].copy_from_slice(&(0xffff_fff0u32).to_le_bytes());
+        pe[0x11c..0x120].copy_from_slice(&(0u32).to_le_bytes());
+        assert!(pe_hash(&pe).is_ok());
+    }
+
     #[test]
     fn pe_hash_matches_osslsigncode() {
         let pe = include_bytes!("../tests/fixtures/hello.exe");

ssign-core/src/timestamp.rs

@@ -7,6 +7,7 @@ use sha2::{Digest, Sha256};
 use std::time::Duration;
 
 const OID_SHA256: &str = "2.16.840.1.101.3.4.2.1";
+const OID_PKCS7_SIGNED_DATA: &str = "1.2.840.113549.1.7.2";
 
 /// Request a timestamp over `signature` from the RFC3161 TSA at `url`; returns
 /// the DER `TimeStampToken` (a PKCS#7 ContentInfo).
@@ -54,5 +55,42 @@ pub fn fetch(url: &str, signature: &[u8]) -> Result<Vec<u8>> {
         bail!("TSA rejected the request (status {status})");
     }
     let token = top.get(1).context("TSR has no timeStampToken")?;
+    validate_timestamp_token(token)?;
     Ok(token.to_vec())
 }
+
+fn validate_timestamp_token(token: &[u8]) -> Result<()> {
+    if token.first() != Some(&0x30) {
+        bail!("timeStampToken is not a DER SEQUENCE");
+    }
+    let children = asn1::children(token).map_err(|e| anyhow::anyhow!("bad timeStampToken: {e}"))?;
+    if children.len() != 2 || children[0] != asn1::oid(OID_PKCS7_SIGNED_DATA).as_slice() {
+        bail!("timeStampToken is not CMS SignedData");
+    }
+    let wrapped = children[1];
+    if wrapped.first() != Some(&0xa0) {
+        bail!("CMS SignedData is missing its explicit wrapper");
+    }
+    let signed_data =
+        asn1::children(wrapped).map_err(|e| anyhow::anyhow!("bad CMS SignedData wrapper: {e}"))?;
+    if signed_data.len() != 1 || signed_data[0].first() != Some(&0x30) {
+        bail!("CMS SignedData wrapper did not contain SignedData");
+    }
+    Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn accepts_only_cms_signed_data_tokens() {
+        let signed_data = asn1::seq(&[]);
+        let token = asn1::seq(&[
+            &asn1::oid("1.2.840.113549.1.7.2"),
+            &asn1::ctx(0, &signed_data),
+        ]);
+        assert!(validate_timestamp_token(&token).is_ok());
+        assert!(validate_timestamp_token(&asn1::seq(&[])).is_err());
+    }
+}