WebCrypto in Safari will not AES-GCM encrypt 0 bytes.
rdar://75093377
Created attachment 424955 [details] Patch
Comment on attachment 424955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424955&action=review > Source/WebCore/crypto/mac/CryptoAlgorithmAES_GCMMac.cpp:42 > + cipherText.reserveCapacity(cipherText.size() + desiredTagLengthInBytes); Can we just do Vector<uint8_t> cipherText(plainText.size() + desiredTagLengthInBytes) and update below code to memcpy the tag? This would allow to do a single allocation. > Source/WebCore/crypto/mac/CryptoAlgorithmAES_GCMMac.cpp:61 > + plainText.reserveCapacity(cipherText.size()); Are we guaranteed that cipherText.size() <= desiredTagLengthInBytes? If not, can we just write: if (cipherText.size() <= desiredTagLengthInBytes) return Vector<uint8_t> { }; If this is guaranteed, can we do: auto length = cipherText.size() - desiredTagLengthInBytes; if (!length) return Vector<uint8_t> { };
Comment on attachment 424955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424955&action=review Thanks Youenn for reviewing this patch. >> Source/WebCore/crypto/mac/CryptoAlgorithmAES_GCMMac.cpp:42 >> + cipherText.reserveCapacity(cipherText.size() + desiredTagLengthInBytes); > > Can we just do Vector<uint8_t> cipherText(plainText.size() + desiredTagLengthInBytes) and update below code to memcpy the tag? > This would allow to do a single allocation. Good catch. Fixed. >> Source/WebCore/crypto/mac/CryptoAlgorithmAES_GCMMac.cpp:61 >> + plainText.reserveCapacity(cipherText.size()); > > Are we guaranteed that cipherText.size() <= desiredTagLengthInBytes? > If not, can we just write: > if (cipherText.size() <= desiredTagLengthInBytes) > return Vector<uint8_t> { }; > > If this is guaranteed, can we do: > auto length = cipherText.size() - desiredTagLengthInBytes; > if (!length) > return Vector<uint8_t> { }; We cannot return empty vector directly. The cipher will contain the tag which will need to be validated by the AES-GCM algorithm.
Created attachment 425078 [details] Patch
Comment on attachment 425078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425078&action=review > Source/WebCore/crypto/mac/CryptoAlgorithmAES_GCMMac.cpp:73 > + plainText.shrink(cipherText.size() - desiredTagLengthInBytes); You could use offset here.
Comment on attachment 425078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425078&action=review Thanks Youenn for r+ this patch. >> Source/WebCore/crypto/mac/CryptoAlgorithmAES_GCMMac.cpp:73 >> + plainText.shrink(cipherText.size() - desiredTagLengthInBytes); > > You could use offset here. Good catch. Fixed.
Created attachment 425300 [details] Patch for landing
rdar://59951871
Committed r275535: <https://commits.webkit.org/r275535> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425300 [details].
Comment on attachment 425300 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=425300&action=review > Source/WebCore/crypto/mac/CryptoAlgorithmAES_GCMMac.cpp:40 > + // This is a wordaround for rdar://75093377. Force the buffer to be a non null pointer. Typo in "workaround" here, and 2 other places below. Also not sure we should call this a "workaround". Just because we misunderstood and thought the pointer could be null, doesn’t mean we should comment each line saying "workaround" like this. Honestly, I find the relationship of the comment to he code confusing. I am fairly expect on how Vector works and I can’t find anything here that is "forcing" a buffer to be a non-null pointer.
(In reply to Darin Adler from comment #11) > Comment on attachment 425300 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425300&action=review > > > Source/WebCore/crypto/mac/CryptoAlgorithmAES_GCMMac.cpp:40 > > + // This is a wordaround for rdar://75093377. Force the buffer to be a non null pointer. > > Typo in "workaround" here, and 2 other places below. > > Also not sure we should call this a "workaround". Just because we > misunderstood and thought the pointer could be null, doesn’t mean we should > comment each line saying "workaround" like this. > > Honestly, I find the relationship of the comment to he code confusing. I am > fairly expect on how Vector works and I can’t find anything here that is > "forcing" a buffer to be a non-null pointer. Got you. Will land a follow up to fix the comments. As long as the Vector's capacity is larger than 0, then .data() will be non-null pointer.
A follow up according to Darin's comment. Committed r275678: <https://commits.webkit.org/r275678> Committed r275678 (236314@main): <https://commits.webkit.org/236314@main>