[Webkit-unassigned] [Bug 207174] Add WebCrypto LayoutTests to check if PKCS#7 padding is correctly implemented in AES-CBC

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 10 03:56:44 PST 2020


https://bugs.webkit.org/show_bug.cgi?id=207174

--- Comment #12 from Tomoki Imai <tomoki.imai at sony.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200210/f3653d21/attachment.htm>


More information about the webkit-unassigned mailing list