Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SECURITY: fix timing variability in backend/serial/u64/scalar.rs #659

Merged
merged 1 commit into from
Jun 18, 2024
Merged

SECURITY: fix timing variability in backend/serial/u64/scalar.rs #659

merged 1 commit into from
Jun 18, 2024

Conversation

rozbb
Copy link
Contributor

@rozbb rozbb commented Jun 18, 2024

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 alexander.wagner@aisec.fraunhofer.de and Lea Themint lea.thiemt@tum.de using their DATA tool:

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

@tarcieri
SECURITY: fix timing variability in backend/serial/u64/scalar.rs
8f38163 
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 <alexander.wagner@aisec.fraunhofer.de> and
Lea Themint <lea.thiemt@tum.de> using their DATA tool:

https://github.com/Fraunhofer-AISEC/DATA
@rozbb rozbb requested a review from tarcieri June 18, 2024 17:09
@rozbb rozbb merged commit 415892a into main Jun 18, 2024
44 checks passed
@tarcieri tarcieri deleted the fix branch June 18, 2024 17:50
tarcieri added a commit that referenced this pull request Jun 18, 2024
@tarcieri
SECURITY: fix timing variability in backend/serial/u32/scalar.rs
4e6a2cf 
Similar security fix to #659, but for the 32-bit backend. See that PR
for more information about the problem.
@tarcieri tarcieri mentioned this pull request Jun 18, 2024
Merged
tarcieri added a commit that referenced this pull request Jun 18, 2024
@tarcieri
SECURITY: fix timing variability in backend/serial/u32/scalar.rs
dac6f1b 
Similar security fix to #659, but for the 32-bit backend. See that PR
for more information about the problem.
rozbb pushed a commit that referenced this pull request Jun 18, 2024
@tarcieri
SECURITY: fix timing variability in backend/serial/u32/scalar.rs (#661)
b4f9e4d 
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
@Frando Frando mentioned this pull request Jun 18, 2024
Merged
4 tasks
github-merge-queue bot pushed a commit to n0-computer/iroh that referenced this pull request Jun 18, 2024
@Frando
deps: bump curve25519-dalek (#2382)
96081e5 
## 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.
This was referenced Jun 19, 2024
Open
Closed
Open
Open
Open
Closed
Open
Open
Open
Closed
Open
@nazar-pc nazar-pc mentioned this pull request Jun 19, 2024
Merged
7 tasks
This was referenced Jun 19, 2024
Closed
Open
@samuelburnham samuelburnham mentioned this pull request Jun 19, 2024
Merged
This was referenced Jun 20, 2024
Open
Open
tarcieri added a commit that referenced this pull request Jun 20, 2024
@tarcieri
curve: use subtle::BlackBox optimization barrier
fbd851b 
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 tarcieri mentioned this pull request Jun 20, 2024
Merged
This was referenced Jun 20, 2024
Open
Open
Open
Open
Open
ppodolsky pushed a commit to izihawa/iroh that referenced this pull request Jun 22, 2024
@Frando @ppodolsky
deps: bump curve25519-dalek (n0-computer#2382)
17b1f23 
## 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.
This was referenced Jun 23, 2024
Closed
Closed
tarcieri added a commit that referenced this pull request Jun 24, 2024
@tarcieri
curve: use subtle::BlackBox optimization barrier
b4a374c 
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
rozbb pushed a commit that referenced this pull request Jun 24, 2024
@tarcieri
curve: use subtle::BlackBox optimization barrier (#662)
5b7082b 
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 added a commit that referenced this pull request Jun 25, 2024
@tarcieri
curve: use subtle::Choice for constant-time fixes
6ab5814 
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.
@tarcieri tarcieri mentioned this pull request Jun 25, 2024
Merged
jmwample pushed a commit to jmwample/curve25519-dalek that referenced this pull request Jun 26, 2024
@rozbb @tarcieri @jmwample
SECURITY: fix timing variability in backend/serial/u64/scalar.rs (dal…
700f602 
…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 <alexander.wagner@aisec.fraunhofer.de> and
Lea Themint <lea.thiemt@tum.de> using their DATA tool:

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

Co-authored-by: Tony Arcieri <bascule@gmail.com>
jmwample pushed a commit to jmwample/curve25519-dalek that referenced this pull request Jun 26, 2024
@tarcieri @jmwample
SECURITY: fix timing variability in backend/serial/u32/scalar.rs (dal…
d55ca44 
…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
jmwample pushed a commit to jmwample/curve25519-dalek that referenced this pull request Jun 26, 2024
@tarcieri @jmwample
curve: use subtle::BlackBox optimization barrier (dalek-cryptograph…
676d572 
…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
yihau pushed a commit to anza-xyz/curve25519-dalek that referenced this pull request Jun 27, 2024
@tarcieri @yihau
SECURITY: fix timing variability in backend/serial/u32/scalar.rs (dal…
d10985b 
…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
yihau pushed a commit to anza-xyz/curve25519-dalek that referenced this pull request Jun 27, 2024
@rozbb @tarcieri @yihau
SECURITY: fix timing variability in backend/serial/u64/scalar.rs (dal…
ecda2f8 
…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 <alexander.wagner@aisec.fraunhofer.de> and
Lea Themint <lea.thiemt@tum.de> using their DATA tool:

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

Co-authored-by: Tony Arcieri <bascule@gmail.com>
yihau pushed a commit to anza-xyz/curve25519-dalek that referenced this pull request Jun 27, 2024
@tarcieri @yihau
SECURITY: fix timing variability in backend/serial/u32/scalar.rs (dal…
aea9c17 
…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
yihau pushed a commit to anza-xyz/curve25519-dalek that referenced this pull request Jun 27, 2024
@rozbb @tarcieri @yihau
SECURITY: fix timing variability in backend/serial/u64/scalar.rs (dal…
0382b67 
…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 <alexander.wagner@aisec.fraunhofer.de> and
Lea Themint <lea.thiemt@tum.de> using their DATA tool:

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

Co-authored-by: Tony Arcieri <bascule@gmail.com>
@github-actions github-actions bot mentioned this pull request Jul 1, 2024
Open
tarcieri added a commit that referenced this pull request Jul 17, 2024
@tarcieri
curve: use subtle::Choice for constant-time fixes (#665)
921bd7c 
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.
@MPSxDev MPSxDev mentioned this pull request Aug 5, 2024
Open
redshiftzero added a commit to redshiftzero/curve25519-dalek-ng that referenced this pull request Aug 14, 2024
@redshiftzero
backport dalek-cryptography#659
a2f0c3d
@redshiftzero redshiftzero mentioned this pull request Aug 14, 2024
Open
@github-actions github-actions bot mentioned this pull request Sep 1, 2024
Open
rklaehn pushed a commit to n0-computer/iroh-blobs that referenced this pull request Oct 22, 2024
@Frando
deps: bump curve25519-dalek (#2382)
111c89b 
## 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.
rklaehn pushed a commit to n0-computer/iroh-blobs that referenced this pull request Oct 22, 2024
@Frando
deps: bump curve25519-dalek (#2382)
2290baa 
## 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.
@tarcieri tarcieri mentioned this pull request Oct 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarcieri tarcieri tarcieri approved these changes

Assignees
No one assigned
Labels
bug safety - constant timedness
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rozbb @tarcieri