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

apply RSA-CRT hardening for PKCS1 sign operations; use eqaf for comparing cstructs in constant time #15

Merged
merged 2 commits into from
Mar 2, 2020

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Feb 26, 2020

No description provided.

@hannesm
Copy link
Member Author

hannesm commented Feb 26, 2020

the remaining Z.powm occurences only use the public keys, and are thus not timing sensitive.

@hannesm hannesm requested review from dinosaure and emillon February 26, 2020 11:05
@hannesm hannesm changed the title use Z.powm_sec for private key operations to avoid timing side channels use Z.powm_sec for private key operations to avoid timing side channels; apply RSA-CRT hardening for PKCS1 sign operations Feb 26, 2020
Copy link
Member

@dinosaure dinosaure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really aware about powm_sec and pown and I did not get the real diff between them on my side with bechamel (and see how many times they need to compute same arguments). But according the documentation, it seems ok!

mirage-crypto-pk.opam Outdated Show resolved Hide resolved
Copy link
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just a small metadata remark.

Copy link
Collaborator

@cfcs cfcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to review this more in-depth, but here's my feedback so far :)

pk/mirage_crypto_pk.mli Outdated Show resolved Hide resolved
pk/rsa.ml Show resolved Hide resolved
@hannesm
Copy link
Member Author

hannesm commented Feb 27, 2020

rebased on master and force-pushed

@hannesm hannesm requested a review from cfcs February 27, 2020 11:03
@hannesm hannesm mentioned this pull request Feb 27, 2020
mirage-crypto-pk.opam Outdated Show resolved Hide resolved
pk/rsa.ml Show resolved Hide resolved
pk/rsa.ml Show resolved Hide resolved
@cfcs
Copy link
Collaborator

cfcs commented Feb 28, 2020

Just found this, which does not sound so good: https://lists.lysator.liu.se/pipermail/nettle-bugs/2016/003104.html

The function mpz_powm_sec() is not a drop in replacement for mpz_powm(),
although it may sound like it. If you look at the docs [1] a
requirement for mpz_powm_sec() is that the modulus must be odd.
If the modulus is even / divisible by 2 then mpz_powm_sec() will crash
with a floating point exception.

# Z.powm_sec (Z.of_int 2) (Z.of_int 2) (Z.of_int 2);;
Exception: Invalid_argument "Z.powm_sec: modulus must be odd".

This is (and other things like caml_invalid_argument("Z.powm_sec: exponent must be positive"); is handled with exceptions in Zarith, is that OK for us in this context?
https://github.com/ocaml/Zarith/blob/a9a309d0596d93b6c0c902951e1cae13d661bebd/caml_z.c#L2710-L2714

hannesm added 2 commits March 2, 2020 10:37
- PKCS1 sign verifies that the computed signature is a valid signature (see
  Arjen Lenstra 1996, Florian Weimer 2015
  https://people.redhat.com/~fweimer/rsa-crt-leaks.pdf)
- other RSA operations (including decrypt / PSS / ..): extend with optional
  `rsa_crt_hardening:bool` argument (defaulting to false)
@hannesm hannesm changed the title use Z.powm_sec for private key operations to avoid timing side channels; apply RSA-CRT hardening for PKCS1 sign operations apply RSA-CRT hardening for PKCS1 sign operations; use eqaf for comparing cstructs in constant time Mar 2, 2020
@hannesm
Copy link
Member Author

hannesm commented Mar 2, 2020

I removed the usage of powm_sec from this PR, it looks a bit too controversial of (a) when to use (b) whether to use (failure semantics -- raises if modulus is even). Hope this PR is less controversial for @cfcs, and we can move on by merging this PR.

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 this pull request may close these issues.

4 participants