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
Tomoki Imai
2020-02-03 18:21:31 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
Not sure why this is needed? You have to implement PKCS#7 yourself? (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 (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 (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 on attachment 389613 [details]
patch
r- for the reasons stated above.
(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. (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. (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 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". (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 (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. 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 Created attachment 390245 [details] patch Made debug messages more expressive as suggested in comment 10 (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. (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 on attachment 390245 [details]
patch
LGTM. r=me.
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. (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 on attachment 390245 [details] patch Clearing flags on attachment: 390245 Committed r256425: <https://trac.webkit.org/changeset/256425> All reviewed patches have been landed. Closing bug. |