WebCrypto in Safari will not AES-GCM encrypt 0 bytes

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

WebCrypto in Safari will not AES-GCM encrypt 0 bytes.

[Jiewen Tan]

rdar://75093377

[Jiewen Tan]

Created attachment 424955 [details]
Patch

[youenn fablet]

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> { };

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 425078 [details]
Patch

[youenn fablet]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 425300 [details]
Patch for landing

[Jiewen Tan]

rdar://59951871

[EWS]

Committed r275535: <https://commits.webkit.org/r275535>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425300 [details].

[Darin Adler]

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.

[Jiewen Tan]

(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.

[Jiewen Tan]

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>