Bug 163125 – [GTK] Use libgcrypt instead of GnuTLS for CryptoDigest and SubtleCrypto HMAC implementation

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Olivier Blin]

As discussed in bug 162913, libgcrypt should be used instead of GnuTLS for CryptoDigest and SubtleCrypto HMAC implementation.
This removes concerns with licensing of gnutls dependencies (nettle and gmp).

[Olivier Blin]

Created attachment 290940 [details]
Patch

[Olivier Blin]

(In reply to comment #1)
> Created attachment 290940 [details]
> Patch

This does not remove gnutls files for now, since I have only migrated the GTK port to use libgcrypt.
If the EFL port wants to make the move as well, we can move the stubs from the gnutls folder to the gcrypt folder.

[Konstantin Tokarev]

Comment on attachment 290940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290940&action=review

> Source/WebCore/platform/crypto/gcrypt/CryptoDigestGCrypt.cpp:92
> +    Vector<uint8_t> result;

You can create Vector of proper size right away, as you do for HMAC

[Olivier Blin]

Created attachment 290946 [details]
Patch

Address comment 3

[Konstantin Tokarev]

I think you should also add gcrypt to install-dependencies (or jhbuild)

[Olivier Blin]

Created attachment 290961 [details]
Patch

Update gtk/install-dependencies to reflect that libgcrypt is needed for WebKitGTK+ build, and gnutls for jhbuild only

[Michael Catanzaro]

Comment on attachment 290961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290961&action=review

Thanks for working on this! I'm giving r+ but with several things that need fixed before committing:

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:3
> + * Copyright (C) 2014 Igalia S.L. All rights reserved.
> + * Copyright (C) 2016 SoftAtHome. All rights reserved.

Please remove the "All rights reserved" since that contradicts the license right below.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:81
> +    UNUSED_PARAM(failureCallback);
> +    int algorithm = getGCryptDigestAlgorithm(parameters.hash);
> +    if (algorithm == GCRY_MAC_NONE) {
> +        ec = NOT_SUPPORTED_ERR;
> +        return;

You don't need to call failureCallback when the function fails...?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:91
> +    UNUSED_PARAM(failureCallback);

Ditto?

> Source/WebCore/platform/crypto/gcrypt/CryptoDigestGCrypt.cpp:3
> + * Copyright (C) 2014 Igalia S.L. All rights reserved.
> + * Copyright (C) 2016 SoftAtHome. All rights reserved.

Ditto.

> Source/WebCore/platform/crypto/gcrypt/CryptoDigestGCrypt.cpp:73
> +    switch (algorithm) {
> +    case CryptoDigest::Algorithm::SHA_1: {
> +        gcryptAlgorithm = GCRY_MD_SHA1;
> +        break;
> +    }
> +    case CryptoDigest::Algorithm::SHA_224: {
> +        gcryptAlgorithm = GCRY_MD_SHA224;
> +        break;
> +    }
> +    case CryptoDigest::Algorithm::SHA_256: {
> +        gcryptAlgorithm = GCRY_MD_SHA256;
> +        break;
> +    }
> +    case CryptoDigest::Algorithm::SHA_384: {
> +        gcryptAlgorithm = GCRY_MD_SHA384;
> +        break;
> +    }
> +    case CryptoDigest::Algorithm::SHA_512: {
> +        gcryptAlgorithm = GCRY_MD_SHA512;
> +        break;
> +    }
> +    }

You don't need the extra braces around the cases here, since you're not declaring any variables in the cases.

> Source/cmake/FindGCrypt.cmake:23
> +    # set(GCRYPT_FOUND TRUE)

Well that's not right, did you comment it out for testing? Or was it broken already when you copied it from wherever its upstream is?

> Source/cmake/FindGCrypt.cmake:70
> +endif (GCRYPT_LIBRARIES AND GCRYPT_INCLUDE_DIRS)

WebKit CMake style is endif ()

> Source/cmake/OptionsGTK.cmake:35
> +find_package(GCrypt REQUIRED)

All the functions you used were available in the first release of GCrypt?

> Tools/ChangeLog:8
> +        * gtk/install-dependencies: List libgcrypt for WebKitGTK+ build, and gnutls for jhbuild only.

Er, why keep GnuTLS? You've completely removed the dependency, so you can get rid of it, right?

[Olivier Blin]

(In reply to comment #7)
> Comment on attachment 290961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290961&action=review
> 
> Thanks for working on this! I'm giving r+ but with several things that need
> fixed before committing:
> 
> > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:3
> > + * Copyright (C) 2014 Igalia S.L. All rights reserved.
> > + * Copyright (C) 2016 SoftAtHome. All rights reserved.
> 
> Please remove the "All rights reserved" since that contradicts the license
> right below.

This is actually copied from CryptoAlgorithmHMACGnuTLS.cpp, from Igalia.

Is the following fine?

+ * Copyright (C) 2014 Igalia S.L.
+ * Copyright (C) 2016 SoftAtHome

> > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:81
> > +    UNUSED_PARAM(failureCallback);
> > +    int algorithm = getGCryptDigestAlgorithm(parameters.hash);
> > +    if (algorithm == GCRY_MAC_NONE) {
> > +        ec = NOT_SUPPORTED_ERR;
> > +        return;
> 
> You don't need to call failureCallback when the function fails...?

I wondered about this too, but this is also copied from the gnutls flavor.
The Mac flavor is also ignoring the failure callback.

I can use failureCallback, not sure about unwanted side-effects.

> > Source/WebCore/platform/crypto/gcrypt/CryptoDigestGCrypt.cpp:73
> > +    switch (algorithm) {
> > +    case CryptoDigest::Algorithm::SHA_1: {
> > +        gcryptAlgorithm = GCRY_MD_SHA1;
> > +        break;
> > +    }
> > +    case CryptoDigest::Algorithm::SHA_224: {
> > +        gcryptAlgorithm = GCRY_MD_SHA224;
> > +        break;
> > +    }
> > +    case CryptoDigest::Algorithm::SHA_256: {
> > +        gcryptAlgorithm = GCRY_MD_SHA256;
> > +        break;
> > +    }
> > +    case CryptoDigest::Algorithm::SHA_384: {
> > +        gcryptAlgorithm = GCRY_MD_SHA384;
> > +        break;
> > +    }
> > +    case CryptoDigest::Algorithm::SHA_512: {
> > +        gcryptAlgorithm = GCRY_MD_SHA512;
> > +        break;
> > +    }
> > +    }
> 
> You don't need the extra braces around the cases here, since you're not
> declaring any variables in the cases.

Sure, I kept the existing style from the gnutls file, but I will fix.

> > Source/cmake/FindGCrypt.cmake:23
> > +    # set(GCRYPT_FOUND TRUE)
> 
> Well that's not right, did you comment it out for testing? Or was it broken
> already when you copied it from wherever its upstream is?

I copied it from https://github.com/substack/libssh/blob/master/cmake/Modules/FindGCrypt.cmake , but it seems to be an old copy.
The real upstream of libssh is this one, with an additional "cmake: Update GCrypt module" commit:
https://git.libssh.org/projects/libssh.git/tree/cmake/Modules/FindGCrypt.cmake

Does it look better?

It still has a "GCRYPT_FOUND" comment on top, while the file never defines it.

Another alternative is this one:
https://quickgit.kde.org/?p=kwallet.git&a=blob&h=20053f1a790c99d5d75c495ed7c3255867d4efb7&hb=23344979ed047053256e21638338ef67a1a937ff&f=cmake%2FFindLibGcrypt.cmake

> > Source/cmake/FindGCrypt.cmake:70
> > +endif (GCRYPT_LIBRARIES AND GCRYPT_INCLUDE_DIRS)
> 
> WebKit CMake style is endif ()

Ok

> > Source/cmake/OptionsGTK.cmake:35
> > +find_package(GCrypt REQUIRED)
> 
> All the functions you used were available in the first release of GCrypt?
> 
> > Tools/ChangeLog:8
> > +        * gtk/install-dep endencies: List libgcrypt for WebKitGTK+ build, and gnutls for jhbuild only.
> 
> Er, why keep GnuTLS? You've completely removed the dependency, so you can
> get rid of it, right?

For glib-networking in jhbuild

[Olivier Blin]

(In reply to comment #7)
> Comment on attachment 290961 [details]
>
> > Source/cmake/OptionsGTK.cmake:35
> > +find_package(GCrypt REQUIRED)
> 
> All the functions you used were available in the first release of GCrypt?

Good point.
The HMAC APIs have been added in 1.6.0:
http://git.gnupg.org/cgi-bin/gitweb.cgi?p=libgcrypt.git;a=commit;h=fcd6da37d55f248d3558ee0ff385b41b866e7ded

Digest APIs are way older, from 1999:
http://git.gnupg.org/cgi-bin/gitweb.cgi?p=libgcrypt.git;a=commit;h=98f4d26ffe97e99d9f9bd5dc58d9f511d9e64514

I'll require 1.6.0 then, released in December 2013

[Michael Catanzaro]

(In reply to comment #8)
> This is actually copied from CryptoAlgorithmHMACGnuTLS.cpp, from Igalia.
> 
> Is the following fine?
> 
> + * Copyright (C) 2014 Igalia S.L.
> + * Copyright (C) 2016 SoftAtHome

Yeah, that's fine.

A lot of files have those bogus notices unfortunately. :( I guess Apple's lawyers would complain if they thought it was a problem, but let's avoid adding it in new places.

> I wondered about this too, but this is also copied from the gnutls flavor.
> The Mac flavor is also ignoring the failure callback.
> 
> I can use failureCallback, not sure about unwanted side-effects.

Hm, if all ports are ignoring the failure callback, that's a little concerning as that indicates the failure cases must never be tested... anyway, that's a preexisting issue; seems safest to ignore it for now.
  
> It still has a "GCRYPT_FOUND" comment on top, while the file never defines
> it.

Ew, good catch; I didn't notice.

> Another alternative is this one:
> https://quickgit.kde.org/?p=kwallet.
> git&a=blob&h=20053f1a790c99d5d75c495ed7c3255867d4efb7&hb=23344979ed047053256e
> 21638338ef67a1a937ff&f=cmake%2FFindLibGcrypt.cmake

I would use the KDE one.

[Carlos Garcia Campos]

Comment on attachment 290961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290961&action=review

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:40
> +static int getGCryptDigestAlgorithm(CryptoAlgorithmIdentifier hashFunction)

Don't use "get" here. You could use cryptoAlgorithmIdentifierToGCryptDigestAlgorithm or something similar, or gcryptDigestAlgorithmForCryptoAlgorithmIdentifier

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:66
> +    gcry_mac_open(&hd, algorithm, 0, 0);

0, nullptr

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:69
> +    gcry_mac_read(hd, result.data(), &digestLength);

Don't you need to handle the return value of digestLength? Can it really be different to result.size()? I guess it can't be bigger than value returned by gcry_mac_get_algo_maclen, but can it be smaller? If it should be the same size I would add an assert, otherwise you would need to resize the vector here.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:72
> +    return result;

Most of the gcry functions used above return a gcry_error_t that is not handled at all. We should call the failureCallback in platformSign if any of those fail.

[Carlos Alberto Lopez Perez]

Comment on attachment 290961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290961&action=review

> Tools/gtk/install-dependencies:109
>          libfaad-dev \
> +        libgcrypt11-dev \
>          $(aptIfElse libgeoclue-2-dev libgeoclue-dev) \

On Debian/Ubuntu libgcrypt11-dev is a transitional dummy package to ease the migration from the old libgcrypt11-dev to libgcrypt20-dev. So I would use here:
$(aptIfElse libgcrypt20-dev libgcrypt11-dev) \

[Olivier Blin]

(In reply to comment #11)
> Comment on attachment 290961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290961&action=review
> 
> > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:40
> > +static int getGCryptDigestAlgorithm(CryptoAlgorithmIdentifier hashFunction)
> 
> Don't use "get" here. You could use
> cryptoAlgorithmIdentifierToGCryptDigestAlgorithm or something similar, or
> gcryptDigestAlgorithmForCryptoAlgorithmIdentifier

The same naming is using for the Mac and gnutls backends.

> > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:66
> > +    gcry_mac_open(&hd, algorithm, 0, 0);
> 
> 0, nullptr

Ok

> > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:69
> > +    gcry_mac_read(hd, result.data(), &digestLength);
> 
> Don't you need to handle the return value of digestLength? Can it really be
> different to result.size()? I guess it can't be bigger than value returned
> by gcry_mac_get_algo_maclen, but can it be smaller? If it should be the same
> size I would add an assert, otherwise you would need to resize the vector
> here.

It can be smaller according to the API:
https://gnupg.org/documentation/manuals/gcrypt/Working-with-MAC-algorithms.html#Working-with-MAC-algorithms

I will resize it.

> > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:72
> > +    return result;
> 
> Most of the gcry functions used above return a gcry_error_t that is not
> handled at all. We should call the failureCallback in platformSign if any of
> those fail.

I did not find code that made use of the failureCallback, besides CryptoAlgorithmHMAC::generateKey
I'll upload a version which handles gcry failures, but it makes the layout tests hang on failure.

(In reply to comment #12)
> Comment on attachment 290961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290961&action=review
> 
> > Tools/gtk/install-dependencies:109
> >          libfaad-dev \
> > +        libgcrypt11-dev \
> >          $(aptIfElse libgeoclue-2-dev libgeoclue-dev) \
> 
> On Debian/Ubuntu libgcrypt11-dev is a transitional dummy package to ease the
> migration from the old libgcrypt11-dev to libgcrypt20-dev. So I would use
> here:
> $(aptIfElse libgcrypt20-dev libgcrypt11-dev) \

Ok

[Olivier Blin]

(In reply to comment #10)
> > Another alternative is this one:
> > https://quickgit.kde.org/?p=kwallet.
> > git&a=blob&h=20053f1a790c99d5d75c495ed7c3255867d4efb7&hb=23344979ed047053256e
> > 21638338ef67a1a937ff&f=cmake%2FFindLibGcrypt.cmake
> 
> I would use the KDE one.

Source/cmake/FindLibGcrypt.cmake has been added for bug 162928 since then.

[Olivier Blin]

Created attachment 293685 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 293685 [details]
Patch

Clearing flags on attachment: 293685

Committed r208297: <http://trac.webkit.org/changeset/208297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.