Add `BlackBox` #123

tarcieri · 2024-06-19T02:10:29Z

Adds a VolatileCell-like struct which introduces an optimization barrier on all accesses.

The *Cell-like get() method is the only accessor for the inner value and uses a more generalized (i.e. for all Copy types) black_box to preclude optimizations.

This is useful for things like bitwise mask values to ensure LLVM doesn't try to optimize around special cases like 0, especially in the context of loops.

For an example use case, see:

https://rustsec.org/advisories/RUSTSEC-2024-0344.html

tarcieri · 2024-06-19T02:53:35Z

src/lib.rs

-        // Unsafe is ok, because:
-        //   - &input is not NULL;
-        //   - size of input is not zero;
-        //   - u8 is neither Sync, nor Send;


See also #91

src/lib.rs

tarcieri · 2024-06-19T21:37:40Z

~~Note: this PR could use a constructor and a test~~

Edit: added in c0774e9

Adds a `VolatileCell`-like struct which introduces an optimization barrier on all accesses. The `*Cell`-like `get()` method is the only accessor for the inner value and uses a more generalized (i.e. for all `Copy` types) `black_box` to preclude optimizations. This is useful for things like bitwise mask values to ensure LLVM doesn't try to optimize around special cases like `0`, especially in the context of loops. For an example use case, see: https://rustsec.org/advisories/RUSTSEC-2024-0344.html

Replaces the security mitigation added in #659 and #661 for masking-related timing variability which used an inline `black_box` using the recently added `subtle::BlackBox` newtype (see dalek-cryptography/subtle#123) Internally `BlackBox` uses a volatile read by default (i.e. same strategy which was used before) or when the `core_hint_black_box` feature of `subtle` is enabled, it uses `core::hint::black_box` (whose documentation was recently updated to reflect the nuances of potential cryptographic use, see rust-lang/rust#126703) This PR goes ahead and uses `BlackBox` for both `mask` and `underflow_mask` where previously it was only used on `underflow_mask`. The general pattern of bitwise masking inside a loop seems worrisome for the optimizer potentially inserting branches in the future. Below are godbolt inspections of the generated assembly, which are free of the `jns` instructions originally spotted in #659/#661: - 32-bit (read_volatile): https://godbolt.org/z/TKo9fqza4 - 32-bit (hint::black_box): https://godbolt.org/z/caoMxYbET - 64-bit (read_volatile): https://godbolt.org/z/PM6zKjj1f - 64-bit (hint::black_box): https://godbolt.org/z/nseaPvdWv

Seems some changes I force pushed to dalek-cryptography#123 didn't wind up getting merged. In that PR, I noted that `pub const fn new` was MSRV breaking and got rid of the `const` but that didn't end up in `develop`. I also encountered a build failure on `develop` since the legacy `black_box` function wasn't gated on the `core_hint_black_box` feature and thus clashed with `core::hint::black_box` when it was imported: ``` Compiling subtle v2.6.0 (/Users/tarcieri/src/subtle) error[E0255]: the name `black_box` is defined multiple times --> src/lib.rs:223:1 | 100 | use core::hint::black_box; | --------------------- previous import of the value `black_box` here ... 223 | fn black_box<T: Copy>(input: T) -> T { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `black_box` redefined here | = note: `black_box` must be defined only once in the value namespace of this module help: you can use `as` to change the binding name of the import | 100 | use core::hint::black_box as other_black_box; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ warning: unused import: `core::hint::black_box` --> src/lib.rs:100:5 | 100 | use core::hint::black_box; | ^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default ``` This is a breaking change since we shipped a `const fn` for `BlackBox::new` already, so I'd suggest releasing this as 2.6.1 and yanking 2.6.0 for being unintentionally MSRV breaking.

