[webkit-reviews] review granted: [Bug 186967] [GCrypt] Zero-prefix (if necessary) output of RSA-based encryption and signing operations : [Attachment 343431] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 186967: [GCrypt] Zero-prefix (if necessary) output of RSA-based encryption
and signing operations
https://bugs.webkit.org/show_bug.cgi?id=186967

Attachment 343431: Patch

https://bugs.webkit.org/attachment.cgi?id=343431&action=review




--- Comment #7 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 343431
  --> https://bugs.webkit.org/attachment.cgi?id=343431
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343431&action=review

Excellent

> Source/WebCore/ChangeLog:11
> +	   length of the RSA key. The way we retrieve the MPI data means
libgcrypt
> +	   can ignore the high-bit zero values and leave us with a valid result
> +	   that's shorter in length compared to the RSA key. For instance, if
the

Wow!

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSAES_PKCS1_v1_5GCrypt.cpp:98
> +    ASSERT(!(key.keySizeInBits() % 8));

I'd probably use ASSERT_WITH_SECURITY_IMPLICATION

> Source/WebCore/crypto/gcrypt/GCryptUtilities.h:183
> +static inline std::optional<Vector<uint8_t>> mpiZeroPrefixedData(gcry_mpi_t
paramMPI, size_t targetLength)

Isn't it pretty long for an inline hint? How long does it have to get before
you worry about cache locality?

> Source/WebCore/crypto/gcrypt/GCryptUtilities.h:191
> +    // Fill out the output buffer with zeros. Properly determine the zero
prefix length,
> +    // and copy the MPI data into memory area following the prefix (if any).

These are good comments. It's a difficult art, finding the right balance
between too much information and not enough. OK, just felt you deserved a
compliment here.