Bug 170350

Created attachment 305988 [details] Patch

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 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.

(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.

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 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 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.

Created attachment 306469 [details] Patch for landing

Created attachment 306475 [details] Patch for landing

Comment on attachment 306475 [details] Patch for landing Clearing flags on attachment: 306475 Committed r215085: <http://trac.webkit.org/changeset/215085>

All reviewed patches have been landed. Closing bug.