Replaces the security mitigation added in #659 and #661 for masking-related timing variability which used an inline `black_box` using the recently added `subtle::BlackBox` newtype (see dalek-cryptography/subtle#123) Internally `BlackBox` uses a volatile read by default (i.e. same strategy which was used before) or when the `core_hint_black_box` feature of `subtle` is enabled, it uses `core::hint::black_box` (whose documentation was recently updated to reflect the nuances of potential cryptographic use, see rust-lang/rust#126703) This PR goes ahead and uses `BlackBox` for both `mask` and `underflow_mask` where previously it was only used on `underflow_mask`. The general pattern of bitwise masking inside a loop seems worrisome for the optimizer potentially inserting branches in the future. Below are godbolt inspections of the generated assembly, which are free of the `jns` instructions originally spotted in #659/#661: - 32-bit (read_volatile): https://godbolt.org/z/TKo9fqza4 - 32-bit (hint::black_box): https://godbolt.org/z/caoMxYbET - 64-bit (read_volatile): https://godbolt.org/z/PM6zKjj1f - 64-bit (hint::black_box): https://godbolt.org/z/nseaPvdWv

…y#662) Replaces the security mitigation added in dalek-cryptography#659 and dalek-cryptography#661 for masking-related timing variability which used an inline `black_box` using the recently added `subtle::BlackBox` newtype (see dalek-cryptography/subtle#123) Internally `BlackBox` uses a volatile read by default (i.e. same strategy which was used before) or when the `core_hint_black_box` feature of `subtle` is enabled, it uses `core::hint::black_box` (whose documentation was recently updated to reflect the nuances of potential cryptographic use, see rust-lang/rust#126703) This PR goes ahead and uses `BlackBox` for both `mask` and `underflow_mask` where previously it was only used on `underflow_mask`. The general pattern of bitwise masking inside a loop seems worrisome for the optimizer potentially inserting branches in the future. Below are godbolt inspections of the generated assembly, which are free of the `jns` instructions originally spotted in dalek-cryptography#659/dalek-cryptography#661: - 32-bit (read_volatile): https://godbolt.org/z/TKo9fqza4 - 32-bit (hint::black_box): https://godbolt.org/z/caoMxYbET - 64-bit (read_volatile): https://godbolt.org/z/PM6zKjj1f - 64-bit (hint::black_box): https://godbolt.org/z/nseaPvdWv

tarcieri changed the base branch from main to develop June 19, 2024 02:49

tarcieri commented Jun 19, 2024

View reviewed changes

isislovecruft approved these changes Jun 19, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

tarcieri force-pushed the black-box branch 3 times, most recently from 45f03d4 to 068de49 Compare June 19, 2024 21:50

tarcieri force-pushed the black-box branch from 068de49 to bf87dee Compare June 19, 2024 21:54

isislovecruft merged commit 3e8d92b into dalek-cryptography:develop Jun 19, 2024
3 of 4 checks passed

tarcieri deleted the black-box branch June 19, 2024 22:30

tarcieri mentioned this pull request Jun 20, 2024

curve: use subtle::BlackBox optimization barrier dalek-cryptography/curve25519-dalek#662

Merged

tarcieri mentioned this pull request Jun 21, 2024

Restore 1.60 MSRV and fix build #125

Merged

divergentdave mentioned this pull request Jul 1, 2024

Remove incorrect comment about core_hint_black_box #130

Open

redshiftzero added a commit to redshiftzero/subtle-ng that referenced this pull request Aug 14, 2024

backport dalek-cryptography#123

d53e273

redshiftzero mentioned this pull request Aug 14, 2024

backport BlackBox zkcrypto/subtle-ng#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `BlackBox` #123

Add `BlackBox` #123

tarcieri commented Jun 19, 2024

tarcieri Jun 19, 2024

tarcieri commented Jun 19, 2024 •

edited

Loading

Add BlackBox #123

Add BlackBox #123

Conversation

tarcieri commented Jun 19, 2024

tarcieri Jun 19, 2024

Choose a reason for hiding this comment

tarcieri commented Jun 19, 2024 • edited Loading

Add `BlackBox` #123

Add `BlackBox` #123

tarcieri commented Jun 19, 2024 •

edited

Loading