Bug 170345 – [GCrypt] Implement CryptoKeyEC::keySizeInBits(), ::platformGeneratePair()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[GCrypt] Implement CryptoKeyEC::keySizeInBits(), ::platformGeneratePair()

[Zan Dobersek]

Created attachment 305976 [details]
Patch

[Michael Catanzaro]

Comment on attachment 305976 [details]
Patch

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

The remaining notImplemented() functions here are all TODO?

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:54
> +    case CryptoKeyEC::NamedCurve::P256:
> +        return "NIST P-256";
> +    case CryptoKeyEC::NamedCurve::P384:
> +        return "NIST P-384";

You're sure GCrypt doesn't provide constants for these? Really...?

Anyway, please add a comment here to check with relevant downstreams (e.g. https://src.fedoraproject.org/cgit/rpms/libgcrypt.git/tree/ecc-curves.c) before adding additional curves; we don't want this to suddenly start mysteriously failing in only a subset of distributions that have legal restrictions on which curves they have to remove from libgcrypt. (No problems with the curves you used here, fortunately.)

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:66
> -std::optional<CryptoKeyPair> CryptoKeyEC::platformGeneratePair(CryptoAlgorithmIdentifier, NamedCurve, bool, CryptoKeyUsageBitmap)
> +size_t CryptoKeyEC::keySizeInBits() const
>  {
> -    notImplemented();
> +    size_t size = curveSize(m_curve);

Now I know you didn't design this interface, but size_t is definitely not the right type for key size... it should be changed to unsigned (int) instead. Sane key size is way smaller than the size of an allocatable memory region, and it's not even bytes, but bits, so if we were hoping to allow absurd key sizes it'd be too small.

I don't really expect you to change that in this particular patch, but it'd be nice to do it in a follow-up.

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:74
> +    gcry_error_t error = gcry_sexp_build(&genkeySexp, nullptr, "(genkey(ecc(curve %s)))", curveName(curve));

Wow, I'd never heard of S-expressions before. I'll trust this is correct....

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:93
> +    auto publicKey = CryptoKeyEC::create(identifier, curve, CryptoKeyType::Public, publicKeySexp.release(), true, usages);
> +    auto privateKey = CryptoKeyEC::create(identifier, curve, CryptoKeyType::Private, privateKeySexp.release(), extractable, usages);

What does that extractable parameter do?

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:94
> +    return CryptoKeyPair { WTFMove(publicKey), WTFMove(privateKey) };

No WTFMove

[Zan Dobersek]

(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 305976 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305976&action=review
> 
> The remaining notImplemented() functions here are all TODO?
> 

Yes, everything is implemented in the patch in bug #133122. I'm uploading these incrementally to simplify the reviews.

[Jiewen Tan]

Comment on attachment 305976 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:94
>> +    return CryptoKeyPair { WTFMove(publicKey), WTFMove(privateKey) };
> 
> No WTFMove

May I ask why there is no need for WTFMove here?

[Zan Dobersek]

Comment on attachment 305976 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:54
>> +        return "NIST P-384";
> 
> You're sure GCrypt doesn't provide constants for these? Really...?
> 
> Anyway, please add a comment here to check with relevant downstreams (e.g. https://src.fedoraproject.org/cgit/rpms/libgcrypt.git/tree/ecc-curves.c) before adding additional curves; we don't want this to suddenly start mysteriously failing in only a subset of distributions that have legal restrictions on which curves they have to remove from libgcrypt. (No problems with the curves you used here, fortunately.)

Yes, really. But these are well-documented names:
https://www.gnupg.org/documentation/manuals/gcrypt/ECC-key-parameters.html#ECC-key-parameters

Interesting re: legal restrictions. Anyway, under the Web Crypto standard the only remaining EC we'd have to support is P-521, and that's supported even in Fedora.

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:74
>> +    gcry_error_t error = gcry_sexp_build(&genkeySexp, nullptr, "(genkey(ecc(curve %s)))", curveName(curve));
> 
> Wow, I'd never heard of S-expressions before. I'll trust this is correct....

For the most part these are well-documented. I also examined the libgcrypt test suite extensively.

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:93
>> +    auto privateKey = CryptoKeyEC::create(identifier, curve, CryptoKeyType::Private, privateKeySexp.release(), extractable, usages);
> 
> What does that extractable parameter do?

"... indicates whether or not the raw keying material may be exported by the application."
https://w3c.github.io/webcrypto/Overview.html#dfn-CryptoKey-extractable

In case of generateKey(), which ends up calling this method, `extractable` specifies whether the private key may be exported:
https://w3c.github.io/webcrypto/Overview.html#ecdh-operations

>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:94
>>> +    return CryptoKeyPair { WTFMove(publicKey), WTFMove(privateKey) };
>> 
>> No WTFMove
> 
> May I ask why there is no need for WTFMove here?

There is, publicKey and privateKey are Ref<CryptoKeyEC> objects.

[Michael Catanzaro]

(In reply to Jiewen Tan from comment #4)
> May I ask why there is no need for WTFMove here?

I'm wrong. :)

[Zan Dobersek]

Comment on attachment 305976 [details]
Patch

Clearing flags on attachment: 305976

Committed r214825: <http://trac.webkit.org/changeset/214825>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.