[GCrypt] Zero-prefix (if necessary) RSA-OAEP encryption, RSA-PSS signing output
Created attachment 343431 [details] Patch
*** Bug 176759 has been marked as a duplicate of this bug. ***
*** Bug 180095 has been marked as a duplicate of this bug. ***
*** Bug 181139 has been marked as a duplicate of this bug. ***
*** Bug 176358 has been marked as a duplicate of this bug. ***
*** Bug 177529 has been marked as a duplicate of this bug. ***
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.
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.
Created attachment 343488 [details] Patch
Comment on attachment 343488 [details] Patch Clearing flags on attachment: 343488 Committed r233138: <https://trac.webkit.org/changeset/233138>
All reviewed patches have been landed. Closing bug.
<rdar://problem/41416540>
(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.