From 10fd02e42e7bbae6574cf2d66e8d273c6ffaba58 Mon Sep 17 00:00:00 2001 From: Noa Date: Wed, 16 Apr 2025 17:01:21 -0500 Subject: [PATCH] Use constant_time_eq instead of our bespoke implementation --- Cargo.lock | 14 ++++++------ Cargo.toml | 1 + common/Cargo.toml | 1 - common/src/cmp.rs | 48 --------------------------------------- common/src/lib.rs | 1 - vm/Cargo.toml | 1 + vm/src/stdlib/operator.rs | 8 +++---- 7 files changed, 12 insertions(+), 62 deletions(-) delete mode 100644 common/src/cmp.rs diff --git a/Cargo.lock b/Cargo.lock index 941b1f97fe..87cfb55109 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -439,6 +439,12 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "constant_time_eq" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3d52eff69cd5e647efe296129160853a42795992097e8af39800e1060caeea9b" + [[package]] name = "core-foundation" version = "0.9.4" @@ -2348,7 +2354,6 @@ dependencies = [ "rustpython-wtf8", "siphasher", "unicode_names2", - "volatile", "widestring", "windows-sys 0.59.0", ] @@ -2556,6 +2561,7 @@ dependencies = [ "caseless", "cfg-if", "chrono", + "constant_time_eq", "crossbeam-utils", "errno", "exitcode", @@ -3279,12 +3285,6 @@ version = "0.9.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" -[[package]] -name = "volatile" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f8e76fae08f03f96e166d2dfda232190638c10e0383841252416f9cfe2ae60e6" - [[package]] name = "walkdir" version = "2.5.0" diff --git a/Cargo.toml b/Cargo.toml index 621c255175..21fb9de990 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -155,6 +155,7 @@ bitflags = "2.4.2" bstr = "1" cfg-if = "1.0" chrono = "0.4.39" +constant_time_eq = "0.4" criterion = { version = "0.5", features = ["html_reports"] } crossbeam-utils = "0.8.21" flame = "0.2.2" diff --git a/common/Cargo.toml b/common/Cargo.toml index 87e1ee118d..0649c2a509 100644 --- a/common/Cargo.toml +++ b/common/Cargo.toml @@ -34,7 +34,6 @@ unicode_names2 = { workspace = true } lock_api = "0.4" radium = "1.1" siphasher = "1" -volatile = "0.3" [target.'cfg(windows)'.dependencies] widestring = { workspace = true } diff --git a/common/src/cmp.rs b/common/src/cmp.rs deleted file mode 100644 index d182340a98..0000000000 --- a/common/src/cmp.rs +++ /dev/null @@ -1,48 +0,0 @@ -use volatile::Volatile; - -/// Compare 2 byte slices in a way that ensures that the timing of the operation can't be used to -/// glean any information about the data. -#[inline(never)] -#[cold] -pub fn timing_safe_cmp(a: &[u8], b: &[u8]) -> bool { - // we use raw pointers here to keep faithful to the C implementation and - // to try to avoid any optimizations rustc might do with slices - let len_a = a.len(); - let a = a.as_ptr(); - let len_b = b.len(); - let b = b.as_ptr(); - /* The volatile type declarations make sure that the compiler has no - * chance to optimize and fold the code in any way that may change - * the timing. - */ - let mut result: u8 = 0; - /* loop count depends on length of b */ - let length: Volatile = Volatile::new(len_b); - let mut left: Volatile<*const u8> = Volatile::new(std::ptr::null()); - let mut right: Volatile<*const u8> = Volatile::new(b); - - /* don't use else here to keep the amount of CPU instructions constant, - * volatile forces re-evaluation - * */ - if len_a == length.read() { - left.write(Volatile::new(a).read()); - result = 0; - } - if len_a != length.read() { - left.write(b); - result = 1; - } - - for _ in 0..length.read() { - let l = left.read(); - left.write(l.wrapping_add(1)); - let r = right.read(); - right.write(r.wrapping_add(1)); - // safety: the 0..length range will always be either: - // * as long as the length of both a and b, if len_a and len_b are equal - // * as long as b, and both `left` and `right` are b - result |= unsafe { l.read_volatile() ^ r.read_volatile() }; - } - - result == 0 -} diff --git a/common/src/lib.rs b/common/src/lib.rs index c75451802a..a62c9517b4 100644 --- a/common/src/lib.rs +++ b/common/src/lib.rs @@ -10,7 +10,6 @@ pub mod atomic; pub mod borrow; pub mod boxvec; pub mod cformat; -pub mod cmp; #[cfg(any(unix, windows, target_os = "wasi"))] pub mod crt_fd; pub mod encodings; diff --git a/vm/Cargo.toml b/vm/Cargo.toml index 1fdd700b75..9c85c4cc1a 100644 --- a/vm/Cargo.toml +++ b/vm/Cargo.toml @@ -51,6 +51,7 @@ bstr = { workspace = true } cfg-if = { workspace = true } crossbeam-utils = { workspace = true } chrono = { workspace = true, features = ["wasmbind"] } +constant_time_eq = { workspace = true } flame = { workspace = true, optional = true } getrandom = { workspace = true } hex = { workspace = true } diff --git a/vm/src/stdlib/operator.rs b/vm/src/stdlib/operator.rs index fbb8147e9f..2404b0c337 100644 --- a/vm/src/stdlib/operator.rs +++ b/vm/src/stdlib/operator.rs @@ -2,7 +2,6 @@ pub(crate) use _operator::make_module; #[pymodule] mod _operator { - use crate::common::cmp; use crate::{ AsObject, Py, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine, builtins::{PyInt, PyIntRef, PyStr, PyStrRef, PyTupleRef, PyTypeRef}, @@ -13,6 +12,7 @@ mod _operator { recursion::ReprGuard, types::{Callable, Constructor, PyComparisonOp, Representable}, }; + use constant_time_eq::constant_time_eq; #[pyfunction] fn lt(a: PyObjectRef, b: PyObjectRef, vm: &VirtualMachine) -> PyResult { @@ -328,11 +328,9 @@ mod _operator { "comparing strings with non-ASCII characters is not supported".to_owned(), )); } - cmp::timing_safe_cmp(a.as_bytes(), b.as_bytes()) - } - (Either::B(a), Either::B(b)) => { - a.with_ref(|a| b.with_ref(|b| cmp::timing_safe_cmp(a, b))) + constant_time_eq(a.as_bytes(), b.as_bytes()) } + (Either::B(a), Either::B(b)) => a.with_ref(|a| b.with_ref(|b| constant_time_eq(a, b))), _ => { return Err(vm.new_type_error( "unsupported operand types(s) or combination of types".to_owned(),