From 390aa49cbae5a3f16f086e769df7ff8726bd7611 Mon Sep 17 00:00:00 2001 From: Bryan Oltman Date: Thu, 23 May 2024 14:41:02 -0400 Subject: [PATCH] feat: update patch manager to check signature on boot (#171) * feat: store patch signature on disk with patch metadata * add note about explicit lifetime * fix tests * feat: update patch manager to check signature on boot * remove unused imports * fix tests * add log when no public key detected * clean up log message * pr feedback * rename test * rename tests null -> none * change signature field name to hash_signature * Add comment * update print statements for clearer device logs * fix test * improve docs * cleanup of signing error handling --- Cargo.lock | 19 ++- library/Cargo.toml | 2 + library/src/c_api/mod.rs | 4 +- library/src/cache/mod.rs | 1 + library/src/cache/patch_manager.rs | 184 ++++++++++++++++++++++++----- library/src/cache/signing.rs | 134 +++++++++++++++++++++ library/src/cache/updater_state.rs | 49 +++++--- library/src/network.rs | 2 +- library/src/updater.rs | 114 ++++++++++++------ 9 files changed, 422 insertions(+), 87 deletions(-) create mode 100644 library/src/cache/signing.rs diff --git a/Cargo.lock b/Cargo.lock index 83a5e83..635a4bb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -107,6 +107,12 @@ version = "0.21.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "35636a1494ede3b646cc98f74f8e62c773a38a659ebc777a2cf26b9b74171df9" +[[package]] +name = "base64" +version = "0.22.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6" + [[package]] name = "bidiff" version = "1.0.0" @@ -1220,7 +1226,7 @@ version = "0.11.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "046cd98826c46c2ac8ddecae268eb5c2e58628688a5fc7a2643704a73faba95b" dependencies = [ - "base64", + "base64 0.21.5", "bytes", "encoding_rs", "futures-core", @@ -1256,16 +1262,17 @@ dependencies = [ [[package]] name = "ring" -version = "0.17.7" +version = "0.17.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "688c63d65483050968b2a8937f7995f443e27041a0f7700aa59b0822aedebb74" +checksum = "c17fa4cb658e3583423e915b9f3acc01cceaee1860e33d59ebae66adc3a2dc0d" dependencies = [ "cc", + "cfg-if", "getrandom", "libc", "spin", "untrusted", - "windows-sys 0.48.0", + "windows-sys 0.52.0", ] [[package]] @@ -1305,7 +1312,7 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1c74cae0a4cf6ccbbf5f359f08efdf8ee7e1dc532573bf0db71968cb56b1448c" dependencies = [ - "base64", + "base64 0.21.5", ] [[package]] @@ -1755,6 +1762,7 @@ version = "0.1.0" dependencies = [ "android_logger", "anyhow", + "base64 0.22.1", "bipatch", "cbindgen", "comde", @@ -1770,6 +1778,7 @@ dependencies = [ "oslog", "pipe", "reqwest", + "ring", "serde", "serde_json", "serde_yaml", diff --git a/library/Cargo.toml b/library/Cargo.toml index 537fd45..a45b6f0 100644 --- a/library/Cargo.toml +++ b/library/Cargo.toml @@ -14,6 +14,7 @@ crate-type = ["lib", "cdylib", "staticlib"] [dependencies] # Used for error handling for now. anyhow = { version = "1.0.69", features = ["backtrace"] } +base64 = "0.22.0" # For inflating compressed patch files. bipatch = "1.0.0" # Used for exposing C API @@ -41,6 +42,7 @@ reqwest = { version = "0.11", default-features = false, features = [ "json", "rustls-tls", ] } +ring = "0.17.8" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0.93" # For reading shorebird.yaml diff --git a/library/src/c_api/mod.rs b/library/src/c_api/mod.rs index c014477..f195b94 100644 --- a/library/src/c_api/mod.rs +++ b/library/src/c_api/mod.rs @@ -480,7 +480,7 @@ mod test { number: 1, hash: hash.to_owned(), download_url: "ignored".to_owned(), - signature: None, + hash_signature: None, }), }) }, @@ -583,7 +583,7 @@ mod test { number: 1, hash: "ignored".to_owned(), download_url: "ignored".to_owned(), - signature: None, + hash_signature: None, }), }) }, diff --git a/library/src/cache/mod.rs b/library/src/cache/mod.rs index c50ac7b..c6ae0e5 100644 --- a/library/src/cache/mod.rs +++ b/library/src/cache/mod.rs @@ -1,5 +1,6 @@ mod disk_io; mod patch_manager; +mod signing; pub mod updater_state; pub use updater_state::UpdaterState; diff --git a/library/src/cache/patch_manager.rs b/library/src/cache/patch_manager.rs index cf10c5e..b98ef58 100644 --- a/library/src/cache/patch_manager.rs +++ b/library/src/cache/patch_manager.rs @@ -1,4 +1,4 @@ -use super::{disk_io, PatchInfo}; +use super::{disk_io, signing, PatchInfo}; use anyhow::{bail, Context, Result}; use core::fmt::Debug; use serde::{Deserialize, Serialize}; @@ -124,18 +124,23 @@ pub struct PatchManager { /// Metadata about the patches we have downloaded that is persisted to disk. patches_state: PatchesState, + + /// The key used to sign patch hashes for the current release, if any. If this is + /// not None, all patches must have a signature that can be verified with this key. + patch_public_key: Option, } impl PatchManager { /// Creates a new PatchManager with the given root directory. This directory is /// assumed to exist. The PatchManager will use this directory to store its /// state and patch binaries. - pub fn with_root_dir(root_dir: PathBuf) -> Self { + pub fn new(root_dir: PathBuf, patch_public_key: Option<&str>) -> Self { let patches_state = Self::load_patches_state(&root_dir).unwrap_or_default(); Self { root_dir, patches_state, + patch_public_key: patch_public_key.map(|s| s.to_owned()), } } @@ -229,6 +234,20 @@ impl PatchManager { } } + if let Some(public_key) = &self.patch_public_key { + // If we have a public key, verify that the patch's hash has a signature. + let signature = patch + .signature + .clone() + .context("Patch signature is missing")?; + + // Check that the signature is valid. + let patch_hash = signing::hash_file(&artifact_path)?; + signing::check_signature(&patch_hash, &signature, public_key)?; + } else { + info!("No public key provided, skipping signature verification"); + } + Ok(()) } @@ -471,15 +490,32 @@ impl ManagePatches for PatchManager { #[cfg(test)] impl PatchManager { pub fn manager_for_test(temp_dir: &TempDir) -> PatchManager { - PatchManager::with_root_dir(temp_dir.path().to_owned()) + PatchManager::new(temp_dir.path().to_owned(), None) } pub fn add_patch_for_test(&mut self, temp_dir: &TempDir, patch_number: usize) -> Result<()> { + self.add_signed_patch_for_test(temp_dir, patch_number, "hash", None) + } + + pub fn add_signed_patch_for_test( + &mut self, + temp_dir: &TempDir, + patch_number: usize, + hash: &str, + signature: Option<&str>, + ) -> Result<()> { let file_path = &temp_dir .path() .join(format!("patch{}.vmcode", patch_number)); std::fs::write(file_path, patch_number.to_string().repeat(patch_number)).unwrap(); - self.add_patch(patch_number, file_path, "hash", None) + info!( + "Adding patch {} with contents {} hash {} at {}", + patch_number, + patch_number.to_string().repeat(patch_number), + hash, + file_path.display() + ); + self.add_patch(patch_number, file_path, hash, signature) } } @@ -492,18 +528,17 @@ mod debug_tests { #[test] fn manage_patches_is_debug() { let temp_dir = TempDir::new("patch_manager").unwrap(); - let patch_manager: Box = Box::new( - super::PatchManager::with_root_dir(temp_dir.path().to_owned()), - ); + let patch_manager: Box = + Box::new(PatchManager::manager_for_test(&temp_dir)); assert_eq!(format!("{:?}", patch_manager), "ManagePatches"); } #[test] fn patch_manager_is_debug() { let temp_dir = TempDir::new("patch_manager").unwrap(); - let patch_manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let patch_manager = PatchManager::new(temp_dir.path().to_owned(), Some("public_key")); let expected_str = format!( - "PatchManager {{ root_dir: \"{}\", patches_state: PatchesState {{ last_booted_patch: None, last_attempted_patch: None, next_boot_patch: None, highest_seen_patch_number: None }} }}", + "PatchManager {{ root_dir: \"{}\", patches_state: PatchesState {{ last_booted_patch: None, last_attempted_patch: None, next_boot_patch: None, highest_seen_patch_number: None }}, patch_public_key: Some(\"public_key\") }}", temp_dir.path().display() ); assert_eq!(format!("{:?}", patch_manager), expected_str); @@ -534,7 +569,7 @@ mod add_patch_tests { let patch_number = 1; let patch_file_contents = "patch contents"; let temp_dir = TempDir::new("patch_manager").unwrap(); - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); let file_path = &temp_dir.path().join("patch1.vmcode"); std::fs::write(file_path, patch_file_contents).unwrap(); @@ -632,7 +667,7 @@ mod next_boot_patch_tests { #[test] fn returns_none_if_no_next_boot_patch() { let temp_dir = TempDir::new("patch_manager").unwrap(); - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); assert!(manager.next_boot_patch().is_none()); } @@ -662,7 +697,7 @@ mod next_boot_patch_tests { fn clears_current_and_next_on_boot_failure_if_they_are_the_same() -> Result<()> { let patch_file_contents = "patch contents"; let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); let file_path = &temp_dir.path().join("patch1.vmcode"); std::fs::write(file_path, patch_file_contents)?; assert!(manager.add_patch(1, file_path, "hash", None).is_ok()); @@ -688,7 +723,7 @@ mod next_boot_patch_tests { fn falls_back_to_last_booted_patch_if_still_bootable() -> Result<()> { let patch_file_contents = "patch contents"; let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); let file_path = &temp_dir.path().join("patch1.vmcode"); std::fs::write(file_path, patch_file_contents)?; @@ -714,7 +749,7 @@ mod next_boot_patch_tests { fn does_not_fall_back_to_last_booted_patch_if_corrupted() -> Result<()> { let patch_file_contents = "patch contents"; let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); let file_path = &temp_dir.path().join("patch1.vmcode"); std::fs::write(file_path, patch_file_contents)?; @@ -741,7 +776,7 @@ mod next_boot_patch_tests { } #[test] - fn returns_null_patch_if_first_patch_failed_to_boot() -> Result<()> { + fn returns_none_patch_if_first_patch_failed_to_boot() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; let mut manager = PatchManager::manager_for_test(&temp_dir); @@ -778,7 +813,7 @@ mod next_boot_patch_tests { } #[test] - fn returns_null_if_first_patch_did_not_successfully_boot() -> Result<()> { + fn returns_none_if_first_patch_did_not_successfully_boot() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; let mut manager = PatchManager::manager_for_test(&temp_dir); @@ -792,7 +827,7 @@ mod next_boot_patch_tests { } #[test] - fn returns_null_if_next_patch_did_not_successfully_boot() -> Result<()> { + fn returns_none_if_next_patch_did_not_successfully_boot() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; let mut manager = PatchManager::manager_for_test(&temp_dir); @@ -810,6 +845,97 @@ mod next_boot_patch_tests { Ok(()) } + + // The constant values below were generated by taking an arbitrary sha256 hash (INFLATED_PATCH_HASH) + // and using openssl to sign it with the private key corresponding to `PUBLIC_KEY`. + + // The base64-encoded public key in a DER format. This is required by ring to verify signatures. + // See https://docs.rs/ring/latest/ring/signature/index.html#signing-and-verifying-with-rsa-pkcs1-15-padding + const PUBLIC_KEY: &str = "MIIBCgKCAQEA2wdpEGbuvlPsb9i0qYrfMefJnEw1BHTi8SYZTKrXOvJWmEpPE1hWfbkvYzXu5a96gV1yocF3DMwn04VmRlKhC4AhsD0NL0UNhYhotbKG91Kwi1vAXpHhCdz5gQEBw0K1uB4Jz+zK6WK+31PryYpwLwbyXNqXoY8IAAUQ4STsHYV5w+BMSi8pepWMRd7DR9RHcbNOZlJvdBQ5NxvB4JN4dRMq8cC73ez1P9d7Dfwv3TWY+he9EmuXLT2UivZSlHIrGBa7MFfqyUe2ro0F7Te/B0si12itBbWIqycvqcXjeOPNn6WEpqN7IWjb9LUh162JyYaz5Lb/VeeJX8LKtElccwIDAQAB"; + + // The message that was signed. In practice, this will be the sha256 hash of an inflated patch artifact. + const INFLATED_PATCH_HASH: &str = + "6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b"; + + // The base64-encoded signature of `INFLATED_PATCH_HASH` created using the private key corresponding + // to `PUBLIC_KEY`. + const SIGNATURE: &str = "ZGccldv01XqHQ76bXuKV/9EQnNK0Q+reQ9bJHVnGfLldF+BLRx0divgPfKP5Df9BJPA3dw1Z1VortfepmMGebP3kS593l5zoktu9MIepxvRAFWNKE5PDTIIvCL/ddTPEHt6NNCeD6HLOMLzbEX3cFZa+lq3UymGi0aqA5DlXirJBGtopojc9nOXZ22n/qHNZIHEkGcqKbSMSK9oC55whKHnlJTbCXdmSyDc65B4PcgseqJom1riVK3XGW1YMrSpuMAU+CDT7HhdESmI1UtH1bYeBITfRhQztdDTfti2vJTf2Y+lYC99CFiISgD7f1m0KUcC+VnEAMZSYtgxSk6AX2A=="; + + #[test] + fn returns_none_if_public_key_is_invalid() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::new(temp_dir.path().to_path_buf(), Some("not a valid key")); + + manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, Some(SIGNATURE))?; + + assert!(manager.next_boot_patch().is_none()); + + Ok(()) + } + + #[test] + fn returns_none_if_patch_is_missing_expected_signature() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::new(temp_dir.path().to_path_buf(), Some(PUBLIC_KEY)); + + manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, None)?; + + assert!(manager.next_boot_patch().is_none()); + + Ok(()) + } + + #[test] + fn returns_none_if_patch_has_invalid_signature() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::new(temp_dir.path().to_path_buf(), Some(PUBLIC_KEY)); + + // Using MESSAGE as a signature because it is valid base64, but not a valid signature. + manager.add_signed_patch_for_test( + &temp_dir, + 1, + INFLATED_PATCH_HASH, + Some(INFLATED_PATCH_HASH), + )?; + + assert!(manager.next_boot_patch().is_none()); + + Ok(()) + } + + #[test] + fn returns_patch_if_patch_has_valid_signature() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + let mut manager = PatchManager::new(temp_dir.path().to_path_buf(), Some(PUBLIC_KEY)); + + manager.add_signed_patch_for_test(&temp_dir, 1, INFLATED_PATCH_HASH, Some(SIGNATURE))?; + + assert!(manager.next_boot_patch().is_some()); + let patch = manager.next_boot_patch().unwrap(); + assert_eq!(patch.number, 1); + + Ok(()) + } + + #[test] + fn returns_patch_with_arbitrary_signature_if_no_public_key() -> Result<()> { + let temp_dir = TempDir::new("patch_manager")?; + // Create a PatchManager without a public key. + let mut manager = PatchManager::manager_for_test(&temp_dir); + + manager.add_signed_patch_for_test( + &temp_dir, + 1, + INFLATED_PATCH_HASH, + Some("not a valid signature"), + )?; + + assert!(manager.next_boot_patch().is_some()); + let patch = manager.next_boot_patch().unwrap(); + assert_eq!(patch.number, 1); + + Ok(()) + } } #[cfg(test)] @@ -819,7 +945,7 @@ mod fall_back_tests { #[test] fn does_nothing_if_no_patch_exists() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); assert!(manager.patches_state.last_booted_patch.is_none()); assert!(manager.patches_state.next_boot_patch.is_none()); @@ -835,7 +961,7 @@ mod fall_back_tests { #[test] fn sets_next_patch_to_latest_patch_if_no_next_patch_exists() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); assert!(manager.patches_state.next_boot_patch.is_none()); @@ -858,7 +984,7 @@ mod fall_back_tests { #[test] fn sets_next_patch_to_latest_patch_if_both_are_present() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); // Download and successfully boot from patch 1 manager.add_patch_for_test(&temp_dir, 1)?; @@ -879,7 +1005,7 @@ mod fall_back_tests { #[test] fn clears_next_and_last_patches_if_both_fail_validation() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); // Download and successfully boot from patch 1, and then corrupt it on disk. manager.add_patch_for_test(&temp_dir, 1)?; @@ -903,7 +1029,7 @@ mod fall_back_tests { #[test] fn does_not_clear_next_patch_if_changed_since_boot_start() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); // Simulate a situation where we download both patches 1 and 2. manager.add_patch_for_test(&temp_dir, 1)?; @@ -927,7 +1053,7 @@ mod fall_back_tests { #[test] fn succeeds_if_deleting_artifacts_fails() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); // Download and successfully boot from patch 1, and then corrupt it on disk. manager.add_patch_for_test(&temp_dir, 1)?; @@ -961,7 +1087,7 @@ mod record_boot_success_for_patch_tests { #[test] fn errs_if_no_next_boot_patch() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); // This should fail because no patches have been added. assert!(manager.record_boot_success().is_err()); @@ -974,7 +1100,7 @@ mod record_boot_success_for_patch_tests { let patch_number = 1; let patch_file_contents = "patch contents"; let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); let file_path = &temp_dir.path().join("patch1.vmcode"); std::fs::write(file_path, patch_file_contents)?; assert!(manager @@ -990,7 +1116,7 @@ mod record_boot_success_for_patch_tests { let patch_number = 1; let patch_file_contents = "patch contents"; let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); let file_path = &temp_dir.path().join("patch1.vmcode"); std::fs::write(file_path, patch_file_contents)?; assert!(manager @@ -1008,7 +1134,7 @@ mod record_boot_success_for_patch_tests { let patch_number = 1; let patch_file_contents = "patch contents"; let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); let file_path = &temp_dir.path().join("patch1.vmcode"); std::fs::write(file_path, patch_file_contents)?; @@ -1044,7 +1170,7 @@ mod record_boot_success_for_patch_tests { #[test] fn deletes_other_patch_artifacts() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); // Download patches 1, 2, and 3 before we start booting from patch 2. manager.add_patch_for_test(&temp_dir, 1)?; @@ -1076,7 +1202,7 @@ mod record_boot_success_for_patch_tests { #[test] fn deletes_unrecognized_directories_in_patches_dir() -> Result<()> { let temp_dir = TempDir::new("patch_manager")?; - let mut manager = PatchManager::with_root_dir(temp_dir.path().to_owned()); + let mut manager = PatchManager::manager_for_test(&temp_dir); // Add a junk directory to the patches directory. let junk_dir = manager.patches_dir().join("junk"); diff --git a/library/src/cache/signing.rs b/library/src/cache/signing.rs new file mode 100644 index 0000000..706ccce --- /dev/null +++ b/library/src/cache/signing.rs @@ -0,0 +1,134 @@ +use anyhow::{bail, Context, Result}; +use base64::Engine; +use std::path::Path; +// https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests +#[cfg(test)] +use std::{println as info, println as debug}; // Workaround to use println! for logs. + +/// Reads the file at `path` and returns the SHA-256 hash of its contents as a String. +pub fn hash_file>(path: P) -> Result { + use sha2::{Digest, Sha256}; // `Digest` is needed for `Sha256::new()`; + + let mut file = std::fs::File::open(path)?; + let mut hasher = Sha256::new(); + std::io::copy(&mut file, &mut hasher)?; + let hash = hasher.finalize(); + Ok(hex::encode(hash)) +} + +/// `public_key` is a DER base64-encoded RSA public key. +/// +/// Given a public_key.pem file, this can be generated with the following command: +/// openssl rsa -pubin \ +/// -in public_key.pem \ +/// -inform PEM \ +/// -RSAPublicKey_out \ +/// -outform DER \ +/// -out public_key.der +/// +/// See https://docs.rs/ring/latest/ring/signature/index.html#signing-and-verifying-with-rsa-pkcs1-15-padding +/// for more information. +pub fn check_signature(message: &str, signature: &str, public_key: &str) -> Result<()> { + debug!("Message is {}", message); + debug!("Public key is {:?}", public_key); + debug!("Signature is {}", signature); + + let public_key_bytes = base64::prelude::BASE64_STANDARD + .decode(public_key) + .with_context(|| format!("Failed to decode public_key: {}", public_key))?; + let public_key = ring::signature::UnparsedPublicKey::new( + &ring::signature::RSA_PKCS1_2048_8192_SHA256, + public_key_bytes, + ); + let decoded_sig = base64::prelude::BASE64_STANDARD + .decode(signature) + .map_err(|e| anyhow::Error::msg(format!("Failed to decode signature: {:?}", e)))?; + + info!("Verifying patch signature..."); + match public_key.verify(message.as_bytes(), &decoded_sig) { + Ok(_) => { + info!("Patch signature is valid"); + Ok(()) + } + Err(_) => { + // The error provided by `verify` is (by design) not helpful, so we ignore it. + // See https://docs.rs/ring/latest/ring/error/struct.Unspecified.html + bail!("Patch signature is invalid") + } + } +} + +#[cfg(test)] +mod tests { + // The constant values below were generated by taking an arbitrary hash (`MESSAGE`) and + // using openssl to sign it with a private key. + + // The base64-encoded public half of the key pair used to sign `MESSAGE`. + const PUBLIC_KEY: &str = "MIIBCgKCAQEA2wdpEGbuvlPsb9i0qYrfMefJnEw1BHTi8SYZTKrXOvJWmEpPE1hWfbkvYzXu5a96gV1yocF3DMwn04VmRlKhC4AhsD0NL0UNhYhotbKG91Kwi1vAXpHhCdz5gQEBw0K1uB4Jz+zK6WK+31PryYpwLwbyXNqXoY8IAAUQ4STsHYV5w+BMSi8pepWMRd7DR9RHcbNOZlJvdBQ5NxvB4JN4dRMq8cC73ez1P9d7Dfwv3TWY+he9EmuXLT2UivZSlHIrGBa7MFfqyUe2ro0F7Te/B0si12itBbWIqycvqcXjeOPNn6WEpqN7IWjb9LUh162JyYaz5Lb/VeeJX8LKtElccwIDAQAB"; + + // The message that was signed. + const MESSAGE: &str = "404e5caa5b906f6d03c97657e8c4d604d759f9cfba1a8bba9d5b49a5ebc174f9"; + + // The base64-encoded signature of `MESSAGE` using the private key corresponding to `PUBLIC_KEY`. + const SIGNATURE: &str = "2ixSo5LpaWUSLg2GJEV+D+uyLeLjp0c3vNXnl0yb1iJjAdpn10BFlbcwCcjaJW9PNky2HU2hKOBe62PkFHOU8DDYOfxf2LGg/ToLGPHin85WrwFAceAUYDs7JpQr43dRTbrXcT8k5tuCQOTwXecGwuWcOFFvh0GbXFnyAmi7fLfN9CtTsG2GIOle/LyYLwoviTrXn/fZTZEYrqxD/wZ4QzoWOWLWNvrPbILhqWELkBLhdZeK0+nC2CIxFRYd3bUeOi1AGtPyHKBfdwuf4VO3+HbwJVaAEiD7HU2Bj+Zp1xeSdbznmYgBV86oizrLFd23D+lBfTlmDGgdfNE9J4Z2/g=="; + + use std::io::Write; + + use anyhow::Result; + use tempdir::TempDir; + + #[test] + fn errs_if_file_does_not_exist() { + let path = "/tmp/does_not_exist"; + let result = super::hash_file(path); + assert!(result.is_err()); + } + + #[test] + fn hashes_file_contents() -> Result<()> { + // Write "hello, world!" to a file. + let temp_dir = TempDir::new("signing")?; + let file_path = temp_dir.path().join("test.txt"); + let mut file = std::fs::File::create(&file_path)?; + file.write_all("hello, world!".as_bytes())?; + + // Verify that the hash is correct. + let hashed = super::hash_file(file_path)?; + assert_eq!( + &hashed, + "68e656b251e67e8358bef8483ab0d51c6619f3e7a1a9f0e75838d41ff368f728" + ); + Ok(()) + } + + #[test] + fn errs_if_public_key_cannot_be_decoded() { + let result = super::check_signature(MESSAGE, SIGNATURE, "bad_public_key"); + assert!(result.is_err()); + let error = result.unwrap_err().to_string(); + assert_eq!(error, "Failed to decode public_key: bad_public_key"); + } + + #[test] + fn errs_if_signature_cannot_be_decoded() { + let result = super::check_signature(MESSAGE, "signature", PUBLIC_KEY); + assert!(result.is_err()); + let error = result.unwrap_err().to_string(); + assert!(error.starts_with("Failed to decode signature")); + } + + #[test] + fn errs_if_signature_is_not_valid() { + // Pass PUBLIC_KEY as the signature to ensure that the signature is invalid. + let result = super::check_signature(MESSAGE, PUBLIC_KEY, PUBLIC_KEY); + assert!(result.is_err()); + let error = result.unwrap_err().to_string(); + assert!(error.starts_with("Patch signature is invalid")); + } + + #[test] + fn is_ok_if_signature_is_valid() { + let result = super::check_signature(MESSAGE, SIGNATURE, PUBLIC_KEY); + assert!(result.is_ok()); + } +} diff --git a/library/src/cache/updater_state.rs b/library/src/cache/updater_state.rs index e9bd9b2..6d92018 100644 --- a/library/src/cache/updater_state.rs +++ b/library/src/cache/updater_state.rs @@ -70,10 +70,10 @@ fn is_file_not_found(error: &anyhow::Error) -> bool { /// Lifecycle methods for the updater state. impl UpdaterState { /// Creates a new `UpdaterState`. - fn new(cache_dir: PathBuf, release_version: String) -> Self { + fn new(cache_dir: PathBuf, release_version: String, patch_public_key: Option<&str>) -> Self { Self { cache_dir: cache_dir.clone(), - patch_manager: Box::new(PatchManager::with_root_dir(cache_dir.clone())), + patch_manager: Box::new(PatchManager::new(cache_dir.clone(), patch_public_key)), serialized_state: SerializedState { release_version, queued_events: Vec::new(), @@ -82,27 +82,39 @@ impl UpdaterState { } /// Loads UpdaterState from disk - fn load(cache_dir: &Path) -> anyhow::Result { + fn load(cache_dir: &Path, patch_public_key: Option<&str>) -> anyhow::Result { let path = cache_dir.join(STATE_FILE_NAME); let serialized_state = disk_io::read(&path)?; Ok(UpdaterState { cache_dir: cache_dir.to_path_buf(), - patch_manager: Box::new(PatchManager::with_root_dir(cache_dir.to_path_buf())), + patch_manager: Box::new(PatchManager::new(cache_dir.to_path_buf(), patch_public_key)), serialized_state, }) } /// Initializes a new UpdaterState and saves it to disk. - fn create_new_and_save(storage_dir: &Path, release_version: &str) -> Self { - let state = Self::new(storage_dir.to_owned(), release_version.to_owned()); + fn create_new_and_save( + storage_dir: &Path, + release_version: &str, + patch_public_key: Option<&str>, + ) -> Self { + let state = Self::new( + storage_dir.to_owned(), + release_version.to_owned(), + patch_public_key, + ); if let Err(e) = state.save() { warn!("Error saving state {:?}, ignoring.", e); } state } - pub fn load_or_new_on_error(storage_dir: &Path, release_version: &str) -> Self { - let load_result = Self::load(storage_dir); + pub fn load_or_new_on_error( + storage_dir: &Path, + release_version: &str, + patch_public_key: Option<&str>, + ) -> Self { + let load_result = Self::load(storage_dir, patch_public_key); match load_result { Ok(mut loaded) => { if loaded.serialized_state.release_version != release_version { @@ -111,7 +123,11 @@ impl UpdaterState { loaded.serialized_state.release_version, release_version ); let _ = loaded.patch_manager.reset(); - return Self::create_new_and_save(storage_dir, release_version); + return Self::create_new_and_save( + storage_dir, + release_version, + patch_public_key, + ); } loaded } @@ -119,7 +135,7 @@ impl UpdaterState { if !is_file_not_found(&e) { info!("No existing state file found: {:#}, creating new state.", e); } - Self::create_new_and_save(storage_dir, release_version) + Self::create_new_and_save(storage_dir, release_version, patch_public_key) } } } @@ -252,7 +268,7 @@ mod tests { #[test] fn release_version_changed_resets_patches() { let tmp_dir = TempDir::new("example").unwrap(); - let mut patch_manager = PatchManager::with_root_dir(tmp_dir.path().to_path_buf()); + let mut patch_manager = PatchManager::manager_for_test(&tmp_dir); let file_path = &tmp_dir.path().join("patch1.vmcode"); std::fs::write(file_path, "patch file contents").unwrap(); assert!(patch_manager.add_patch(1, file_path, "hash", None).is_ok()); @@ -261,11 +277,12 @@ mod tests { let release_version = state.serialized_state.release_version.clone(); assert!(state.save().is_ok()); - let mut state = UpdaterState::load_or_new_on_error(&state.cache_dir, &release_version); + let mut state = + UpdaterState::load_or_new_on_error(&state.cache_dir, &release_version, None); assert_eq!(state.next_boot_patch().unwrap().number, 1); let mut next_version_state = - UpdaterState::load_or_new_on_error(&state.cache_dir, "1.0.0+2"); + UpdaterState::load_or_new_on_error(&state.cache_dir, "1.0.0+2", None); assert!(next_version_state.next_boot_patch().is_none()); assert!(next_version_state.latest_seen_patch_number().is_none()); } @@ -286,9 +303,7 @@ mod tests { let original_tmp_dir = TempDir::new("example").unwrap(); let original_state = UpdaterState { cache_dir: original_tmp_dir.path().to_path_buf(), - patch_manager: Box::new(PatchManager::with_root_dir( - original_tmp_dir.path().to_path_buf(), - )), + patch_manager: Box::new(PatchManager::manager_for_test(&original_tmp_dir)), serialized_state: SerializedState { release_version: "1.0.0+1".to_string(), queued_events: Vec::new(), @@ -301,7 +316,7 @@ mod tests { let new_state_path = new_tmp_dir.path().join(STATE_FILE_NAME); std::fs::rename(original_state_path, new_state_path).unwrap(); - let new_state = UpdaterState::load(new_tmp_dir.path()).unwrap(); + let new_state = UpdaterState::load(new_tmp_dir.path(), None).unwrap(); assert_eq!(new_state.cache_dir, new_tmp_dir.path()); } diff --git a/library/src/network.rs b/library/src/network.rs index fab02da..5d9748e 100644 --- a/library/src/network.rs +++ b/library/src/network.rs @@ -148,7 +148,7 @@ pub struct Patch { pub download_url: String, /// The signature of `hash`, if this patch is signed. None otherwise. #[serde(default)] - pub signature: Option, + pub hash_signature: Option, } /// Any edits to this struct should be made carefully and in accordance diff --git a/library/src/updater.rs b/library/src/updater.rs index 4bbb3fc..8623166 100644 --- a/library/src/updater.rs +++ b/library/src/updater.rs @@ -163,8 +163,11 @@ fn check_for_update_internal() -> anyhow::Result { let (request, url, request_fn) = with_config(|config| { // Load UpdaterState from disk // If there is no state, make an empty state. - let state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); // Get the required info to make the request. Ok(( @@ -258,8 +261,11 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result { // We should never try to write this state as some other writer may be // racing with us, we should get a new state inside a lock if we want // to write. - let read_only_state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let read_only_state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); // We discard any events if we have more than 3 queued to make sure // we don't stall the client. @@ -272,8 +278,11 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result { } // We're abusing the config lock as a UpdateState lock for now. let read_only_state = with_config(|_| { - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); // This will clear any events which got queued between the time we // loaded the state now, but that's OK for now. let result = state.clear_events(); @@ -318,10 +327,13 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result { path: output_path, number: patch.number, }; - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); // Move/state update should be "atomic" (it isn't today). - state.install_patch(&patch_info, &patch.hash, patch.signature.as_deref())?; + state.install_patch(&patch_info, &patch.hash, patch.hash_signature.as_deref())?; info!("Patch {} successfully installed.", patch.number); // Should set some state to say the status is "update required" and that // we now have a different "next" version of the app from the current @@ -389,8 +401,11 @@ where /// 3. `report_launch_failure()` pub fn next_boot_patch() -> anyhow::Result> { with_config(|config| { - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); Ok(state.next_boot_patch()) }) } @@ -400,8 +415,11 @@ pub fn next_boot_patch() -> anyhow::Result> { /// `next_boot_patch`. pub fn current_boot_patch() -> anyhow::Result> { with_config(|config| { - let state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); Ok(state.current_boot_patch()) }) } @@ -414,8 +432,11 @@ pub fn report_launch_start() -> anyhow::Result<()> { info!("Reporting launch start."); with_config(|config| { - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); let next_boot_patch = match state.next_boot_patch() { Some(patch) => patch, @@ -432,8 +453,11 @@ pub fn report_launch_failure() -> anyhow::Result<()> { info!("Reporting failed launch."); with_config(|config| { - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); let patch = state .last_attempted_boot_patch() @@ -464,8 +488,11 @@ pub fn report_launch_success() -> anyhow::Result<()> { with_config(|config| { // We can tell the UpdaterState that we have successfully booted from the "next" patch // and make that the "current" patch. - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); let last_attempted_boot_patch = match state.last_attempted_boot_patch() { Some(patch) => patch, @@ -582,8 +609,11 @@ mod tests { fs::create_dir_all(&download_dir).unwrap(); fs::write(&artifact_path, "hello").unwrap(); - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); state .install_patch( &PatchInfo { @@ -703,8 +733,11 @@ mod tests { fs::create_dir_all(&download_dir).unwrap(); fs::write(&artifact_path, "hello").unwrap(); - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); state .install_patch( &PatchInfo { @@ -725,8 +758,11 @@ mod tests { super::report_launch_success().unwrap(); with_config(|config| { - let state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); assert_eq!(state.current_boot_patch().unwrap().number, patch_number); Ok(()) }) @@ -748,8 +784,11 @@ mod tests { fs::create_dir_all(&download_dir).unwrap(); fs::write(&artifact_path, "hello").unwrap(); - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); state .install_patch( &PatchInfo { @@ -770,8 +809,11 @@ mod tests { super::report_launch_failure().unwrap(); with_config(|config| { - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); // It's now bad. assert!(state.next_boot_patch().is_none()); // And we've queued an event. @@ -813,8 +855,11 @@ mod tests { init_for_testing(&tmp_dir, Some(&server.url())); with_config(|config| { - let mut state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let mut state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); let fail_event = PatchEvent { app_id: config.app_id.clone(), arch: current_arch().to_string(), @@ -838,8 +883,11 @@ mod tests { event_mock.expect(3); with_config(|config| { - let state = - UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version); + let state = UpdaterState::load_or_new_on_error( + &config.storage_dir, + &config.release_version, + config.patch_public_key.as_deref(), + ); // All 5 events should be cleared, even though only 3 were sent. assert_eq!(state.copy_events(10).len(), 0); Ok(())