Bug 227804 – [WebCrypto] decrypt() does not properly validate padding for AES-CBC algorithm

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

decrypt() does not properly validate padding for AES-CBC algorithm:
- https://www.w3.org/TR/WebCryptoAPI/#aes-cbc-operations (Decrypt section)

[Chris Dumez]

Created attachment 433139 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 433139 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        - https://www.w3.org/TR/WebCryptoAPI/#aes-cbc-operations (Decrypt section)

However, please see https://github.com/w3c/webcrypto/issues/177, which is closed bit not for a great reason AFAICT.

[Kate Cheney]

Comment on attachment 433139 [details]
Patch

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

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:77
>      ASSERT(parameters.ivVector().size() == kCCBlockSizeAES128 || parameters.ivVector().isEmpty());

Based on the spec, don't we want to return an OperationError here if the ivVector is the wrong size?

> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:79
> +    auto result = transformAES_CBC(kCCDecrypt, parameters.ivVector(), key.key(), cipherText, Padding::No);

I am confused as to why we are always passing Padding::No here, can you explain?

[Chris Dumez]

(In reply to Kate Cheney from comment #3)
> Comment on attachment 433139 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433139&action=review
> 
> > Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:77
> >      ASSERT(parameters.ivVector().size() == kCCBlockSizeAES128 || parameters.ivVector().isEmpty());
> 
> Based on the spec, don't we want to return an OperationError here if the
> ivVector is the wrong size?

I haven’t looked into that part and do not know if this is causing any test failure. This seems like an issue separate from what I am fixing.

> > Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:79
> > +    auto result = transformAES_CBC(kCCDecrypt, parameters.ivVector(), key.key(), cipherText, Padding::No);
> 
> I am confused as to why we are always passing Padding::No here, can you
> explain?

If we pass Yes, then Common Crypto will trim the padding for us but it does not seem to do padding validation that matches the spec, causing us to fail WPT tests if we let common crypto deal with the padding. For this reason, we ask common crypto to NOT deal with padding after decrypting and we validate / trim the padding ourselves as a post processing step.

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #2)
> Comment on attachment 433139 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433139&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        - https://www.w3.org/TR/WebCryptoAPI/#aes-cbc-operations (Decrypt section)
> 
> However, please see https://github.com/w3c/webcrypto/issues/177, which is
> closed bit not for a great reason AFAICT.

It was closed for not being backward compatible. We are the only browser behaving this way and I do not think that’s a good thing.

[Radar WebKit Bug Importer]

<rdar://problem/80639418>

[Angela]

I am currently working on this https://github.com/WebKit/WebKit/pull/3819

[EWS]

Committed 254308@main (0062d4332b57): <https://commits.webkit.org/254308@main>

Reviewed commits have been landed. Closing PR #3819 and removing active labels.