Bug 207174 – Add WebCrypto LayoutTests to check if PKCS#7 padding is correctly implemented in AES-CBC

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

[Tomoki Imai]

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

[Tomoki Imai]

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

[Jiewen Tan]

Not sure why this is needed? You have to implement PKCS#7 yourself?

[Tomoki Imai]

(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

[Tomoki Imai]

(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

[Jiewen Tan]

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

[Jiewen Tan]

Comment on attachment 389613 [details]
patch

r- for the reasons stated above.

[Jiewen Tan]

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

[Tomoki Imai]

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

[Jiewen Tan]

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

[Jiewen Tan]

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

[Jiewen Tan]

(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

[Tomoki Imai]

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

[Tomoki Imai]

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

[Tomoki Imai]

Created attachment 390245 [details]
patch

Made debug messages more expressive as suggested in comment 10

[Jiewen Tan]

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

[Jiewen Tan]

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

[Jiewen Tan]

Comment on attachment 390245 [details]
patch

LGTM. r=me.

[Tomoki Imai]

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.

[Tomoki Imai]

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

[WebKit Commit Bot]

Comment on attachment 390245 [details]
patch

Clearing flags on attachment: 390245

Committed r256425: <https://trac.webkit.org/changeset/256425>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59376171>