Complete re-do of Secret and Zeroize

[ounsworth]

The old way we were doing Secret to force a zeroizing Drop was well-intentioned, but likely completely useless since by definition nothing can read a value after it is Drop'd, so the compiler is well within its rights to elide that write. This complete redo of the whole zeroizing architecture does a few things:

1. Concentrate all the Secret behaviours (zeroizing Drop / Debug / Display) into a new module bouncycastle-utils::Secret.
2. The new object Secret<T> can be wrapped around any u8, u16, u32, u64, u128, usize, i8, i16, i32, i64, i128, isize, bool, and char or arrays thereof. It impl's Deref and DerefMut, so its presence is largely transparent.
   For example:

   struct HMAC<..> {
     key: [u8; LEN],
   }
   

   becomes:

   struct HMAC<..> {
     key: Secret<[u8; LEN]>,
   }
   

   and everything just works. Secret<T> doesn't even add any memory overhead ... ie size_of(T) == size_of(Secret<T>), which is cool.
3. The Secret implementation uses (very small) unsafe blocks to ensure that the zeroizing write (and constant-time comparison) can't be elided or short-circuited by the compiler). This means that the bouncycastle-utils crate is no longer #![forbid(unsafe_code)], which is probably right because eventually we'd need that for constant-time behaviour anyway, but with this architecture, the unsafe rust is contained to the utils crate and does not infect the algorithm crates.
4. This means that structs like HMAC, MLDSAPrivateKey, etc no longer need to be responsible for zeroizing Drop or redacting Debug / Display (which, honestly, was fairly error-prone), so we get to delete a bunch of code that we don't need anymore, including the Secret trait that we used to have..

If merged, replaces #36 and closes #32.

Reviewers should start with crypto/utils/src/secret.rs as this is the core new thing that needs to be scrutinized.

The rest is a massive refactor to use the new construct (which went surprisingly easily, which I guess validates the design).

This PR is almost a net decrease in lines-of-code since it removes the requirement for all structs to impl Drop, Debug and Display, and instead delegates that to the new bouncycastle-utils::Secret object.

[npajkovsky]

The overall implementation is sound and way better than what it was. However, this bit requires a final touch.

Having a loop where every byte written into the index is volatile is too much. You just need to prevent the variable from being elided as a whole, so marking the base is just enough.

You can even simplify it further, and use core::hint::black_box; on base_ptr. black_box uses inline assembly and marks base_ptr variable as clobbered memory. The trick is that compiler does not understand asm, hence cannot optimize it.

[ounsworth]

I'm aware of the black_box trick, and I considered this also, but the contract on
https://doc.rust-lang.org/std/hint/fn.black_box.html
very clearly says that it is a "best effort"
whereas the contract on
https://doc.rust-lang.org/std/ptr/fn.write_volatile.html
is much stronger.

[tad-fr]

Given this assumption, could the implementation not be simplified to :

unsafe {
    ptr::write_volatile(self, core::mem::zeroed());
}

Maybe I am missing something but I would've thought that achieves identical results without manually looping.

[ounsworth]

Great suggestion!
I have updated to simplify this.

[romen]

I had a look at this earlier this morning, and we had some back and forth in private.
I am posting this to summarize (and cleanup, as I was commenting on the phone :) ), the main points.

---

My impression is that there aren't substantial difference between the new struct Secret of this PR, and the struct Zeroizing from the zeroize crate.

At this point, I am not strongly advocating against this PR to stick with the RustCrypto zeroize crate, but I'd like to have a discussion about the tradeoffs that come with the choice of implementing "our own zeroize".

The downsides to me seem to be:

• we lose the #![forbid(unsafe_code)]
  • good that this was moved from bc-core to bc-utils, and as you noted maybe that crate would need to use unsafe anyway for some constant-time shenanigans in the future
• we have to study the UB around volatile/non-volatile patterns
  • see note at the bottom
• we have to keep up with rustlang/LLVM semantics/evolution
  • this means we will also have to monitor across different target triplets as they become supported by Rust
  • personally to me this seems to be the most impactful (in terms of maintenance)

On the pro side:

