diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a7f3d39..cb048af 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,17 +4,16 @@ on: push: paths: # Run if workflow changes - - '.github/workflows/ci.yml' + - ".github/workflows/ci.yml" # Run on changed dependencies - - '**/Cargo.toml' - - '**/Cargo.lock' - - '**/rust-toolchain.toml' + - "**/Cargo.toml" + - "**/Cargo.lock" + - "**/rust-toolchain.toml" # Run on changed source files - - 'src/**' - - 'src/**' - branches: main + - "src/**" + branches: [main] pull_request: - branches: main + branches: [main] # Sometimes the rules above don't match even though they should. # This allows us to run the workflow manually anyways. workflow_dispatch: @@ -45,15 +44,12 @@ jobs: name: Rust format runs-on: ubuntu-latest - strategy: - fail-fast: true - steps: - uses: actions/checkout@v3 - name: Install toolchain uses: actions-rust-lang/setup-rust-toolchain@v1 - with: + with: components: rustfmt - name: cargo format @@ -75,7 +71,7 @@ jobs: - name: Install toolchain uses: actions-rust-lang/setup-rust-toolchain@v1 - with: + with: components: clippy toolchain: ${{matrix.toolchain}} @@ -87,9 +83,6 @@ jobs: runs-on: ubuntu-latest needs: ["rust_check", "rust_lint"] - strategy: - fail-fast: true - steps: - uses: actions/checkout@v3 @@ -97,4 +90,4 @@ jobs: uses: actions-rust-lang/setup-rust-toolchain@v1 - name: cargo test - run: cargo test --all-features \ No newline at end of file + run: cargo test --all-features diff --git a/.vscode/settings.json b/.vscode/settings.json deleted file mode 100644 index dbb4894..0000000 --- a/.vscode/settings.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "rust-analyzer.cargo.features": ["serde"] -} \ No newline at end of file diff --git a/.woodpecker.yml b/.woodpecker.yml deleted file mode 100644 index dafbeb4..0000000 --- a/.woodpecker.yml +++ /dev/null @@ -1,12 +0,0 @@ -pipeline: - test: - image: rust:alpine - pull: true - commands: - - apk add musl-dev - - RUST_BACKTRACE=1 cargo test -- --nocapture - - RUST_BACKTRACE=1 cargo test -F serde -- --nocapture - - rustup target add i686-unknown-freebsd aarch64-pc-windows-msvc - - cargo check --target i686-unknown-freebsd - - cargo check --target aarch64-pc-windows-msvc - - cargo fmt --check diff --git a/Cargo.lock b/Cargo.lock index 3913792..f661667 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,243 +1,127 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 - -[[package]] -name = "cfg-if" -version = "0.1.10" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4785bdd1c96b2a846b2bd7cc02e86b6b3dbf14e7e53446c4f54c92a361040822" - -[[package]] -name = "half" -version = "1.8.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eabb4a44450da02c90444cf74558da904edde8fb4e9035a9a6a4e15445af0bd7" +version = 4 [[package]] name = "itoa" -version = "1.0.9" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "af150ab688ff2122fcef229be89cb50dd66af9e01a4ff320cc137eecc9bacc38" - -[[package]] -name = "lazy_static" -version = "1.4.0" +version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" +checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682" [[package]] name = "libc" -version = "0.2.148" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9cdc71e17332e86d2e1d38c1f99edcb6288ee11b815fb1a4b049eaa2114d369b" - -[[package]] -name = "pre" -version = "0.2.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f87e7dc12cd3c36e90697e072bd70d886def4580fb268292022f89014dc9ea4b" -dependencies = [ - "cfg-if", - "pre-proc-macro", - "rustc_version", -] - -[[package]] -name = "pre-proc-macro" -version = "0.2.1" +version = "0.2.185" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a40339af35253b6b41a9e74f4c747e0948a9cb9cc9ea01d117d8440524560de3" -dependencies = [ - "cfg-if", - "lazy_static", - "proc-macro-crate", - "proc-macro-error", - "proc-macro2", - "quote", - "rustc_version", - "syn 1.0.109", -] - -[[package]] -name = "proc-macro-crate" -version = "0.1.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1d6ea3c4595b96363c13943497db34af4460fb474a95c43f4446ad341b8c9785" -dependencies = [ - "toml", -] +checksum = "52ff2c0fe9bc6cb6b14a0592c2ff4fa9ceb83eea9db979b0487cd054946a2b8f" [[package]] -name = "proc-macro-error" -version = "1.0.4" +name = "memchr" +version = "2.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c" -dependencies = [ - "proc-macro-error-attr", - "proc-macro2", - "quote", - "syn 1.0.109", - "version_check", -] - -[[package]] -name = "proc-macro-error-attr" -version = "1.0.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869" -dependencies = [ - "proc-macro2", - "quote", - "version_check", -] +checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" [[package]] name = "proc-macro2" -version = "1.0.66" +version = "1.0.106" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "18fb31db3f9bddb2ea821cde30a9f70117e3f119938b5ee630b7403aa6e2ead9" +checksum = "8fd00f0bb2e90d81d1044c2b32617f68fcb9fa3bb7640c23e9c748e53fb30934" dependencies = [ "unicode-ident", ] [[package]] name = "quote" -version = "1.0.33" +version = "1.0.45" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5267fca4496028628a95160fc423a33e8b2e6af8a5302579e322e4b520293cae" +checksum = "41f2619966050689382d2b44f664f4bc593e129785a36d6ee376ddf37259b924" dependencies = [ "proc-macro2", ] -[[package]] -name = "rustc_version" -version = "0.2.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "138e3e0acb6c9fb258b19b67cb8abd63c00679d2851805ea151465464fe9030a" -dependencies = [ - "semver", -] - -[[package]] -name = "ryu" -version = "1.0.15" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1ad4cc8da4ef723ed60bced201181d83791ad433213d8c24efffda1eec85d741" - [[package]] name = "secure-string" version = "0.3.0" dependencies = [ "libc", - "pre", "serde", - "serde_cbor", "serde_json", + "subtle", "zeroize", ] -[[package]] -name = "semver" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1d7eb9ef2c18661902cc47e535f9bc51b78acd254da71d375c2f6720d9a40403" -dependencies = [ - "semver-parser", -] - -[[package]] -name = "semver-parser" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "388a1df253eca08550bef6c72392cfe7c30914bf41df5269b68cbd6ff8f570a3" - [[package]] name = "serde" -version = "1.0.188" +version = "1.0.228" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cf9e0fcba69a370eed61bcf2b728575f726b50b55cba78064753d708ddc7549e" +checksum = "9a8e94ea7f378bd32cbbd37198a4a91436180c5bb472411e48b5ec2e2124ae9e" dependencies = [ - "serde_derive", + "serde_core", ] [[package]] -name = "serde_cbor" -version = "0.11.2" +name = "serde_core" +version = "1.0.228" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2bef2ebfde456fb76bbcf9f59315333decc4fda0b2b44b420243c11e0f5ec1f5" +checksum = "41d385c7d4ca58e59fc732af25c3983b67ac852c1a25000afe1175de458b67ad" dependencies = [ - "half", - "serde", + "serde_derive", ] [[package]] name = "serde_derive" -version = "1.0.188" +version = "1.0.228" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4eca7ac642d82aa35b60049a6eccb4be6be75e599bd2e9adb5f875a737654af2" +checksum = "d540f220d3187173da220f885ab66608367b6574e925011a9353e4badda91d79" dependencies = [ "proc-macro2", "quote", - "syn 2.0.29", + "syn", ] [[package]] name = "serde_json" -version = "1.0.105" +version = "1.0.149" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "693151e1ac27563d6dbcec9dee9fbd5da8539b20fa14ad3752b2e6d363ace360" +checksum = "83fc039473c5595ace860d8c4fafa220ff474b3fc6bfdb4293327f1a37e94d86" dependencies = [ "itoa", - "ryu", + "memchr", "serde", + "serde_core", + "zmij", ] [[package]] -name = "syn" -version = "1.0.109" +name = "subtle" +version = "2.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "72b64191b275b66ffe2469e8af2c1cfe3bafa67b529ead792a6d0160888b4237" -dependencies = [ - "proc-macro2", - "quote", - "unicode-ident", -] +checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" [[package]] name = "syn" -version = "2.0.29" +version = "2.0.117" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c324c494eba9d92503e6f1ef2e6df781e78f6a7705a0202d9801b198807d518a" +checksum = "e665b8803e7b1d2a727f4023456bbbbe74da67099c585258af0ad9c5013b9b99" dependencies = [ "proc-macro2", "quote", "unicode-ident", ] -[[package]] -name = "toml" -version = "0.5.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f4f7f0dd8d50a853a531c426359045b1998f04219d88799810762cd4ad314234" -dependencies = [ - "serde", -] - [[package]] name = "unicode-ident" -version = "1.0.11" +version = "1.0.24" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "301abaae475aa91687eb82514b328ab47a211a533026cb25fc3e519b86adfc3c" +checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" [[package]] -name = "version_check" -version = "0.9.4" +name = "zeroize" +version = "1.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f" +checksum = "b97154e67e32c85465826e8bcc1c59429aaaf107c1e4a9e53c8d8ccd5eff88d0" [[package]] -name = "zeroize" -version = "1.6.0" +name = "zmij" +version = "1.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2a0956f1ba7c7909bfb66c2e9e4124ab6f6482560f6628b5aaeba39207c9aad9" +checksum = "b8848ee67ecc8aedbaf3e4122217aff892639231befc6a1b58d29fff4c2cabaa" diff --git a/Cargo.toml b/Cargo.toml index ff07063..a8bc165 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,11 +11,10 @@ documentation = "https://docs.rs/secure-string/" edition = "2021" [dependencies] -libc = "0.2.148" -zeroize = { version = "1.6.0", features = ["std"] } -serde = { version = "1.0.188", optional = true } +libc = "0.2" +subtle = "2" +zeroize = { version = "1", features = ["std"] } +serde = { version = "1", optional = true } [dev-dependencies] -pre = "0.2.1" -serde_cbor = "0.11" -serde_json = "1.0.105" +serde_json = "1" diff --git a/src/lib.rs b/src/lib.rs index 3d1a3bd..4574fa4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3,10 +3,14 @@ mod secure_types; mod secure_utils; +#[cfg(test)] +mod test_utils; + #[cfg(feature = "serde")] mod serde; pub use secure_types::{array::SecureArray, boxed::SecureBox, string::SecureString, vec::SecureBytes, vec::SecureVec}; +pub use subtle::ConstantTimeEq; #[doc = include_str!("../README.md")] #[cfg(doctest)] diff --git a/src/secure_types/array.rs b/src/secure_types/array.rs index 99e18a2..8d76237 100644 --- a/src/secure_types/array.rs +++ b/src/secure_types/array.rs @@ -4,6 +4,7 @@ use std::{ str::FromStr, }; +use subtle::ConstantTimeEq; use zeroize::Zeroize; use crate::secure_utils::memlock; @@ -16,8 +17,6 @@ use crate::secure_utils::memlock; /// - Automatic `mlock` to protect against leaking into swap (any unix) /// - Automatic `madvise(MADV_NOCORE/MADV_DONTDUMP)` to protect against leaking into core dumps (FreeBSD, DragonflyBSD, Linux) /// -/// Comparisons using the `PartialEq` implementation are undefined behavior (and most likely wrong) if `T` has any padding bytes. -#[derive(Eq, PartialEq, PartialOrd, Ord, Hash)] pub struct SecureArray where T: Copy + Zeroize, @@ -56,6 +55,20 @@ impl Clone for SecureArray { } } +impl ConstantTimeEq for SecureArray { + fn ct_eq(&self, other: &Self) -> subtle::Choice { + self.content.as_slice().ct_eq(other.content.as_slice()) + } +} + +impl PartialEq for SecureArray { + fn eq(&self, other: &Self) -> bool { + self.ct_eq(other).into() + } +} + +impl Eq for SecureArray {} + // Creation impl From<[T; LENGTH]> for SecureArray where @@ -163,7 +176,6 @@ mod tests { } #[test] - #[cfg_attr(feature = "pre", pre::pre)] fn test_zero_out() { let mut my_sec: SecureArray<_, 5> = SecureArray::from_str("hello").unwrap(); my_sec.zero_out(); @@ -190,16 +202,15 @@ mod tests { } #[test] - #[cfg_attr(feature = "pre", pre::pre)] - fn test_comparison_zero_out_mb() { - let mbstring1 = SecureArray::from(['H', 'a', 'l', 'l', 'o', ' ', '🦄', '!']); - let mbstring2 = SecureArray::from(['H', 'a', 'l', 'l', 'o', ' ', '🦄', '!']); - let mbstring3 = SecureArray::from(['!', '🦄', ' ', 'o', 'l', 'l', 'a', 'H']); - assert!(mbstring1 == mbstring2); - assert!(mbstring1 != mbstring3); - - let mut mbstring = mbstring1.clone(); - mbstring.zero_out(); - assert_eq!(mbstring.unsecure(), &['\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0']); + fn test_comparison_zero_out_multibyte() { + let data1 = SecureArray::from([0x0048u32, 0x0061, 0x006C, 0x006C, 0x006F, 0x0020, 0x1F984, 0x0021]); + let data2 = SecureArray::from([0x0048u32, 0x0061, 0x006C, 0x006C, 0x006F, 0x0020, 0x1F984, 0x0021]); + let data3 = SecureArray::from([0x0021u32, 0x1F984, 0x0020, 0x006F, 0x006C, 0x006C, 0x0061, 0x0048]); + assert!(data1 == data2); + assert!(data1 != data3); + + let mut zeroed = data1.clone(); + zeroed.zero_out(); + assert_eq!(zeroed.unsecure(), &[0u32; 8]); } } diff --git a/src/secure_types/boxed.rs b/src/secure_types/boxed.rs index 7177c29..cdd3f3f 100644 --- a/src/secure_types/boxed.rs +++ b/src/secure_types/boxed.rs @@ -4,6 +4,7 @@ use std::{ mem::MaybeUninit, }; +use subtle::ConstantTimeEq; use zeroize::Zeroize; use crate::secure_utils::memlock; @@ -16,8 +17,6 @@ use crate::secure_utils::memlock; /// - Automatic `mlock` to protect against leaking into swap (any unix) /// - Automatic `madvise(MADV_NOCORE/MADV_DONTDUMP)` to protect against leaking into core dumps (FreeBSD, DragonflyBSD, Linux) /// -/// Comparisons using the `PartialEq` implementation are undefined behavior (and most likely wrong) if `T` has any padding bytes. -#[derive(Eq, PartialEq, PartialOrd, Ord, Hash)] pub struct SecureBox where T: Copy, @@ -36,23 +35,45 @@ where SecureBox { content: Some(cont) } } + fn content_ref(&self) -> &T { + self.content.as_deref().expect("SecureBox content accessed after drop") + } + + fn content_mut(&mut self) -> &mut T { + self.content.as_deref_mut().expect("SecureBox content accessed after drop") + } + /// Borrow the contents of the string. pub fn unsecure(&self) -> &T { - self.content.as_ref().unwrap() + self.content_ref() } /// Mutably borrow the contents of the string. pub fn unsecure_mut(&mut self) -> &mut T { - self.content.as_mut().unwrap() + self.content_mut() } } impl Clone for SecureBox { fn clone(&self) -> Self { - Self::new(self.content.clone().unwrap()) + Self::new(Box::new(*self.content_ref())) + } +} + +impl ConstantTimeEq for SecureBox { + fn ct_eq(&self, other: &Self) -> subtle::Choice { + self.content_ref().ct_eq(other.content_ref()) } } +impl PartialEq for SecureBox { + fn eq(&self, other: &Self) -> bool { + self.ct_eq(other).into() + } +} + +impl Eq for SecureBox {} + // Delegate indexing impl std::ops::Index for SecureBox where @@ -61,7 +82,7 @@ where type Output = >::Output; fn index(&self, index: U) -> &Self::Output { - std::ops::Index::index(self.content.as_ref().unwrap().as_ref(), index) + std::ops::Index::index(self.content_ref(), index) } } @@ -71,7 +92,7 @@ where T: Copy, { fn borrow(&self) -> &T { - self.content.as_ref().unwrap() + self.content_ref() } } impl BorrowMut for SecureBox @@ -79,7 +100,7 @@ where T: Copy, { fn borrow_mut(&mut self) -> &mut T { - self.content.as_mut().unwrap() + self.content_mut() } } @@ -88,17 +109,19 @@ impl Drop for SecureBox where T: Copy, { - #[cfg_attr(feature = "pre", pre::pre)] fn drop(&mut self) { // Make sure that the box does not need to be dropped after this function, because it may // see an invalid type, if `T` does not support an all-zero byte-pattern // Instead we manually destruct the box and only handle the potentially invalid values // behind the pointer - let ptr = Box::into_raw(self.content.take().unwrap()); + let ptr = Box::into_raw(self.content.take().expect("SecureBox dropped twice")); // There is no need to worry about dropping the contents, because `T: Copy` and `Copy` // types cannot implement `Drop` + // SAFETY: `ptr` was just obtained from `Box::into_raw` so it is valid, aligned, and + // points to `size_of::()` allocated bytes. Writing `MaybeUninit` zeros is always + // valid regardless of `T`'s invariants. unsafe { std::slice::from_raw_parts_mut::>(ptr as *mut MaybeUninit, std::mem::size_of::()).zeroize(); } @@ -107,10 +130,9 @@ where // Deallocate only non-zero-sized types, because otherwise it's UB if std::mem::size_of::() != 0 { - // Safety: - // This way to manually deallocate is advertised in the documentation of `Box::into_raw`. - // The box was allocated with the global allocator and a layout of `T` and is thus - // deallocated using the same allocator and layout here. + // SAFETY: This way to manually deallocate is advertised in the documentation of + // `Box::into_raw`. The box was allocated with the global allocator and a layout of + // `T` and is thus deallocated using the same allocator and layout here. unsafe { std::alloc::dealloc(ptr as *mut u8, std::alloc::Layout::new::()) }; } } @@ -142,36 +164,28 @@ mod tests { use zeroize::Zeroize; use super::SecureBox; + use crate::test_utils::{Packed, Padded, PRIVATE_KEY_1, PRIVATE_KEY_2}; - const PRIVATE_KEY_1: [u8; 32] = [ - 0xb0, 0x3b, 0x34, 0xc3, 0x3a, 0x1c, 0x44, 0xf2, 0x25, 0xb6, 0x62, 0xd2, 0xbf, 0x48, 0x59, 0xb8, 0x13, 0x54, 0x11, 0xfa, - 0x7b, 0x03, 0x86, 0xd4, 0x5f, 0xb7, 0x5d, 0xc5, 0xb9, 0x1b, 0x44, 0x66, - ]; - - const PRIVATE_KEY_2: [u8; 32] = [ - 0xc8, 0x06, 0x43, 0x9d, 0xc9, 0xd2, 0xc4, 0x76, 0xff, 0xed, 0x8f, 0x25, 0x80, 0xc0, 0x88, 0x8d, 0x58, 0xab, 0x40, 0x6b, - 0xf7, 0xae, 0x36, 0x98, 0x87, 0x90, 0x21, 0xb9, 0x6b, 0xb4, 0xbf, 0x59, - ]; - - /// Overwrite the contents with zeros. This is automatically done in the destructor. + /// Overwrite the contents with zeros. /// /// # Safety /// An all-zero byte-pattern must be a valid value of `T` in order for this function call to not be /// undefined behavior. - #[cfg_attr(feature = "pre", pre::pre("an all-zero byte-pattern is a valid value of `T`"))] - pub(crate) unsafe fn zero_out_secure_box(secure_box: &mut SecureBox) + unsafe fn zero_out_secure_box(secure_box: &mut SecureBox) where T: Copy, { + // SAFETY: The pointer is derived from a live `Box` via mutable reference, so it is + // valid and aligned for `size_of::()` bytes. The caller guarantees that an all-zero + // byte-pattern is a valid value of `T`. std::slice::from_raw_parts_mut::>( - &mut **secure_box.content.as_mut().unwrap() as *mut T as *mut MaybeUninit, + secure_box.content_mut() as *mut T as *mut MaybeUninit, std::mem::size_of::(), ) .zeroize(); } #[test] - #[cfg_attr(feature = "pre", pre::pre)] fn test_secure_box() { let key_1 = SecureBox::new(Box::new(PRIVATE_KEY_1)); let key_2 = SecureBox::new(Box::new(PRIVATE_KEY_2)); @@ -182,16 +196,39 @@ mod tests { assert!(key_1 == key_3); let mut final_key = key_1.clone(); - #[cfg_attr( - feature = "pre", - assure( - "an all-zero byte-pattern is a valid value of `T`", - reason = "`T` is `i32`, for which an all-zero byte-pattern is valid" - ) - )] + // SAFETY: `T` is `Key([u8; 32])`, for which an all-zero byte-pattern is valid. unsafe { - zero_out_secure_box(&mut final_key) - }; - assert_eq!(final_key.unsecure(), &[0; 32]); + zero_out_secure_box(&mut final_key); + } + assert_eq!(final_key.unsecure().0, [0; 32]); + } + + #[test] + fn test_repr_c_with_padding() { + assert_eq!(std::mem::size_of::(), 4); // 1 + 1 (pad) + 2 + + let sec_a = SecureBox::new(Box::new(Padded { x: 1, y: 2 })); + let sec_b = SecureBox::new(Box::new(Padded { x: 1, y: 2 })); + assert_eq!(sec_a, sec_b); + + let sec_c = SecureBox::new(Box::new(Padded { x: 1, y: 3 })); + assert_ne!(sec_a, sec_c); + + let sec_d = SecureBox::new(Box::new(Padded { x: 2, y: 2 })); + assert_ne!(sec_a, sec_d); + } + + #[test] + fn test_repr_c_packed() { + assert_eq!(std::mem::size_of::(), 3); + + let sec_a = SecureBox::new(Box::new(Packed { x: 42, y: 1000 })); + let sec_b = SecureBox::new(Box::new(Packed { x: 42, y: 1000 })); + let sec_c = SecureBox::new(Box::new(Packed { x: 42, y: 1001 })); + let sec_d = SecureBox::new(Box::new(Packed { x: 43, y: 1000 })); + + assert_eq!(sec_a, sec_b); + assert_ne!(sec_a, sec_c); + assert_ne!(sec_a, sec_d); } } diff --git a/src/secure_types/string.rs b/src/secure_types/string.rs index 81a9e0e..0643230 100644 --- a/src/secure_types/string.rs +++ b/src/secure_types/string.rs @@ -4,65 +4,30 @@ use std::str::FromStr; use crate::{secure_utils::memlock, SecureVec}; /// Wrapper for a vector that stores a valid UTF-8 string -#[derive(Clone, Eq)] +#[derive(Clone)] pub struct SecureString(SecureVec); impl SecureString { /// Borrow the contents of the string. - #[cfg_attr(feature = "pre", pre::pre)] pub fn unsecure(&self) -> &str { - #[cfg_attr( - feature = "pre", - forward(pre), - assure( - "the content of `v` is valid UTF-8", - reason = "it is not possible to create a `SecureString` with invalid UTF-8 content - and it is also not possible to modify the content as non-UTF-8 directly, so - they must still be valid UTF-8 here" - ) - )] - unsafe { - std::str::from_utf8_unchecked(self.0.unsecure()) - } + // SAFETY: SecureString can only be constructed from valid UTF-8 (String or &str), + // and the contents cannot be modified as non-UTF-8, so they remain valid UTF-8. + unsafe { std::str::from_utf8_unchecked(self.0.unsecure()) } } /// Mutably borrow the contents of the string. - #[cfg_attr(feature = "pre", pre::pre)] pub fn unsecure_mut(&mut self) -> &mut str { - #[cfg_attr( - feature = "pre", - forward(pre), - assure( - "the content of `v` is valid UTF-8", - reason = "it is not possible to create a `SecureString` with invalid UTF-8 content - and it is also not possible to modify the content as non-UTF-8 directly, so - they must still be valid UTF-8 here" - ) - )] - unsafe { - std::str::from_utf8_unchecked_mut(self.0.unsecure_mut()) - } + // SAFETY: Same as `unsecure` — contents are always valid UTF-8. + unsafe { std::str::from_utf8_unchecked_mut(self.0.unsecure_mut()) } } /// Turn the string into a regular `String` again. - #[cfg_attr(feature = "pre", pre::pre)] pub fn into_unsecure(mut self) -> String { memlock::munlock(self.0.content.as_mut_ptr(), self.0.content.capacity()); let content = std::mem::take(&mut self.0.content); std::mem::forget(self); - #[cfg_attr( - feature = "pre", - forward(impl pre::std::string::String), - assure( - "the content of `bytes` is valid UTF-8", - reason = "it is not possible to create a `SecureString` with invalid UTF-8 content - and it is also not possible to modify the content as non-UTF-8 directly, so - they must still be valid UTF-8 here" - ) - )] - unsafe { - String::from_utf8_unchecked(content) - } + // SAFETY: Same as `unsecure` — contents are always valid UTF-8. + unsafe { String::from_utf8_unchecked(content) } } /// Overwrite the string with zeros. This is automatically called in the destructor. @@ -75,11 +40,13 @@ impl SecureString { impl PartialEq for SecureString { fn eq(&self, other: &SecureString) -> bool { - // use implementation of SecureVec + // use constant-time implementation of SecureVec self.0 == other.0 } } +impl Eq for SecureString {} + impl fmt::Debug for SecureString { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.write_str("***SECRET***").map_err(|_| fmt::Error) diff --git a/src/secure_types/vec.rs b/src/secure_types/vec.rs index 8598025..d90e6a9 100644 --- a/src/secure_types/vec.rs +++ b/src/secure_types/vec.rs @@ -4,6 +4,7 @@ use std::{ str::FromStr, }; +use subtle::ConstantTimeEq; use zeroize::Zeroize; use crate::secure_utils::memlock; @@ -20,7 +21,6 @@ use crate::secure_utils::memlock; /// /// Be careful with `SecureBytes::from`: if you have a borrowed string, it will be copied. /// Use `SecureBytes::new` if you have a `Vec`. -#[derive(PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct SecureVec where T: Copy + Zeroize, @@ -90,6 +90,20 @@ impl Clone for SecureVec { } } +impl ConstantTimeEq for SecureVec { + fn ct_eq(&self, other: &Self) -> subtle::Choice { + self.content.ct_eq(&other.content) + } +} + +impl PartialEq for SecureVec { + fn eq(&self, other: &Self) -> bool { + self.ct_eq(other).into() + } +} + +impl Eq for SecureVec {} + // Creation impl From for SecureVec where @@ -183,26 +197,14 @@ mod tests { } #[test] - #[cfg_attr(feature = "pre", pre::pre)] fn test_zero_out() { let mut my_sec = SecureBytes::from("hello"); my_sec.zero_out(); // `zero_out` sets the `len` to 0, here we reset it to check that the bytes were zeroed - #[cfg_attr( - feature = "pre", - forward(impl pre::std::vec::Vec), - assure( - new_len <= self.capacity(), - reason = "the call to `zero_out` did not reduce the capacity and the length was `5` before, - so the capacity must be greater or equal to `5`" - ), - assure( - "the elements at `old_len..new_len` are initialized", - reason = "they were initialized to `0` by the call to `zero_out`" - ) - )] + // SAFETY: capacity is still >= 5 (zero_out does not shrink), and all 5 bytes were + // zeroed, so they are valid initialized `u8` values. unsafe { - my_sec.content.set_len(5) + my_sec.content.set_len(5); } assert_eq!(my_sec.unsecure(), b"\x00\x00\x00\x00\x00"); } @@ -240,32 +242,20 @@ mod tests { } #[test] - #[cfg_attr(feature = "pre", pre::pre)] fn test_comparison_zero_out_mb() { let mbstring1 = SecureVec::from(vec!['H', 'a', 'l', 'l', 'o', ' ', '🦄', '!']); let mbstring2 = SecureVec::from(vec!['H', 'a', 'l', 'l', 'o', ' ', '🦄', '!']); let mbstring3 = SecureVec::from(vec!['!', '🦄', ' ', 'o', 'l', 'l', 'a', 'H']); - assert!(mbstring1 == mbstring2); - assert!(mbstring1 != mbstring3); + assert_eq!(mbstring1.unsecure(), mbstring2.unsecure()); + assert_ne!(mbstring1.unsecure(), mbstring3.unsecure()); let mut mbstring = mbstring1.clone(); mbstring.zero_out(); // `zero_out` sets the `len` to 0, here we reset it to check that the bytes were zeroed - #[cfg_attr( - feature = "pre", - forward(impl pre::std::vec::Vec), - assure( - new_len <= self.capacity(), - reason = "the call to `zero_out` did not reduce the capacity and the length was `8` before, - so the capacity must be greater or equal to `8`" - ), - assure( - "the elements at `old_len..new_len` are initialized", - reason = "they were initialized to `0` by the call to `zero_out`" - ) - )] + // SAFETY: capacity is still >= 8 (zero_out does not shrink), and all bytes were + // zeroed, which is a valid bit pattern for `u32`. unsafe { - mbstring.content.set_len(8) + mbstring.content.set_len(8); } assert_eq!(mbstring.unsecure(), &['\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0']); } diff --git a/src/secure_utils.rs b/src/secure_utils.rs index faa8dab..d32fde1 100644 --- a/src/secure_utils.rs +++ b/src/secure_utils.rs @@ -4,6 +4,9 @@ pub mod memlock { pub fn mlock(cont: *mut T, count: usize) { let byte_num = count * std::mem::size_of::(); + // SAFETY: `cont` points to a valid allocation of at least `count * size_of::()` + // bytes (guaranteed by callers passing pointers from live Vec/Array/Box allocations). + // mlock/madvise are safe to call on any valid memory region. unsafe { let ptr = cont as *mut libc::c_void; libc::mlock(ptr, byte_num); @@ -16,6 +19,8 @@ pub mod memlock { pub fn munlock(cont: *mut T, count: usize) { let byte_num = count * std::mem::size_of::(); + // SAFETY: Same as `mlock` — the pointer is to a valid allocation that was previously + // locked. munlock/madvise are safe to call on any valid memory region. unsafe { let ptr = cont as *mut libc::c_void; libc::munlock(ptr, byte_num); diff --git a/src/serde.rs b/src/serde.rs index 15a3355..372688a 100644 --- a/src/serde.rs +++ b/src/serde.rs @@ -97,27 +97,7 @@ impl Serialize for SecureArray { #[cfg(test)] mod tests { - use std::str::FromStr; - - use crate::{SecureArray, SecureBytes, SecureVec}; - - #[test] - fn test_cbor_vec() { - let data = SecureBytes::from("hello"); - let cbor = serde_cbor::to_vec(&data).unwrap(); - assert_eq!(cbor, b"\x45hello"); - let deserialised = serde_cbor::from_slice(&cbor).unwrap(); - assert_eq!(data, deserialised); - } - - #[test] - fn test_cbor_array() { - let data: SecureArray<_, 5> = SecureArray::from_str("hello").unwrap(); - let cbor = serde_cbor::to_vec(&data).unwrap(); - assert_eq!(cbor, b"\x45hello"); - let deserialised = serde_cbor::from_slice(&cbor).unwrap(); - assert_eq!(data, deserialised); - } + use crate::SecureVec; #[test] fn test_serde_json() { diff --git a/src/test_utils.rs b/src/test_utils.rs new file mode 100644 index 0000000..b39f24d --- /dev/null +++ b/src/test_utils.rs @@ -0,0 +1,56 @@ +use subtle::ConstantTimeEq; + +#[derive(Clone, Copy)] +pub struct Key(pub [u8; 32]); + +impl ConstantTimeEq for Key { + fn ct_eq(&self, other: &Self) -> subtle::Choice { + self.0.as_slice().ct_eq(other.0.as_slice()) + } +} + +pub const PRIVATE_KEY_1: Key = Key([ + 0xb0, 0x3b, 0x34, 0xc3, 0x3a, 0x1c, 0x44, 0xf2, 0x25, 0xb6, 0x62, 0xd2, 0xbf, 0x48, 0x59, 0xb8, 0x13, 0x54, 0x11, 0xfa, + 0x7b, 0x03, 0x86, 0xd4, 0x5f, 0xb7, 0x5d, 0xc5, 0xb9, 0x1b, 0x44, 0x66, +]); + +pub const PRIVATE_KEY_2: Key = Key([ + 0xc8, 0x06, 0x43, 0x9d, 0xc9, 0xd2, 0xc4, 0x76, 0xff, 0xed, 0x8f, 0x25, 0x80, 0xc0, 0x88, 0x8d, 0x58, 0xab, 0x40, 0x6b, + 0xf7, 0xae, 0x36, 0x98, 0x87, 0x90, 0x21, 0xb9, 0x6b, 0xb4, 0xbf, 0x59, +]); + +/// A `#[repr(C)]` struct with a padding byte between `x` and `y`. +/// Total size = 4 bytes: [x, padding, y_lo, y_hi]. +/// +/// `ConstantTimeEq` compares fields, not raw bytes, so padding is irrelevant. +#[repr(C)] +#[derive(Clone, Copy)] +pub struct Padded { + pub x: u8, + pub y: u16, +} + +impl ConstantTimeEq for Padded { + fn ct_eq(&self, other: &Self) -> subtle::Choice { + self.x.ct_eq(&other.x) & self.y.ct_eq(&other.y) + } +} + +/// A `#[repr(C, packed)]` struct with no padding. +/// Total size = 3 bytes: [x, y_lo, y_hi]. +/// +/// Fields are copied to locals before comparison to avoid unaligned references. +#[repr(C, packed)] +#[derive(Clone, Copy)] +pub struct Packed { + pub x: u8, + pub y: u16, +} + +impl ConstantTimeEq for Packed { + fn ct_eq(&self, other: &Self) -> subtle::Choice { + let (sx, sy) = (self.x, self.y); + let (ox, oy) = (other.x, other.y); + sx.ct_eq(&ox) & sy.ct_eq(&oy) + } +}