curve: use subtle::BlackBox optimization barrier
#662
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
tarcieri
force-pushed
the
use-subtle-blackbox
branch
from
fbd851b
to
b4a374c
Compare
tarcieri
added a commit
that referenced
this pull request
Jun 25, 2024
Alternative to #659/#661 and #662 which leverages `subtle::Choice` and `subtle::ConditionallySelectable` as the optimization barriers. Really the previous masking was there to conditionally add the scalar field modulus on underflow, so instead of that, we can conditionally select zero or the modulus using a `Choice` constructed from the underflow bit.
jmwample
pushed a commit
to jmwample/curve25519-dalek
that referenced
this pull request
Jun 26, 2024
…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
added a commit
that referenced
this pull request
Jul 17, 2024
Alternative to #659/#661 and #662 which leverages `subtle::Choice` and `subtle::ConditionallySelectable` as the optimization barriers. Really the previous masking was there to conditionally add the scalar field modulus on underflow, so instead of that, we can conditionally select zero or the modulus using a `Choice` constructed from the underflow bit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces the security mitigation added in #659 and #661 for masking-related timing variability which used an inline
black_boxusing the recently addedsubtle::BlackBoxnewtype (see dalek-cryptography/subtle#123)Internally
BlackBoxuses a volatile read by default (i.e. same strategy which was used before) or when thecore_hint_black_boxfeature ofsubtleis enabled, it usescore::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
BlackBoxfor bothmaskandunderflow_maskwhere previously it was only used onunderflow_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
jnsinstructions originally spotted in #659/#661: