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.

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

P-521 sign not constant-time #228

Closed
ansiwen opened this issue May 5, 2024 · 7 comments · Fixed by #230
Closed

P-521 sign not constant-time #228

ansiwen opened this issue May 5, 2024 · 7 comments · Fixed by #230

Comments

@ansiwen
Copy link

ansiwen commented May 5, 2024

While doing performance measurements I noticed, that P-521 signatures are not constant-time, which I assume they should be. P-256 is not affected. I see the same behavior with and without the recent performance improvements for elliptic curves. However, I'm not using the latest mirage-crypto, but 0.10.7. Here the result of a simple test script, always signing the same message but generating a new key every two measurements:

Generating new key
requests:  1402 ( 4 conn) time: 4s rps:  350.56 latency: 10.78ms/11.38ms/19.22ms
requests:  1347 ( 4 conn) time: 4s rps:  336.88 latency: 10.79ms/11.84ms/29.74ms
Generating new key
requests:  1583 ( 4 conn) time: 4s rps:  395.94 latency: 8.77ms/10.07ms/16.48ms
requests:  1591 ( 4 conn) time: 4s rps:  397.87 latency: 8.42ms/10.02ms/17.3ms
Generating new key
requests:  1658 ( 4 conn) time: 4s rps:  414.71 latency: 9.02ms/9.62ms/18.06ms
requests:  1667 ( 4 conn) time: 4s rps:  416.97 latency: 8.95ms/9.56ms/19.09ms
Generating new key
requests:   956 ( 4 conn) time: 4s rps:  239.14 latency: 10.52ms/16.68ms/23.01ms
requests:   951 ( 4 conn) time: 4s rps:  237.89 latency: 16.05ms/16.76ms/30.08ms
Generating new key
requests:  1150 ( 4 conn) time: 4s rps:  287.71 latency: 12.86ms/13.86ms/34.07ms
requests:  1192 ( 4 conn) time: 4s rps:  298.21 latency: 9.87ms/13.37ms/20.07ms
Generating new key
requests:  1240 ( 4 conn) time: 4s rps:  310.17 latency: 11.95ms/12.86ms/30.99ms
requests:  1276 ( 4 conn) time: 4s rps:  319.10 latency: 11.97ms/12.5ms/31.4ms
Generating new key
requests:   695 ( 4 conn) time: 4s rps:  173.82 latency: 12.77ms/22.94ms/29.86ms
requests:   703 ( 4 conn) time: 4s rps:  175.90 latency: 11.16ms/22.67ms/32.57ms
Generating new key
requests:  1261 ( 4 conn) time: 4s rps:  315.45 latency: 10.32ms/12.64ms/29.33ms
requests:  1256 ( 4 conn) time: 4s rps:  314.01 latency: 11.76ms/12.7ms/21.3ms

@Firobe could reproduce the effect with a modified mirage-crypto benchmark:

* [ecdsa-timing-key1]
    :  566.780 ops per second (5712 iters in 10.078)

* [ecdsa-timing-key2]
    :  480.910 ops per second (4830 iters in 10.043)

* [ecdsa-timing-key3]
    :  338.418 ops per second (3396 iters in 10.035)

* [ecdsa-timing-key4]
    :  681.658 ops per second (6811 iters in 9.992)

* [ecdsa-timing-key5]
    :  554.708 ops per second (5564 iters in 10.031)
@Firobe
Copy link
Member

Firobe commented May 6, 2024

I believe the non-constant part is entirely due to the computation of k when not provided, which is mentioned by the documentation of sign as a risk.

On master, on a constant message and 5 random keys for P521, with k pre-computed with the same method as when not provided:

ops/s Verify Sign with pre-computed k Sign without k
Key 1 175.7 1189.9 573.6
Key 2 175.8 1188.1 487.1
Key 3 175.4 1190.3 344.3
Key 4 174.7 1186.4 695.0
Key 5 173.8 1179.7 563.0
Std. dev. 0.74 3.84 115.33

The computation of k seems to take a huge part of the total spent time (compared to other curves, see below), though I don't have an intuition yet for why . I'll note that the low-level primitives for P521 are generated from a different fiat-crypto algorithm (unsaturated solinas) than all other P-curves (word-by-word-montgomery). Maybe it's also worth to look into Digestif.SHA512.

For comparison, here are the same measurements on P256 (excluding verify):

ops/s Sign with pre-computed k Sign without k
Key 1 8586.3 8388.4
Key 2 8593.2 8467.0
Key 3 8612.1 8377.6
Key 4 8604.4 8363.0
Key 5 8626.3 8392.4
Std. dev. 14.13 36.13

@hannesm
Copy link
Member

hannesm commented May 7, 2024

I'll note that the low-level primitives for P521 are generated from a different fiat-crypto algorithm (unsaturated solinas) than all other P-curves (word-by-word-montgomery).

That is not the case, all of them are using word-by-word-montgomery. The unsaturated salinas is used by ed25519.

Maybe worth to measure how often do_sign is called (which computes k and is recursive)? Maybe worth to investigate #105 - maybe the K_gen gen () function (as well recursive) is called too often?

@ansiwen for what it is worth, please note that old mirage-crypto releases are unsupported. If you like to report an issue in the future, please test with the latest release.

@Firobe
Copy link
Member

Firobe commented May 7, 2024

That is not the case, all of them are using word-by-word-montgomery. The unsaturated salinas is used by ed25519.

Ah my bad, I looked into fiat-crypto directly assuming we'd have reused these.

Maybe worth to measure how often do_sign is called (which computes k and is recursive)?

It will be useful to measure it just in case, but it probably is called once in all cases, or else it would fail when I pre-compute and pass k (since again stops after the first iteration if k was passed).

Maybe worth to investigate #105 - maybe the K_gen gen () function (as well recursive) is called too often?

I'll try to continue to investigate, thanks for the ideas :)

@ansiwen
Copy link
Author

ansiwen commented May 7, 2024

@ansiwen for what it is worth, please note that old mirage-crypto releases are unsupported. If you like to report an issue in the future, please test with the latest release.

We did. That's why I added the reproduction of @Firobe, which was on master.

@Firobe
Copy link
Member

Firobe commented May 7, 2024

@hannesm Your idea was spot on :) See #230

With this, here's the distribution over the same sample of keys

ops/s Sign without k
Key 1 1161
Key 2 1170
Key 3 1162
Key 4 1164
Key 5 1159
Std. dev. 6.05

@ansiwen
Copy link
Author

ansiwen commented May 7, 2024

@Firobe Well done! Great work!
Just for my understanding: the k is calculated deterministically from the key, or why was it constant over identical keys?

@Firobe
Copy link
Member

Firobe commented May 8, 2024

@ansiwen In classical ECDSA, k is chosen randomly from a good, uniform, unbiased random source. These characteristics are not always possible to have, and hard to test, so the RFC proposes a process to compute k deterministically from the key and the message instead (which is supposed to be indistiguishable from random from the PoV of an attacker).

