Bug 171074 – [GCrypt] HKDF bit derivation support

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GCrypt] HKDF bit derivation support

[Zan Dobersek]

Created attachment 307634 [details]
Patch

[Michael Catanzaro]

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]?

[Jiewen Tan]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 307706 [details]
Patch

[Jiewen Tan]

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) ?

[Jiewen Tan]

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.

[Jiewen Tan]

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

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 307931 [details]
Patch for landing

[Zan Dobersek]

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

[Jiewen Tan]

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

[Zan Dobersek]

Created attachment 308379 [details]
Patch

[Zan Dobersek]

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

[Jiewen Tan]

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.

[Michael Catanzaro]

Comment on attachment 308379 [details]
Patch

rs=me

[Zan Dobersek]

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

[Zan Dobersek]

Created attachment 308801 [details]
Patch for landing

[Zan Dobersek]

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