Bug 186967 – [GCrypt] Zero-prefix (if necessary) output of RSA-based encryption and signing operations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[GCrypt] Zero-prefix (if necessary) RSA-OAEP encryption, RSA-PSS signing output

[Zan Dobersek]

Created attachment 343431 [details]
Patch

[Zan Dobersek]

*** Bug 176759 has been marked as a duplicate of this bug. ***

[Zan Dobersek]

*** Bug 180095 has been marked as a duplicate of this bug. ***

[Zan Dobersek]

*** Bug 181139 has been marked as a duplicate of this bug. ***

[Zan Dobersek]

*** Bug 176358 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

*** Bug 177529 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

Comment on attachment 343431 [details]
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.

[Zan Dobersek]

Comment on attachment 343431 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSAES_PKCS1_v1_5GCrypt.cpp:98
>> +    ASSERT(!(key.keySizeInBits() % 8));
> 
> I'd probably use ASSERT_WITH_SECURITY_IMPLICATION

OK.

>> 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?

I don't think this harms cache locality. `inline` specifier is primarily used to denote functions that are defined in headers and that should remain the same in definition between different source files. Nowadays it's completely up to the compiler whether it decides to inline the function or not. This I guess primarily depends on compilation options (optimization flags, debugging options, link-time optimizations). If the `inline` specifier is removed, the compiler already complains of the function being unused in whatever translation unit the header is included in.

What I can do is move all the definitions into a standalone GCryptUtilities.cpp source file.

>> Source/WebCore/crypto/gcrypt/GCryptUtilities.h:191
>> +    // 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.

Thanks.

[Zan Dobersek]

Created attachment 343488 [details]
Patch

[Zan Dobersek]

Comment on attachment 343488 [details]
Patch

Clearing flags on attachment: 343488

Committed r233138: <https://trac.webkit.org/changeset/233138>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41416540>

[Zan Dobersek]

(In reply to Zan Dobersek from comment #8) 
> What I can do is move all the definitions into a standalone
> GCryptUtilities.cpp source file.
> 

Bug #187033.