Bug 207174

Summary: Add WebCrypto LayoutTests to check if PKCS#7 padding is correctly implemented in AES-CBC
Product: WebKit Reporter: Tomoki Imai <tomoki.imai>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, don.olmstead, ews-watchlist, Hironori.Fujii, jiewen_tan, tomoki.imai, webkit-bug-importer, yoshiaki.jitsukawa
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
jiewen_tan: review-
patch none

Description Tomoki Imai 2020-02-03 18:21:31 PST
While we're working on bug 206439, we noticed that layouttests for AES-CBC lacks the testcases for PKCS#7 padding.
It would be nice to have these testcases for new implementers.

- https://www.w3.org/TR/WebCryptoAPI/#aes-cbc-operations
- bug 193153
Comment 1 Tomoki Imai 2020-02-03 18:24:07 PST
Created attachment 389613 [details]
patch

Patch to add more testcases to crypto/subtle/aes-cbc-import-key-decrypt.html and crypto/subtle/aes-cbc-import-key-encrypt.html
Comment 2 Jiewen Tan 2020-02-04 11:30:00 PST
Not sure why this is needed? You have to implement PKCS#7 yourself?
Comment 3 Tomoki Imai 2020-02-04 21:02:00 PST
(In reply to Jiewen Tan from comment #2)
> Not sure why this is needed? You have to implement PKCS#7 yourself?

I think we need this to validate all the padding is done as expected, and our usage of underlying libraries like CommonCrypt, GCrypt, OpenSSL are correct.

For OpenSSL backend, we don't have to implement PKCS#7 by ourselves, but we need to setup enough memory for encrypted/decrypted data, which may contains PKCS#7 padding.

Actually we had a bug in calculating the encrypted data length when the length % 8 == 0, and the case was not covered by the current LayoutTest. bug 207176
Comment 4 Tomoki Imai 2020-02-05 02:31:02 PST
(In reply to Tomoki Imai from comment #3)

> Actually we had a bug in calculating the encrypted data length when the
> length % 8 == 0, and the case was not covered by the current LayoutTest. bug
> 207176

Correction: length % 16 == 0.
When the length % 16 == 0, we need to add 16 bytes for padding, but our first implementation didn't add any padding, but passed all the LayoutTest.
https://en.wikipedia.org/wiki/Padding_(cryptography)#PKCS%235_and_PKCS%237
Comment 5 Jiewen Tan 2020-02-05 11:33:33 PST
(In reply to Tomoki Imai from comment #3)
> (In reply to Jiewen Tan from comment #2)
> > Not sure why this is needed? You have to implement PKCS#7 yourself?
> 
> I think we need this to validate all the padding is done as expected, and
> our usage of underlying libraries like CommonCrypt, GCrypt, OpenSSL are
> correct.

I don't buy that. Just like you don't write tests to validate WTF before you use stuffs from WTF. And I believe if you don't use PKCS#7, the existing tests won't pass.

> 
> For OpenSSL backend, we don't have to implement PKCS#7 by ourselves, but we
> need to setup enough memory for encrypted/decrypted data, which may contains
> PKCS#7 padding.
> 
> Actually we had a bug in calculating the encrypted data length when the
> length % 8 == 0, and the case was not covered by the current LayoutTest. bug
> 207176

Then maybe you should write a test in that patch to help you validate your implementation instead of adding new tests like these here.
Comment 6 Jiewen Tan 2020-02-05 11:33:56 PST
Comment on attachment 389613 [details]
patch

r- for the reasons stated above.
Comment 7 Jiewen Tan 2020-02-05 11:44:53 PST
(In reply to Tomoki Imai from comment #4)
> (In reply to Tomoki Imai from comment #3)
> 
> > Actually we had a bug in calculating the encrypted data length when the
> > length % 8 == 0, and the case was not covered by the current LayoutTest. bug
> > 207176
> 
> Correction: length % 16 == 0.
> When the length % 16 == 0, we need to add 16 bytes for padding, but our
> first implementation didn't add any padding, but passed all the LayoutTest.
> https://en.wikipedia.org/wiki/Padding_(cryptography)#PKCS%235_and_PKCS%237

You could modify the existing test to have the plain text that has a length of multiple of 16 in your other patch. Then the original test is then sufficient enough.
Comment 8 Tomoki Imai 2020-02-06 19:44:38 PST
(In reply to Jiewen Tan from comment #5)
> (In reply to Tomoki Imai from comment #3)
> > (In reply to Jiewen Tan from comment #2)
> > > Not sure why this is needed? You have to implement PKCS#7 yourself?
> > 
> > I think we need this to validate all the padding is done as expected, and
> > our usage of underlying libraries like CommonCrypt, GCrypt, OpenSSL are
> > correct.
> 
> I don't buy that. Just like you don't write tests to validate WTF before you
> use stuffs from WTF. And I believe if you don't use PKCS#7, the existing
> tests won't pass.

Thanks for your reviews!

Right, We don't need to validate CommonCrypt, GCrypt, and OpenSSL itself.
I think the PKCS#7 test cases to cover boundary values are needed from the viewpoint of JavaScript interface, in the other word, black-box testing.
In my opinion, we should validate all the JavaScript interface, regardless of how they are implemented in platform layer.

> And I believe if you don't use PKCS#7, the existing tests won't pass.

Yes, but we didn't have enough test cases to cover boundary values in PKCS#7 now.

> > 
> > For OpenSSL backend, we don't have to implement PKCS#7 by ourselves, but we
> > need to setup enough memory for encrypted/decrypted data, which may contains
> > PKCS#7 padding.
> > 
> > Actually we had a bug in calculating the encrypted data length when the
> > length % 8 == 0, and the case was not covered by the current LayoutTest. bug
> > 207176
> 
> Then maybe you should write a test in that patch to help you validate your
> implementation instead of adding new tests like these here.

Sure, another option was adding these test cases in bug 207176.
If it's better option than making another patch, then I can move the test case to bug 207176.

The reason why I made a separate bug is that attachment 389613 [details] affects the port, but bug 207176 only for OpenSSL backend one.
Comment 9 Jiewen Tan 2020-02-07 12:39:23 PST
(In reply to Tomoki Imai from comment #8)
> (In reply to Jiewen Tan from comment #5)
> > (In reply to Tomoki Imai from comment #3)
> > > (In reply to Jiewen Tan from comment #2)
> > > > Not sure why this is needed? You have to implement PKCS#7 yourself?
> > > 
> > > I think we need this to validate all the padding is done as expected, and
> > > our usage of underlying libraries like CommonCrypt, GCrypt, OpenSSL are
> > > correct.
> > 
> > I don't buy that. Just like you don't write tests to validate WTF before you
> > use stuffs from WTF. And I believe if you don't use PKCS#7, the existing
> > tests won't pass.
> 
> Thanks for your reviews!
> 
> Right, We don't need to validate CommonCrypt, GCrypt, and OpenSSL itself.
> I think the PKCS#7 test cases to cover boundary values are needed from the
> viewpoint of JavaScript interface, in the other word, black-box testing.
> In my opinion, we should validate all the JavaScript interface, regardless
> of how they are implemented in platform layer.

Alright, I usually focus on minimum coverage and leave the burden of black box to wpt.

> 
> > And I believe if you don't use PKCS#7, the existing tests won't pass.
> 
> Yes, but we didn't have enough test cases to cover boundary values in PKCS#7
> now.
> 
> > > 
> > > For OpenSSL backend, we don't have to implement PKCS#7 by ourselves, but we
> > > need to setup enough memory for encrypted/decrypted data, which may contains
> > > PKCS#7 padding.
> > > 
> > > Actually we had a bug in calculating the encrypted data length when the
> > > length % 8 == 0, and the case was not covered by the current LayoutTest. bug
> > > 207176
> > 
> > Then maybe you should write a test in that patch to help you validate your
> > implementation instead of adding new tests like these here.
> 
> Sure, another option was adding these test cases in bug 207176.
> If it's better option than making another patch, then I can move the test
> case to bug 207176.
> 
> The reason why I made a separate bug is that attachment 389613 [details]
> affects the port, but bug 207176 only for OpenSSL backend one.

I think it is better to have tests along with the implementation within the same commit. Then, when you look back the commit, you could reason the decision you made from tests.

Given the other commit is landed, you can continue on this one. You should combine them together next time. Otherwise, it is very hard for reviewers to reason the relations between these two patches.
Comment 10 Jiewen Tan 2020-02-07 12:40:42 PST
Comment on attachment 389613 [details]
patch

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

> LayoutTests/crypto/subtle/aes-cbc-import-key-encrypt.html:185
> +        debug(plainText);

This debug information should be more express like "Plain text: xxxx".
Comment 11 Jiewen Tan 2020-02-07 12:41:03 PST
(In reply to Jiewen Tan from comment #10)
> Comment on attachment 389613 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389613&action=review
> 
> > LayoutTests/crypto/subtle/aes-cbc-import-key-encrypt.html:185
> > +        debug(plainText);
> 
> This debug information should be more express like "Plain text: xxxx".

expressive
Comment 12 Tomoki Imai 2020-02-10 03:56:44 PST
(In reply to Jiewen Tan from comment #9)
> (In reply to Tomoki Imai from comment #8)
> > (In reply to Jiewen Tan from comment #5)
> > > (In reply to Tomoki Imai from comment #3)
> > > > (In reply to Jiewen Tan from comment #2)
> > > > > Not sure why this is needed? You have to implement PKCS#7 yourself?
> > > > 
> > > > I think we need this to validate all the padding is done as expected, and
> > > > our usage of underlying libraries like CommonCrypt, GCrypt, OpenSSL are
> > > > correct.
> > > 
> > > I don't buy that. Just like you don't write tests to validate WTF before you
> > > use stuffs from WTF. And I believe if you don't use PKCS#7, the existing
> > > tests won't pass.
> > 
> > Thanks for your reviews!
> > 
> > Right, We don't need to validate CommonCrypt, GCrypt, and OpenSSL itself.
> > I think the PKCS#7 test cases to cover boundary values are needed from the
> > viewpoint of JavaScript interface, in the other word, black-box testing.
> > In my opinion, we should validate all the JavaScript interface, regardless
> > of how they are implemented in platform layer.
> 
> Alright, I usually focus on minimum coverage and leave the burden of black
> box to wpt.

I'm not sure what "wpt" stands for, but it helps us to understand the current situation.
Is it OK for you to add more LayoutTests to increase the coverage mainly for our OpenSSL implementation?
It should help the other port solid for example bug 207375.

> 
> > 
> > > And I believe if you don't use PKCS#7, the existing tests won't pass.
> > 
> > Yes, but we didn't have enough test cases to cover boundary values in PKCS#7
> > now.
> > 
> > > > 
> > > > For OpenSSL backend, we don't have to implement PKCS#7 by ourselves, but we
> > > > need to setup enough memory for encrypted/decrypted data, which may contains
> > > > PKCS#7 padding.
> > > > 
> > > > Actually we had a bug in calculating the encrypted data length when the
> > > > length % 8 == 0, and the case was not covered by the current LayoutTest. bug
> > > > 207176
> > > 
> > > Then maybe you should write a test in that patch to help you validate your
> > > implementation instead of adding new tests like these here.
> > 
> > Sure, another option was adding these test cases in bug 207176.
> > If it's better option than making another patch, then I can move the test
> > case to bug 207176.
> > 
> > The reason why I made a separate bug is that attachment 389613 [details]
> > affects the port, but bug 207176 only for OpenSSL backend one.
> 
> I think it is better to have tests along with the implementation within the
> same commit. Then, when you look back the commit, you could reason the
> decision you made from tests.
> 
> Given the other commit is landed, you can continue on this one. You should
> combine them together next time. Otherwise, it is very hard for reviewers to
> reason the relations between these two patches.

OK, thanks for letting me know about it. I'll do so from the next time.
Comment 13 Tomoki Imai 2020-02-10 04:07:44 PST
Fujii Hironori -san told me that wpt stands for web platform test.

It seems to have only one "plainText".
https://github.com/web-platform-tests/wpt/blob/master/WebCryptoAPI/encrypt_decrypt/aes_cbc_vectors.js
Comment 14 Tomoki Imai 2020-02-10 04:51:21 PST
Created attachment 390245 [details]
patch

Made debug messages more expressive as suggested in comment 10
Comment 15 Jiewen Tan 2020-02-10 11:08:18 PST
(In reply to Tomoki Imai from comment #13)
> Fujii Hironori -san told me that wpt stands for web platform test.
> 
> It seems to have only one "plainText".
> https://github.com/web-platform-tests/wpt/blob/master/WebCryptoAPI/
> encrypt_decrypt/aes_cbc_vectors.js

I believe they do have badPadding to test padding stuff, but probably for decryption only.
Comment 16 Jiewen Tan 2020-02-10 11:10:12 PST
(In reply to Tomoki Imai from comment #12)
> (In reply to Jiewen Tan from comment #9)
> > (In reply to Tomoki Imai from comment #8)
> > > (In reply to Jiewen Tan from comment #5)
> > > > (In reply to Tomoki Imai from comment #3)
> > > > > (In reply to Jiewen Tan from comment #2)
> > > > > > Not sure why this is needed? You have to implement PKCS#7 yourself?
> > > > > 
> > > > > I think we need this to validate all the padding is done as expected, and
> > > > > our usage of underlying libraries like CommonCrypt, GCrypt, OpenSSL are
> > > > > correct.
> > > > 
> > > > I don't buy that. Just like you don't write tests to validate WTF before you
> > > > use stuffs from WTF. And I believe if you don't use PKCS#7, the existing
> > > > tests won't pass.
> > > 
> > > Thanks for your reviews!
> > > 
> > > Right, We don't need to validate CommonCrypt, GCrypt, and OpenSSL itself.
> > > I think the PKCS#7 test cases to cover boundary values are needed from the
> > > viewpoint of JavaScript interface, in the other word, black-box testing.
> > > In my opinion, we should validate all the JavaScript interface, regardless
> > > of how they are implemented in platform layer.
> > 
> > Alright, I usually focus on minimum coverage and leave the burden of black
> > box to wpt.
> 
> I'm not sure what "wpt" stands for, but it helps us to understand the
> current situation.
> Is it OK for you to add more LayoutTests to increase the coverage mainly for
> our OpenSSL implementation?
> It should help the other port solid for example bug 207375.

It's definitely ok. However, as I explained before, if you have added this test together with the implementation, then I would immediately know this test is to help OpenSSL's implementation.

> 
> > 
> > > 
> > > > And I believe if you don't use PKCS#7, the existing tests won't pass.
> > > 
> > > Yes, but we didn't have enough test cases to cover boundary values in PKCS#7
> > > now.
> > > 
> > > > > 
> > > > > For OpenSSL backend, we don't have to implement PKCS#7 by ourselves, but we
> > > > > need to setup enough memory for encrypted/decrypted data, which may contains
> > > > > PKCS#7 padding.
> > > > > 
> > > > > Actually we had a bug in calculating the encrypted data length when the
> > > > > length % 8 == 0, and the case was not covered by the current LayoutTest. bug
> > > > > 207176
> > > > 
> > > > Then maybe you should write a test in that patch to help you validate your
> > > > implementation instead of adding new tests like these here.
> > > 
> > > Sure, another option was adding these test cases in bug 207176.
> > > If it's better option than making another patch, then I can move the test
> > > case to bug 207176.
> > > 
> > > The reason why I made a separate bug is that attachment 389613 [details]
> > > affects the port, but bug 207176 only for OpenSSL backend one.
> > 
> > I think it is better to have tests along with the implementation within the
> > same commit. Then, when you look back the commit, you could reason the
> > decision you made from tests.
> > 
> > Given the other commit is landed, you can continue on this one. You should
> > combine them together next time. Otherwise, it is very hard for reviewers to
> > reason the relations between these two patches.
> 
> OK, thanks for letting me know about it. I'll do so from the next time.
Comment 17 Jiewen Tan 2020-02-10 11:11:23 PST
Comment on attachment 390245 [details]
patch

LGTM. r=me.
Comment 18 Tomoki Imai 2020-02-11 21:03:57 PST
Jiewen Tan,
Thanks for your review, I'll make sure that the LayoutTest and the implementation.
We'll also take a look what we can do for WPT in the future.
Comment 19 Tomoki Imai 2020-02-11 21:09:12 PST
(In reply to Tomoki Imai from comment #18)
> Jiewen Tan,
> Thanks for your review, I'll make sure that the LayoutTest and the
> implementation.

I'll make sure that the LayoutTest and the implementation combined to one patch.
Comment 20 WebKit Commit Bot 2020-02-11 21:41:26 PST
Comment on attachment 390245 [details]
patch

Clearing flags on attachment: 390245

Committed r256425: <https://trac.webkit.org/changeset/256425>
Comment 21 WebKit Commit Bot 2020-02-11 21:41:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2020-02-11 21:42:16 PST
<rdar://problem/59376171>