[webkit-reviews] review granted: [Bug 170345] [GCrypt] Implement CryptoKeyEC::keySizeInBits(), ::platformGeneratePair() : [Attachment 305976] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 170345: [GCrypt] Implement CryptoKeyEC::keySizeInBits(),
::platformGeneratePair()
https://bugs.webkit.org/show_bug.cgi?id=170345

Attachment 305976: Patch

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




--- Comment #2 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 305976
  --> https://bugs.webkit.org/attachment.cgi?id=305976
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