This does mean the k is the same for each similar message, so it can be used to recover the key if there are other vulnerabilities (https://www.hertzbleed.com/2h2b.pdf). In practice to be truly secure, a k should be generated randomly if a good random source is available, and passed explicitly.

hannesm added a commit to hannesm/opam-repository that referenced this issue Jun 29, 2024
CHANGES:

### Breaking changes

* mirage-crypto: Poly1305 API now uses string (mirage/mirage-crypto#203 @hannesm)
* mirage-crypto: Poly1305 no longer has type alias "type mac = string"
  (mirage/mirage-crypto#232 @hannesm)
* mirage-crypto: the API uses string instead of cstruct (mirage/mirage-crypto#214 @reynir @hannesm)
* mirage-crypto: Hash module has been removed. Use digestif if you need hash
  functions (mirage/mirage-crypto#213 @hannesm)
* mirage-crypto: the Cipher_block and Cipher_stream modules have been removed,
  its contents is inlined:
  Mirage_crypto.Cipher_block.S -> Mirage_crypto.Block
  Mirage_crypto.Cipher_stream.S -> Mirage_crypto.Stream
  Mirage_crypto.Cipher_block.AES.CTR -> Mirage_crypto.AES.CTR
  (mirage/mirage-crypto#225 @hannesm, suggested in mirage/mirage-crypto#224 by @reynir)
* mirage-crypto-pk: s-expression conversions for private and public keys (Dh,
  Dsa, Rsa) have been removed. You can use PKCS8 for encoding and decoding
  `X509.{Private,Public}_key.{en,de}code_{der,pem}` (mirage/mirage-crypto#208 @hannesm)
* mirage-crypto-pk: in the API, Cstruct.t is no longer present. Instead,
  string is used (mirage/mirage-crypto#211 @reynir @hannesm)
* mirage-crypto-rng: the API uses string instead of Cstruct.t. A new function
  `generate_into : ?g -> bytes -> ?off:int -> int -> unit` is provided
  (mirage/mirage-crypto#212 @hannesm @reynir)
* mirage-crypto-ec: remove NIST P224 support (mirage/mirage-crypto#209 @hannesm @Firobe)
* mirage-crypto: in Uncommon.xor_into the arguments ~src_off and ~dst_off are
  required now (mirage/mirage-crypto#232 @hannesm), renamed to unsafe_xor_into
  (98f01b14f5ebf98ba0e7e9c2ba97ec518f90fddc)
* mirage-crypto-pk, mirage-crypto-rng: remove type alias "type bits = int"
  (mirage/mirage-crypto#236 @hannesm)

### Bugfixes

* mirage-crypto (32 bit systems): CCM with long adata (mirage/mirage-crypto#207 @reynir)
* mirage-crypto-ec: fix K_gen for bitlen mod 8 != 0 (reported in mirage/mirage-crypto#105 that
  P521 test vectors don't pass, re-reported mirage/mirage-crypto#228, fixed mirage/mirage-crypto#230 @Firobe)
* mirage-crypto-ec: zero out bytes allocated for Field_element.zero (reported
  mirleft/ocaml-x509#167, fixed mirage/mirage-crypto#226 @dinosaure)

### Data race free

* mirage-crypto (3DES): avoid global state in key derivation (mirage/mirage-crypto#223 @hannesm)
* mirage-crypto-rng: use atomic instead of reference to be domain-safe (mirage/mirage-crypto#221
  @dinosaure @reynir @hannesm)
* mirage-crypto, mirage-crypto-rng, mirage-crypto-pk, mirage-crypto-ec:
  avoid global buffers, use freshly allocated strings/bytes instead, avoids
  data races (mirage/mirage-crypto#186 mirage/mirage-crypto#219 @dinosaure @reynir @hannesm)

### Other changes

* mirage-crypto: add {de,en}crypt_into functions (and unsafe variants) to allow
  less buffer allocations (mirage/mirage-crypto#231 @hannesm)
* mirage-crypto-rng-miou: new package which adds rng support with miou
  (mirage/mirage-crypto#227 @dinosaure)
* PERFORMANCE mirage-crypto: ChaCha20/Poly1305 use string instead of Cstruct.t,
  ChaCha20 interface unchanged, performance improvement roughly 2x
  (mirage/mirage-crypto#203 @hannesm @reynir)
* mirage-crypto-ec, mirage-crypto-pk, mirage-crypto-rng: use digestif for
  hashes (mirage/mirage-crypto#212 mirage/mirage-crypto#215 @reynir @hannesm)
* mirage-crypto-rng: use a set for entropy sources instead of a list
  (mirage/mirage-crypto#218 @hannesm)
* mirage-crypto-rng-mirage: provide a module type S (for use instead of
  mirage-random in mirage) (mirage/mirage-crypto#234 @hannesm)
avsm pushed a commit to avsm/opam-repository that referenced this issue Sep 5, 2024
CHANGES:

### Breaking changes

* mirage-crypto: Poly1305 API now uses string (mirage/mirage-crypto#203 @hannesm)
* mirage-crypto: Poly1305 no longer has type alias "type mac = string"
  (mirage/mirage-crypto#232 @hannesm)
* mirage-crypto: the API uses string instead of cstruct (mirage/mirage-crypto#214 @reynir @hannesm)
* mirage-crypto: Hash module has been removed. Use digestif if you need hash
  functions (mirage/mirage-crypto#213 @hannesm)
* mirage-crypto: the Cipher_block and Cipher_stream modules have been removed,
  its contents is inlined:
  Mirage_crypto.Cipher_block.S -> Mirage_crypto.Block
  Mirage_crypto.Cipher_stream.S -> Mirage_crypto.Stream
  Mirage_crypto.Cipher_block.AES.CTR -> Mirage_crypto.AES.CTR
  (mirage/mirage-crypto#225 @hannesm, suggested in mirage/mirage-crypto#224 by @reynir)
* mirage-crypto-pk: s-expression conversions for private and public keys (Dh,
  Dsa, Rsa) have been removed. You can use PKCS8 for encoding and decoding
  `X509.{Private,Public}_key.{en,de}code_{der,pem}` (mirage/mirage-crypto#208 @hannesm)
* mirage-crypto-pk: in the API, Cstruct.t is no longer present. Instead,
  string is used (mirage/mirage-crypto#211 @reynir @hannesm)
* mirage-crypto-rng: the API uses string instead of Cstruct.t. A new function
  `generate_into : ?g -> bytes -> ?off:int -> int -> unit` is provided
  (mirage/mirage-crypto#212 @hannesm @reynir)
* mirage-crypto-ec: remove NIST P224 support (mirage/mirage-crypto#209 @hannesm @Firobe)
* mirage-crypto: in Uncommon.xor_into the arguments ~src_off and ~dst_off are
  required now (mirage/mirage-crypto#232 @hannesm), renamed to unsafe_xor_into
  (98f01b14f5ebf98ba0e7e9c2ba97ec518f90fddc)
* mirage-crypto-pk, mirage-crypto-rng: remove type alias "type bits = int"
  (mirage/mirage-crypto#236 @hannesm)

### Bugfixes

* mirage-crypto (32 bit systems): CCM with long adata (mirage/mirage-crypto#207 @reynir)
* mirage-crypto-ec: fix K_gen for bitlen mod 8 != 0 (reported in mirage/mirage-crypto#105 that
  P521 test vectors don't pass, re-reported mirage/mirage-crypto#228, fixed mirage/mirage-crypto#230 @Firobe)
* mirage-crypto-ec: zero out bytes allocated for Field_element.zero (reported
  mirleft/ocaml-x509#167, fixed mirage/mirage-crypto#226 @dinosaure)

### Data race free

* mirage-crypto (3DES): avoid global state in key derivation (mirage/mirage-crypto#223 @hannesm)
* mirage-crypto-rng: use atomic instead of reference to be domain-safe (mirage/mirage-crypto#221
  @dinosaure @reynir @hannesm)
* mirage-crypto, mirage-crypto-rng, mirage-crypto-pk, mirage-crypto-ec:
  avoid global buffers, use freshly allocated strings/bytes instead, avoids
  data races (mirage/mirage-crypto#186 mirage/mirage-crypto#219 @dinosaure @reynir @hannesm)

### Other changes

* mirage-crypto: add {de,en}crypt_into functions (and unsafe variants) to allow
  less buffer allocations (mirage/mirage-crypto#231 @hannesm)
* mirage-crypto-rng-miou: new package which adds rng support with miou
  (mirage/mirage-crypto#227 @dinosaure)
* PERFORMANCE mirage-crypto: ChaCha20/Poly1305 use string instead of Cstruct.t,
  ChaCha20 interface unchanged, performance improvement roughly 2x
  (mirage/mirage-crypto#203 @hannesm @reynir)
* mirage-crypto-ec, mirage-crypto-pk, mirage-crypto-rng: use digestif for
  hashes (mirage/mirage-crypto#212 mirage/mirage-crypto#215 @reynir @hannesm)
* mirage-crypto-rng: use a set for entropy sources instead of a list
  (mirage/mirage-crypto#218 @hannesm)
* mirage-crypto-rng-mirage: provide a module type S (for use instead of
  mirage-random in mirage) (mirage/mirage-crypto#234 @hannesm)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants