Bug 170350 – [GCrypt] Implement CryptoKeyRSA::generatePair()

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GCrypt] Implement CryptoKeyRSA::generatePair()

[Zan Dobersek]

Created attachment 305988 [details]
Patch

[Michael Catanzaro]

Comment on attachment 305988 [details]
Patch

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

> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:89
> +    if (exponent.size() > 4) {
> +        if (std::any_of(exponent.begin(), exponent.end() - 4, [](uint8_t element) { return !!element; }))
> +            return std::nullopt;
> +    }

Can you explain this please?

> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:95
> +    uint32_t result = 0;
> +    for (size_t size = exponent.size(), i = std::min<size_t>(4, size); i > 0; --i) {
> +        result <<= 8;
> +        result += exponent[size - i];
> +    }

This too.

> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:102
> +    // libgcrypt doesn't report an error if the exponent is smaller than three or even.

Wow. :(

[Zan Dobersek]

Comment on attachment 305988 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:95
>> +    }
> 
> This too.

The public exponent is passed as a BigInteger, a Uint8Array in big-endian order. This helper function limits it to 32 bits, and checks that any larger value causes a failure.
https://w3c.github.io/webcrypto/Overview.html#dfn-BigInteger

The 32-bit limit was lifted from the Mac implementation, but we're not limited to it with libgcrypt. The only resulting difference would be how the exponent data is loaded into the genkey s-expression.

Let me test it out, it might be relatively easy to get this working.

[Zan Dobersek]

(In reply to Zan Dobersek from comment #3)
> Comment on attachment 305988 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305988&action=review
> 
> >> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:95
> >> +    }
> > 
> > This too.
> 
> The public exponent is passed as a BigInteger, a Uint8Array in big-endian
> order. This helper function limits it to 32 bits, and checks that any larger
> value causes a failure.
> https://w3c.github.io/webcrypto/Overview.html#dfn-BigInteger
> 
> The 32-bit limit was lifted from the Mac implementation, but we're not
> limited to it with libgcrypt. The only resulting difference would be how the
> exponent data is loaded into the genkey s-expression.
> 
> Let me test it out, it might be relatively easy to get this working.

No, this won't work.

Internally, libgcrypt scrapes the data from the s-expression and converts it to an unsigned long (8 bytes in size) via strtoul(). Feeding hex data into the s-expression won't work, since strtoul(), as used in libgcrypt, will default to a base-10 converion.

I would recommend sticking to converting the public exponent array into a 32-bit integer for now, adding a FIXME explaining we can stretch it out to 8 bytes. I can add that comment in the next iteration of the patch, or before landing it.

[Michael Catanzaro]

I don't think it requires any FIXME, because normal public exponents are very small (3, 5, 17). Nobody should be using something more than four bytes unless they're doing strange research, and it's not WebKit's job to support that.

[Michael Catanzaro]

Comment on attachment 305988 [details]
Patch

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

OK.

> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:88
> +    if (exponent.size() > 4) {
> +        if (std::any_of(exponent.begin(), exponent.end() - 4, [](uint8_t element) { return !!element; }))
> +            return std::nullopt;

I'd add a comment here: "ensure exponent is limited to four bytes"

[Zan Dobersek]

Comment on attachment 305988 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:88
>> +            return std::nullopt;
> 
> I'd add a comment here: "ensure exponent is limited to four bytes"

OK.

[Zan Dobersek]

Created attachment 306469 [details]
Patch for landing

[Zan Dobersek]

Created attachment 306475 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 306475 [details]
Patch for landing

Clearing flags on attachment: 306475

Committed r215085: <http://trac.webkit.org/changeset/215085>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.