Similar security fix to #659, but for the 32-bit backend. See that PR
for more information about the problem.

Similar security fix to #659, but for the 32-bit backend. See that PR
for more information about the problem.

Similar security fix to #659, but for the 32-bit backend. See that PR
for more information about the problem. Relevant compiler outputs (thanks to @tarcieri):

Without fix
https://godbolt.org/z/zvaWxzvqv
Notice the `jns` ("jump if not sign") instruction on line 106.

With fix
https://godbolt.org/z/jc9j7eb8E

## Description

Fixes `cargo-deny` warning for
[RUSTSEC-2024-0344](https://rustsec.org/advisories/RUSTSEC-2024-0344):

*Timing variability in `curve25519-dalek`'s
`Scalar29::sub`/`Scalar52::sub`*

Upstream PR:
dalek-cryptography/curve25519-dalek#659

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [x] Self-review.
- [ ] ~~Documentation updates if relevant.~~
- [ ] ~~Tests if relevant.~~
- [x] All breaking changes documented.

## Description

Fixes `cargo-deny` warning for
[RUSTSEC-2024-0344](https://rustsec.org/advisories/RUSTSEC-2024-0344):

*Timing variability in `curve25519-dalek`'s
`Scalar29::sub`/`Scalar52::sub`*

Upstream PR:
dalek-cryptography/curve25519-dalek#659

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [x] Self-review.
- [ ] ~~Documentation updates if relevant.~~
- [ ] ~~Tests if relevant.~~
- [x] All breaking changes documented.

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

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

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.

…ek-cryptography#659)

Timing variability of any kind is problematic when working with
potentially secret values such as elliptic curve scalars, and such
issues can potentially leak private keys and other secrets. Such a
problem was recently discovered in `curve25519-dalek`.

The `Scalar52::sub` function contained usage of a mask value inside of a
loop where LLVM saw an opportunity to insert a branch instruction
(`jns` on x86) to conditionally bypass this code section when the mask
value is set to zero, as can be seen in godbolt:

https://godbolt.org/z/PczYj7Pda

A similar problem was recently discovered in the Kyber reference
implementation:

https://groups.google.com/a/list.nist.gov/g/pqc-forum/c/hqbtIGFKIpU/m/cnE3pbueBgAJ

As discussed on that thread, one portable solution, which is also used
in this PR, is to introduce a volatile read as an optimization barrier,
which prevents the compiler from optimizing it away.

The fix can be validated in godbolt here:

https://godbolt.org/z/x8d46Yfah

The problem was discovered and the solution independently verified by
Alexander Wagner <[email protected]> and
Lea Themint <[email protected]> using their DATA tool:

https://github.com/Fraunhofer-AISEC/DATA

Co-authored-by: Tony Arcieri <[email protected]>

…ek-cryptography#661)

Similar security fix to dalek-cryptography#659, but for the 32-bit backend. See that PR
for more information about the problem. Relevant compiler outputs (thanks to @tarcieri):

Without fix
https://godbolt.org/z/zvaWxzvqv
Notice the `jns` ("jump if not sign") instruction on line 106.

With fix
https://godbolt.org/z/jc9j7eb8E

…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

…ek-cryptography#661)

Similar security fix to dalek-cryptography#659, but for the 32-bit backend. See that PR
for more information about the problem. Relevant compiler outputs (thanks to @tarcieri):

Without fix
https://godbolt.org/z/zvaWxzvqv
Notice the `jns` ("jump if not sign") instruction on line 106.

With fix
https://godbolt.org/z/jc9j7eb8E

…ek-cryptography#659)

Timing variability of any kind is problematic when working with
potentially secret values such as elliptic curve scalars, and such
issues can potentially leak private keys and other secrets. Such a
problem was recently discovered in `curve25519-dalek`.

The `Scalar52::sub` function contained usage of a mask value inside of a
loop where LLVM saw an opportunity to insert a branch instruction
(`jns` on x86) to conditionally bypass this code section when the mask
value is set to zero, as can be seen in godbolt:

https://godbolt.org/z/PczYj7Pda

A similar problem was recently discovered in the Kyber reference
implementation:

https://groups.google.com/a/list.nist.gov/g/pqc-forum/c/hqbtIGFKIpU/m/cnE3pbueBgAJ

As discussed on that thread, one portable solution, which is also used
in this PR, is to introduce a volatile read as an optimization barrier,
which prevents the compiler from optimizing it away.

The fix can be validated in godbolt here:

https://godbolt.org/z/x8d46Yfah

The problem was discovered and the solution independently verified by
Alexander Wagner <[email protected]> and
Lea Themint <[email protected]> using their DATA tool:

https://github.com/Fraunhofer-AISEC/DATA

Co-authored-by: Tony Arcieri <[email protected]>

…ek-cryptography#661)

Similar security fix to dalek-cryptography#659, but for the 32-bit backend. See that PR
for more information about the problem. Relevant compiler outputs (thanks to @tarcieri):

Without fix
https://godbolt.org/z/zvaWxzvqv
Notice the `jns` ("jump if not sign") instruction on line 106.

With fix
https://godbolt.org/z/jc9j7eb8E

…ek-cryptography#659)

Timing variability of any kind is problematic when working with
potentially secret values such as elliptic curve scalars, and such
issues can potentially leak private keys and other secrets. Such a
problem was recently discovered in `curve25519-dalek`.

The `Scalar52::sub` function contained usage of a mask value inside of a
loop where LLVM saw an opportunity to insert a branch instruction
(`jns` on x86) to conditionally bypass this code section when the mask
value is set to zero, as can be seen in godbolt:

https://godbolt.org/z/PczYj7Pda

A similar problem was recently discovered in the Kyber reference
implementation:

https://groups.google.com/a/list.nist.gov/g/pqc-forum/c/hqbtIGFKIpU/m/cnE3pbueBgAJ

As discussed on that thread, one portable solution, which is also used
in this PR, is to introduce a volatile read as an optimization barrier,
which prevents the compiler from optimizing it away.

The fix can be validated in godbolt here:

https://godbolt.org/z/x8d46Yfah

The problem was discovered and the solution independently verified by
Alexander Wagner <[email protected]> and
Lea Themint <[email protected]> using their DATA tool:

https://github.com/Fraunhofer-AISEC/DATA

Co-authored-by: Tony Arcieri <[email protected]>

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.

## Description

Fixes `cargo-deny` warning for
[RUSTSEC-2024-0344](https://rustsec.org/advisories/RUSTSEC-2024-0344):

*Timing variability in `curve25519-dalek`'s
`Scalar29::sub`/`Scalar52::sub`*

Upstream PR:
dalek-cryptography/curve25519-dalek#659

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [x] Self-review.
- [ ] ~~Documentation updates if relevant.~~
- [ ] ~~Tests if relevant.~~
- [x] All breaking changes documented.

## Description

Fixes `cargo-deny` warning for
[RUSTSEC-2024-0344](https://rustsec.org/advisories/RUSTSEC-2024-0344):

*Timing variability in `curve25519-dalek`'s
`Scalar29::sub`/`Scalar52::sub`*

Upstream PR:
dalek-cryptography/curve25519-dalek#659

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [x] Self-review.
- [ ] ~~Documentation updates if relevant.~~
- [ ] ~~Tests if relevant.~~
- [x] All breaking changes documented.

## Description

Fixes `cargo-deny` warning for
[RUSTSEC-2024-0344](https://rustsec.org/advisories/RUSTSEC-2024-0344):

*Timing variability in `curve25519-dalek`'s
`Scalar29::sub`/`Scalar52::sub`*

Upstream PR:
dalek-cryptography/curve25519-dalek#659

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [x] Self-review.
- [ ] ~~Documentation updates if relevant.~~
- [ ] ~~Tests if relevant.~~
- [x] All breaking changes documented.