• convenience factor of the struct Secret design compatibility with the rest of the bc-rust type design.
  • Personal note: I expect this should improve both readability and maintenance.
• one less external dependency
  • being as self-contained as possible has been one of the driving directions for the bc-rust project since the beginning, so I do appreciate how much this is impactful.

I am not advocating against this PR in favor of the RustCrypto zeroize crate, but I believe this discussion needs to happen, the tradeoffs evaluated, and the design decision documented.

Please, fire away your considerations!

---

Note on volatile and Undefined Behavior

IIRC, the volatile pattern in the zeroize crate has been refined over time to avoid undefined behavior, because there were subtle nuances to the intuitive way of using volatile for this.
My understanding is that they still worked in practice on available LLVM versions, but were not entirely kosher nor future proof.

This is an excerpt from the zeroize docs that should sum-up the current state:

What guarantees does this crate provide?

This crate guarantees the zeroing operation can't be "optimized away" by the compiler, as
ensured by LLVM's volatile semantics.

Previously there were worries that the approach used by this crate (mixing volatile and
non-volatile accesses) was undefined behavior due to language contained
in the documentation for write_volatile, however after some discussion
within the Unsafe Code Guidelines Working Group, these remarks have been removed and the
specific usage pattern in this crate is considered to be well-defined.

All of that said, there is still potential for microarchitectural attacks
(ala Spectre/Meltdown) to leak "zeroized" secrets through covert channels.
This crate makes no guarantees that zeroized values cannot be leaked
through such channels, as they represent flaws in the underlying hardware.

[romen]

One more suggestion:

I believe our previous attempts at using the zeroize crate have been negatively impacted by the lack of "real" examples in its documentation.

The documentation of Secret here has a similar problem, it shows what to do for a trivial (but relevant) "secret" array of u8.
I think we would benefit from adding an example of a more complex struct, with secret and public fields.
Maybe inspired to the MLKEMPrivateKey?

[ounsworth]

Thanks @romen. Very valuable discussion.
At the moment I am quite comfortable with the contract on write_volatile, and I appreciate the work that the rustcrypto team did to make it that way. I'm willing to undertake maintenance of this as the compiler evolves -- I think that we'll need to go there eventually anyway for other aspects of side-channel resistance that we won't be able to just borrow from rustcrypto.

At some point, side-channel protection -- especially the kind where you are trying to protect your process against a malicious host / OS / root process -- is a losing battle. At some point you have to say "best effort" somewhere in your docs. That's not to say that we can't increase our level of effort here in the future, but for today I think this PR is a dramatic increase over what we had before.

[ounsworth]

@romen How's this for a toy example of applying this to a more complex struct?
I've added that to the docstring for the Secret module.

use bouncycastle_utils::secret::{Secret, ZeroizablePrimitive};

/// Holds a system user
#[derive(Clone, Copy)]
struct User {
    userid: i32,
    name: [u8; 64],
}

/// Provide the const ZEROED value for the type.
impl ZeroizablePrimitive for User {
    const ZEROED: Self = Self{ userid: 0i32, name: [0u8; 64] };
}

/// We will tag the admins as Secret<User> to give them extra protections against
/// having their info leaked.
struct AllUsers {
    users: Vec<User>,
    admins: Vec<Secret<User>>,
}

[ounsworth]

The commit above re-works ML-DSA and ML-KEM to make more surgical use of the new Secret type by being more selective about which components are tagged as secret. This gives about a 5% performance increase over the previous commit.

As an example, the MLDSAPrivateKey object is now defined as:

pub struct MLDSAPrivateKey<..> {
    rho: [u8; 32],
    K: Secret<[u8; 32]>,
    tr: [u8; 64],
    s1_hat: Secret<Vector<l>>,
    s2_hat: Secret<Vector<k>>,
    t0_hat: Vector<k>,
    seed: Option<KeyMaterial<32>>,
}

which clearly indicates to a human reader which values are secret (ie the actual private key components) and which are not (ie part of the public key). This also gives developers more fine-grained control over which data pays the extra performance penalty associated with zeroization-on-drop.

[ounsworth]

Merged manually, but with a --squash, so might not close manually.