diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index 7aa9e0a..42027dc 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -31,6 +31,9 @@ jobs: - name: Portable digest CLI integration test run: cargo test -p psign --test cli_pe_digest --locked + - name: Portable MSIX/AppX native makeappx.exe validation + run: cargo test -p psign --test msix_native_makeappx --locked + - name: Cross-CLI parity (portable verify-pe vs Windows rust-sip PE digest routine) run: cargo test -p psign --test cross_cli_windows --locked diff --git a/crates/psign-portable-core/src/lib.rs b/crates/psign-portable-core/src/lib.rs index e9f15fd..c9cd485 100644 --- a/crates/psign-portable-core/src/lib.rs +++ b/crates/psign-portable-core/src/lib.rs @@ -3421,7 +3421,12 @@ fn build_flat_msix_block_map( r#""# ); for (name, data) in payloads { - let escaped_name = xml_escape_attr(name); + // `AppxBlockMap.xml` `File/@Name` uses Windows-style backslash separators + // (matching native `AppxSip`/`makeappx` output), even though the physical + // ZIP entry name uses forward slashes. Getting this wrong makes real Windows + // AppX package validation fail with 0x80080205 ("block map is not valid"). + let block_map_name = name.replace('/', "\\"); + let escaped_name = xml_escape_attr(&block_map_name); xml.push_str(&format!( r#""#, data.len(), @@ -3599,6 +3604,128 @@ mod tests { ); } + /// Regression test for the HRESULT 0x80080205 ("The Appx package's block map + /// is invalid") corruption reported against `--mode portable sign` for flat + /// `.msix`/`.appx` packages. + /// + /// Root cause: `build_flat_msix_block_map` emitted `AppxBlockMap.xml` + /// `File/@Name` attributes using the physical ZIP entry's forward-slash path + /// separators (e.g. `Assets/StoreLogo.png`), but native AppX packages (as + /// produced by `makeappx`/`AppxSip.dll`, and required by the real Windows + /// AppX package validator) always use backslash separators in the block map + /// (`Assets\StoreLogo.png`) even though the physical ZIP entry name itself + /// stays forward-slash. This test signs a real MSIX fixture and then + /// independently re-parses the produced ZIP container (via a fresh + /// `ZipArchive` read, not by reusing `msix_digest`'s internal state) to + /// assert that every `AppxBlockMap.xml` `File/@Name` uses backslash + /// separators, while the physical ZIP entry names remain forward-slash. + #[test] + fn signed_flat_msix_block_map_uses_backslash_separators() { + let fixture_dir = PathBuf::from("../../tests/fixtures/devolutions-authenticode"); + let source = PathBuf::from("../../tests/fixtures/generated-unsigned/msix/sample.msix"); + + let temp_dir = std::env::temp_dir().join(format!( + "psign-portable-msix-blockmap-sep-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + )); + std::fs::create_dir_all(&temp_dir).expect("create temp dir"); + let output = temp_dir.join("sample.signed.msix"); + + portable_sign(PortableSignRequest { + path: source, + output_path: Some(output.clone()), + pfx_path: Some(fixture_dir.join("authenticode-test-cert.pfx")), + pfx_password: Some("CodeSign123!".to_string()), + ..default_sign_request() + }) + .expect("sign flat MSIX package"); + + let signed_bytes = std::fs::read(&output).expect("read signed MSIX package"); + let mut archive = + ZipArchive::new(std::io::Cursor::new(&signed_bytes)).expect("open signed MSIX zip"); + + // Independently collect the physical ZIP entry names: these must stay + // forward-slash (this is the correct, unaffected convention). + let mut physical_names = Vec::new(); + for i in 0..archive.len() { + let entry = archive.by_index(i).expect("read zip entry"); + physical_names.push(entry.name().to_string()); + } + assert!( + physical_names.iter().any(|n| n == "Assets/StoreLogo.png"), + "expected physical zip entry with forward-slash name, got: {physical_names:?}" + ); + assert!( + !physical_names.iter().any(|n| n.contains('\\')), + "physical zip entry names must never contain backslashes, got: {physical_names:?}" + ); + + let block_map_xml = { + let mut entry = archive + .by_name("AppxBlockMap.xml") + .expect("AppxBlockMap.xml entry present"); + let mut buf = String::new(); + std::io::Read::read_to_string(&mut entry, &mut buf).expect("read AppxBlockMap.xml"); + buf + }; + + // Independently extract every `File Name="..."` attribute value using a + // small ad-hoc parse (deliberately not reusing any block-map-writing + // helper from this module). + let mut file_names = Vec::new(); + let marker = " Result<()> { + // Do NOT special-case `response.skipped`: a skip (e.g. `--skip-signed`) can + // still carry a failed self-check when `target_should_skip_signed` treats + // mere `AppxSignature.p7x` presence as "already signed" even though the + // embedded signature's digests don't match the package. Requiring `Valid` + // unconditionally preserves the exact silent-success case this guard + // exists to prevent; a genuinely valid skipped file already reports + // `Valid` here. + match response.signature.status { + psign_portable_core::PortableSignatureStatus::Valid => Ok(()), + status => Err(anyhow!( + "portable sign produced a signature that failed self-verification for '{}': {status:?} ({})", + target.display(), + response.signature.status_message + )), + } +} + fn appinstaller_companion_path(path: &Path) -> PathBuf { path.with_extension( path.extension() @@ -1093,14 +1119,13 @@ fn run_portable_sign_portable_core_artifact_signing( artifact_signing_exclude_credentials: std::mem::take(&mut exclude_credentials), ..Default::default() }; - psign_portable_core::portable_sign(request) - .map(|_| ()) - .with_context(|| { - format!( - "portable Artifact Signing {target_kind} target '{}'", - target.display() - ) - }) + let response = psign_portable_core::portable_sign(request).with_context(|| { + format!( + "portable Artifact Signing {target_kind} target '{}'", + target.display() + ) + })?; + ensure_valid_signature_response(&response, target) } #[cfg(not(feature = "artifact-signing-rest"))] @@ -1556,6 +1581,7 @@ mod tests { use crate::cli::{Cli, Command}; use clap::Parser; use std::collections::HashSet; + use std::path::{Path, PathBuf}; #[test] fn deduplicates_existing_path_aliases() { @@ -1608,4 +1634,89 @@ mod tests { assert_eq!(targets, vec![target.clone(), target]); assert!(has_duplicate_target_identities(&targets)); } + + #[test] + fn ensure_valid_signature_response_accepts_valid_status() { + let response = fake_sign_response(psign_portable_core::PortableSignatureStatus::Valid); + super::ensure_valid_signature_response(&response, Path::new("target.msix")) + .expect("Valid status must not be rejected"); + } + + #[test] + fn ensure_valid_signature_response_rejects_hash_mismatch_status() { + // Regression guard: the `--mode portable sign` CLI must not silently + // replace the original file when the internal digest self-check + // (AXPC/AXCD/AXCT/AXBM consistency for MSIX, or equivalent for other + // formats) reports anything other than `Valid`. + let response = + fake_sign_response(psign_portable_core::PortableSignatureStatus::HashMismatch); + let err = super::ensure_valid_signature_response(&response, Path::new("target.msix")) + .expect_err("HashMismatch status must fail the sign command"); + let message = err.to_string(); + assert!( + message.contains("target.msix"), + "unexpected message: {message}" + ); + assert!( + message.contains("HashMismatch"), + "unexpected message: {message}" + ); + } + + #[test] + fn ensure_valid_signature_response_rejects_hash_mismatch_even_when_skipped() { + // Regression guard for the reviewer-identified silent-success case: + // `--skip-signed` can report `skipped: true` for an MSIX whose + // embedded signature is already present but fails digest + // verification (`target_should_skip_signed` only checks for + // `AppxSignature.p7x` presence, not validity). The status must still + // gate the CLI even when `skipped` is set. + let mut response = + fake_sign_response(psign_portable_core::PortableSignatureStatus::HashMismatch); + response.skipped = true; + let err = super::ensure_valid_signature_response(&response, Path::new("target.msix")) + .expect_err("a skipped response with a failed self-check must still fail"); + assert!( + err.to_string().contains("HashMismatch"), + "unexpected message: {err}" + ); + } + + #[test] + fn ensure_valid_signature_response_accepts_valid_status_when_skipped() { + let mut response = fake_sign_response(psign_portable_core::PortableSignatureStatus::Valid); + response.skipped = true; + super::ensure_valid_signature_response(&response, Path::new("target.msix")) + .expect("a genuinely valid skipped file must still pass"); + } + + fn fake_sign_response( + status: psign_portable_core::PortableSignatureStatus, + ) -> psign_portable_core::PortableSignResponse { + psign_portable_core::PortableSignResponse { + schema_version: 1, + input_path: PathBuf::from("target.msix"), + output_path: PathBuf::from("target.msix"), + format: psign_portable_core::PortableFileFormat::Msix, + signature: psign_portable_core::PortableSignatureResponse { + schema_version: 1, + path: PathBuf::from("target.msix"), + format: psign_portable_core::PortableFileFormat::Msix, + status, + status_message: "test fixture".to_string(), + trust_status: None, + signature_count: 1, + signer_index: None, + signer_certificate_der_base64: None, + timestamper_certificate_der_base64: None, + embedded_certificate_count: 0, + digest_algorithm: None, + timestamp_kinds: Vec::new(), + timestamp_signing_time: None, + pkcs7_der_base64: None, + diagnostics: Vec::new(), + }, + skipped: false, + } + } } diff --git a/tests/msix_native_makeappx.rs b/tests/msix_native_makeappx.rs new file mode 100644 index 0000000..e544665 --- /dev/null +++ b/tests/msix_native_makeappx.rs @@ -0,0 +1,133 @@ +#![cfg(windows)] + +//! Native Windows AppX validation regression test for the HRESULT **0x80080205** +//! ("The Appx package's block map is invalid") corruption reported against +//! `psign-tool --mode portable sign` for flat `.msix`/`.appx` packages. +//! +//! `psign`'s own `portable verify-msix` self-check reuses the same +//! `psign-sip-digest` code that produced the signature, so it cannot detect a +//! systematic modeling divergence from real Windows AppX semantics (such as the +//! block-map path-separator bug this test guards against). This test instead +//! shells out to the real Windows SDK `makeappx.exe unpack` tool — the same +//! validator behind `Add-AppxPackage`'s block-map check — against a package +//! freshly signed by `psign-tool --mode portable sign`. +//! +//! The test is skipped (not failed) when `makeappx.exe` cannot be located, +//! since not every Windows host is guaranteed to have the Windows SDK +//! installed; `windows-latest` GitHub Actions runners do include it. + +use assert_cmd::Command; +use std::path::{Path, PathBuf}; +use std::process::Command as StdCommand; + +/// Locate `makeappx.exe` via `PATH`, then by scanning the usual Windows Kits +/// install locations (newest version first). +fn find_makeappx() -> Option { + if let Ok(path) = which_in_path("makeappx.exe") { + return Some(path); + } + + for program_files in ["C:\\Program Files (x86)", "C:\\Program Files"] { + let bin_root = Path::new(program_files).join("Windows Kits\\10\\bin"); + let Ok(entries) = std::fs::read_dir(&bin_root) else { + continue; + }; + let mut versions: Vec = entries.flatten().map(|e| e.path()).collect(); + // Prefer the highest SDK version (lexicographic sort works for the + // `10.0.XXXXX.0` naming scheme). + versions.sort(); + for version_dir in versions.into_iter().rev() { + for arch in ["x64", "x86", "arm64"] { + let candidate = version_dir.join(arch).join("makeappx.exe"); + if candidate.is_file() { + return Some(candidate); + } + } + } + } + None +} + +fn which_in_path(exe: &str) -> Result { + let path_var = std::env::var_os("PATH").ok_or(())?; + for dir in std::env::split_paths(&path_var) { + let candidate = dir.join(exe); + if candidate.is_file() { + return Ok(candidate); + } + } + Err(()) +} + +fn unique_temp_dir(label: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "psign-makeappx-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + )); + std::fs::create_dir_all(&dir).expect("create temp dir"); + dir +} + +#[test] +fn portable_signed_flat_msix_unpacks_cleanly_with_real_makeappx() { + let Some(makeappx) = find_makeappx() else { + eprintln!( + "skipping: makeappx.exe not found (Windows SDK not installed); \ + this test only runs where the real Windows AppX validator is available" + ); + return; + }; + + let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let source = manifest_dir.join("tests/fixtures/generated-unsigned/msix/sample.msix"); + let pfx = + manifest_dir.join("tests/fixtures/devolutions-authenticode/authenticode-test-cert.pfx"); + assert!(source.is_file(), "fixture missing: {}", source.display()); + assert!(pfx.is_file(), "fixture missing: {}", pfx.display()); + + let temp_dir = unique_temp_dir("flat-msix"); + let signed = temp_dir.join("sample.signed.msix"); + std::fs::copy(&source, &signed).expect("copy fixture to scratch path"); + + let mut sign_cmd = Command::cargo_bin("psign-tool").expect("psign-tool binary"); + sign_cmd + .arg("--mode") + .arg("portable") + .arg("sign") + .arg("--pfx") + .arg(&pfx) + .arg("--password") + .arg("CodeSign123!") + .arg(&signed); + sign_cmd.assert().success(); + + let unpack_dir = temp_dir.join("unpacked"); + let output = StdCommand::new(&makeappx) + .arg("unpack") + .arg("/p") + .arg(&signed) + .arg("/d") + .arg(&unpack_dir) + .arg("/o") + .output() + .expect("run makeappx unpack"); + + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success(), + "makeappx unpack failed on psign-signed MSIX (regression for HRESULT \ + 0x80080205 block-map corruption):\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + assert!( + !stdout.contains("0x80080205") && !stderr.contains("0x80080205"), + "makeappx reported the block-map-invalid HRESULT despite a successful \ + exit code; stdout:\n{stdout}\nstderr:\n{stderr}" + ); + + let _ = std::fs::remove_dir_all(temp_dir); +}