Maniphest T7136

libgcrypt: Implement constant-time RSA decryption (Marvin attack fix)
Open, LowPublic
Actions

Assigned To
None
Authored By
Jakuje
May 28 2024, 7:07 PM
Tags
Subscribers
• gniibe
Jakuje
jukivili
pl13
• werner

Description

This was previously filled in gitlab mirror, but re-posting rebased patches based on initial feedback here:

https://gitlab.com/redhat-crypto/libgcrypt/libgcrypt-mirror/-/merge_requests/17

The current MPI code is not constant time, potentially leaking plaintext
when the attacker can observe enough decipher operations using RSA
PKCS#1.5. This is described as a Marvin Attack:

The changes are likely not complete as there are still some timing leaks detected, but if needed I can follow-up.

libgcrypt-marvin.patch31 KBDownload

CVE-id: CVE-2024-2236

Details

Version
master

Event Timeline

Jakuje created this task.May 28 2024, 7:07 PM
jukivili added a subscriber: jukivili.May 29 2024, 8:01 AM

I left review comments in gitlab. One additional concern is license for mpi-mul-cs.c, original code not having copyright information... "does not have any copyright information, assuming public domain".

• werner triaged this task as Low priority.May 29 2024, 8:22 AM
• werner added subscribers: • gniibe, • werner.

We discussed this forth and back with the RedHat people at our jour-fix to explain that the Kairo fix is done at the wrong layer - this needs to be done at the protocol layer and not in the building blocks. This is not covered by our security policy and @gniibe already came up with some extra support to help at the protocol layer. There are only a few use cases where this side-channel or the Minerva one (for ECDSA) should be considered (e.g. time stamping services). Generally required protection against DoS are also pat of the mitigation.

Thus set the priority to low.

Jakuje added a comment.May 29 2024, 3:03 PM

I left review comments in gitlab.

Thank you!

One additional concern is license for mpi-mul-cs.c, original code not having copyright information... "does not have any copyright information, assuming public domain".

Just discussed with Hubert that the license is not specified and he is happy to provide it under any license you would need.

pl13 added a subscriber: pl13.Wed, Jun 10, 8:34 AM
• werner updated the task description. (Show Details)Wed, Jun 10, 10:06 AM
• gniibe added a comment.Thu, Jun 11, 6:40 AM

Review again in 2026.

  • For the commit of rsa: Do not accept invalid PKCS#1.5 padding when deciphering
    • It is correct in terms of RFC8017, but old standard RFC2313 didn't specify the requirement. So, we can't apply this change (we need to support older encrypted messages).
  • For the commit of rsa: Constant time blinding removal
    • As of now, we don't need mpi-mul-cs.c; we have _gcry_mpih_mul_lli and mpih_mod_lli. Revising the change, we will have better code.
  • For the commit of Constant time conversion of the message to the SEXP
    • While I understand the intention and the effectiveness for gcry_pk_decrypt timing, I think that more changes are needed to be really constant-time; The fundamental problem is the representation in SEXP can be variable length. The change fixes the timing by %c which does dummy write, but the result is still variable length, so, the computation after gcry_pk_decrypt highly likely exposes timing difference, still.
    • Real improvement would be: The result of decoding should be fixed-length, padded by zero, and the result of RSA value in SEXP should be fixed-length, determined by the length of modulus N. I think that conforming the current standard of PKCS#1, this is a kind of requirement (I2OSP. OS2IP). This is major behavior change, so, careful treatment is needed, though.
  • For the commit of rsa: Implement constant-time conversion of MPI to string
    • It makes sense to have such a function for fixed-length representation.
  • For the commit of Implement implicit rejection for PKCS#1.5 decipher
    • I have been watching the IETF draft (to be RFC?) for the implicit rejection of RSA. If it will be recommended and there will be some demand among users, it's good to have it in libgcrypt.
• gniibe mentioned this in Unknown Object (Maniphest Task).Mon, Jun 15, 4:15 AM