Bug 170350 - [GCrypt] Implement CryptoKeyRSA::generatePair()
Summary: [GCrypt] Implement CryptoKeyRSA::generatePair()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 133122
  Show dependency treegraph
 
Reported: 2017-03-31 11:30 PDT by Zan Dobersek
Modified: 2017-04-07 00:35 PDT (History)
3 users (show)

See Also:


Attachments
Patch (11.48 KB, patch)
2017-03-31 11:49 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (11.58 KB, patch)
2017-04-06 22:47 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (10.32 KB, patch)
2017-04-07 00:17 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-03-31 11:30:19 PDT
[GCrypt] Implement CryptoKeyRSA::generatePair()
Comment 1 Zan Dobersek 2017-03-31 11:49:26 PDT
Created attachment 305988 [details]
Patch
Comment 2 Michael Catanzaro 2017-04-01 18:50:16 PDT
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. :(
Comment 3 Zan Dobersek 2017-04-03 11:34:15 PDT
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.
Comment 4 Zan Dobersek 2017-04-03 12:34:49 PDT
(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.
Comment 5 Michael Catanzaro 2017-04-06 08:14:12 PDT
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.
Comment 6 Michael Catanzaro 2017-04-06 08:18:00 PDT
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"
Comment 7 Zan Dobersek 2017-04-06 22:31:08 PDT
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.
Comment 8 Zan Dobersek 2017-04-06 22:47:19 PDT
Created attachment 306469 [details]
Patch for landing
Comment 9 Zan Dobersek 2017-04-07 00:17:06 PDT
Created attachment 306475 [details]
Patch for landing
Comment 10 Zan Dobersek 2017-04-07 00:35:24 PDT
Comment on attachment 306475 [details]
Patch for landing

Clearing flags on attachment: 306475

Committed r215085: <http://trac.webkit.org/changeset/215085>
Comment 11 Zan Dobersek 2017-04-07 00:35:28 PDT
All reviewed patches have been landed.  Closing bug.