Bug 171074

Created attachment 307634 [details] Patch

Comment on attachment 307634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307634&action=review Thanks. Would be good to ask Jiewen to look at this one, too. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHKDFGCrypt.cpp:42 > +static std::optional<int> macAlgorithmForHashFunction(CryptoAlgorithmIdentifier identifier) This really isn't needed anywhere else, or isn't ever going to be needed anywhere else? Seems like it'd fit better in Utilities.[cpp,h]?

Comment on attachment 307634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307634&action=review > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHKDFGCrypt.cpp:86 > + error = gcry_mac_setkey(handle, salt.data(), salt.size()); Does gcry_mac_setkey handle the case where salt is an empty vector? > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHKDFGCrypt.cpp:147 > + lastBlock.resize(blockSize); Feel weird that we need to resize every time. Even though only the first invocation takes effect, it still feels weird to write it this way.

Comment on attachment 307634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307634&action=review >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHKDFGCrypt.cpp:42 >> +static std::optional<int> macAlgorithmForHashFunction(CryptoAlgorithmIdentifier identifier) > > This really isn't needed anywhere else, or isn't ever going to be needed anywhere else? Seems like it'd fit better in Utilities.[cpp,h]? Yes, this should all be gathered in the Utilities header. I would maybe prefer doing that in later cleanups. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHKDFGCrypt.cpp:86 >> + error = gcry_mac_setkey(handle, salt.data(), salt.size()); > > Does gcry_mac_setkey handle the case where salt is an empty vector? It does, but on a second thought I'd prefer being explicit here and use a 0-filled array of bytes in case the salt Vector is empty. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHKDFGCrypt.cpp:147 >> + lastBlock.resize(blockSize); > > Feel weird that we need to resize every time. Even though only the first invocation takes effect, it still feels weird to write it this way. OK. It's easy to work around -- the lastBlock should just not be appended to the block data when in the first iteration.

Created attachment 307706 [details] Patch

Comment on attachment 307706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307706&action=review Looks good otherwise. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHKDFGCrypt.cpp:140 > + if (!i) Should it be just if (i) ?

Comment on attachment 307706 [details] Patch Forgot to mention. I suggest you should add some false tests for this gcrypt specific implementation as you implement HKDF by yourself.

(In reply to Jiewen Tan from comment #7) > Comment on attachment 307706 [details] > Patch > > Forgot to mention. I suggest you should add some false tests for this > gcrypt specific implementation as you implement HKDF by yourself. Just tests include false and pass ones.

Comment on attachment 307706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307706&action=review >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHKDFGCrypt.cpp:140 >> + if (!i) > > Should it be just if (i) ? Correct, it's a typo.

Created attachment 307931 [details] Patch for landing

(In reply to Jiewen Tan from comment #7) > Comment on attachment 307706 [details] > Patch > > Forgot to mention. I suggest you should add some false tests for this > gcrypt specific implementation as you implement HKDF by yourself. I can put together a length limit test that checks the algorithm properly rejects lengths that go over the 255 * MAC-length limit. Is there anything else you'd suggest testing around this HKDF implementation?

(In reply to Zan Dobersek from comment #11) > (In reply to Jiewen Tan from comment #7) > > Comment on attachment 307706 [details] > > Patch > > > > Forgot to mention. I suggest you should add some false tests for this > > gcrypt specific implementation as you implement HKDF by yourself. > > I can put together a length limit test that checks the algorithm properly > rejects lengths that go over the 255 * MAC-length limit. > > Is there anything else you'd suggest testing around this HKDF implementation? I would suggest checking cases: length >|=|< macLength for positive tests.

Created attachment 308379 [details] Patch

(In reply to Zan Dobersek from comment #13) > Created attachment 308379 [details] > Patch This is the reviewed patch, plus a layout test that tests various length values. Please review.

Comment on attachment 308379 [details] Patch Looks good. I suggest you could file a bug to replace this customized HKDF implementation with gcrypt one once it is available. We should not implement crypto by ourselves if we don't have to.

Comment on attachment 308379 [details] Patch rs=me

(In reply to Jiewen Tan from comment #15) > Comment on attachment 308379 [details] > Patch > > Looks good. I suggest you could file a bug to replace this customized HKDF > implementation with gcrypt one once it is available. We should not implement > crypto by ourselves if we don't have to. I'll add a comment linking to bug #171536.

Created attachment 308801 [details] Patch for landing

Committed r216061: <http://trac.webkit.org/changeset/216061>