Document assumptions made by bssl-crypto's unboxed HMAC_CTX
I believe it is currently fine, but we probably should either box it, or
get to the point that the assumptions are less precarious. Rust FFI is
anything but safe.
Bug: 682
Change-Id: I4b45dd3c3f58fb0ce7c0b8b80b1e6d7d2f7f119f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65627
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/fipsmodule/digest/digest.c b/crypto/fipsmodule/digest/digest.c
index f499c46..52197c1 100644
--- a/crypto/fipsmodule/digest/digest.c
+++ b/crypto/fipsmodule/digest/digest.c
@@ -185,6 +185,10 @@
void EVP_MD_CTX_move(EVP_MD_CTX *out, EVP_MD_CTX *in) {
EVP_MD_CTX_cleanup(out);
// While not guaranteed, |EVP_MD_CTX| is currently safe to move with |memcpy|.
+ // bssl-crypto currently relies on this, however, so if we change this, we
+ // need to box the |HMAC_CTX|. (Relying on this is only fine because we assume
+ // BoringSSL and bssl-crypto will always be updated atomically. We do not
+ // allow any version skew between the two.)
OPENSSL_memcpy(out, in, sizeof(EVP_MD_CTX));
EVP_MD_CTX_init(in);
}
diff --git a/rust/bssl-crypto/README.md b/rust/bssl-crypto/README.md
index 9518862..678f45e 100644
--- a/rust/bssl-crypto/README.md
+++ b/rust/bssl-crypto/README.md
@@ -10,5 +10,5 @@
Unlike BoringSSL itself, this crate does not attempt to handle allocation failures. If an allocation fails, functions in this crate will panic.
-WARNING - This crate is experimental and does *NOT* have a stable API. We expect to iterate on the API as it develops. If you use this crate you must be prepared to adapt your code to future changes as they occur.
+WARNING - This crate is experimental and does *NOT* have a stable API. We expect to iterate on the API as it develops. If you use this crate you must be prepared to adapt your code to future changes as they occur. Additionally, this crate must be updated atomically with BoringSSL. The crate, internally, may depend on implementation details of the library.
diff --git a/rust/bssl-crypto/src/hmac.rs b/rust/bssl-crypto/src/hmac.rs
index bf482f7..5924fa8 100644
--- a/rust/bssl-crypto/src/hmac.rs
+++ b/rust/bssl-crypto/src/hmac.rs
@@ -234,6 +234,10 @@
/// until the Rust language can support the `min_const_generics` feature. Until then we will have to
/// pass both separately: https://github.com/rust-lang/rust/issues/60551
struct Hmac<const N: usize, MD: digest::Algorithm> {
+ // Safety: this relies on HMAC_CTX being relocatable via `memcpy`, which is
+ // not generally true of BoringSSL types. This is fine to rely on only
+ // because we do not allow any version skew between bssl-crypto and
+ // BoringSSL. It is *not* safe to copy this code in any other project.
ctx: bssl_sys::HMAC_CTX,
_marker: PhantomData<MD>,
}
@@ -366,6 +370,7 @@
#[cfg(test)]
mod tests {
use super::*;
+ use alloc::boxed::Box;
#[test]
fn hmac_sha256() {
@@ -401,10 +406,13 @@
let mut hmac = HmacSha256::new_from_slice(&key);
hmac.update(&data[..1]);
let mut hmac2 = hmac.clone();
+ let mut hmac3 = Box::new(hmac2.clone());
hmac.update(&data[1..]);
hmac2.update(&data[1..]);
+ hmac3.update(&data[1..]);
assert_eq!(hmac.digest(), expected);
assert_eq!(hmac2.digest(), expected);
+ assert_eq!(hmac3.digest(), expected);
}
#[test]
@@ -458,9 +466,12 @@
let mut hmac = HmacSha512::new_from_slice(&key);
hmac.update(&data[..1]);
let mut hmac2 = hmac.clone();
+ let mut hmac3 = Box::new(hmac.clone());
hmac.update(&data[1..]);
hmac2.update(&data[1..]);
+ hmac3.update(&data[1..]);
assert_eq!(hmac.digest(), expected);
assert_eq!(hmac2.digest(), expected);
+ assert_eq!(hmac3.digest(), expected);
}
}
