Bug 239858

Created attachment 458535 [details]
Patch

(In reply to Don Olmstead from comment #2)
> Created attachment 458535 [details]
> Patch

LGTM besides style warings.

Created attachment 458613 [details]
Patch

<rdar://problem/92805748>

Created attachment 459081 [details]
Patch

Comment on attachment 459081 [details]
Patch

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

> Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:84
> +    auto n = convertToBigNumber(keyData.modulus());

I think it should be wrapped by `BIGNUMPtr` if it's allocated. More to say, if convertToBigNumber always allocate a new memory, then it can return BIGNUMPtr wrapped result.

> Source/WebCore/crypto/openssl/OpenSSLUtilities.h:47
> +BIGNUM* convertToBigNumber(const Vector<uint8_t>& bytes, BIGNUM* ret = nullptr);

If all usage doesn't pass second argument, why can't we remove the second argument?

Comment on attachment 459081 [details]
Patch

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

>> Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:84
>> +    auto n = convertToBigNumber(keyData.modulus());
> 
> I think it should be wrapped by `BIGNUMPtr` if it's allocated. More to say, if convertToBigNumber always allocate a new memory, then it can return BIGNUMPtr wrapped result.

When the associated `set` is called OpenSSL then takes over the BIGNUM so we wouldn't want it accidentally deleted.

>> Source/WebCore/crypto/openssl/OpenSSLUtilities.h:47
>> +BIGNUM* convertToBigNumber(const Vector<uint8_t>& bytes, BIGNUM* ret = nullptr);
> 
> If all usage doesn't pass second argument, why can't we remove the second argument?

I'm fine with removing it. Can add back later if its needed.

Comment on attachment 459081 [details]
Patch

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

> Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:86
> +    if (!n || !e)

Then if n is assigned and e was failed, here comes a memory leak.

Created attachment 459139 [details]
Patch

Comment on attachment 459139 [details]
Patch

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

Always I feel it's difficult to use OpenSSL in C++ code.

> Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:98
> +        if (!RSA_set0_factors(rsa.get(), p.release(), q.release()))

Do they release the parameter on failure?

> Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:110
> +        if (!RSA_set0_crt_params(rsa.get(), dmp1.release(), dmq1.release(), iqmp.release()))

Ditto.

> Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:114
> +    if (!RSA_set0_key(rsa.get(), n.release(), e.release(), d.release()))

Ditto.

Created attachment 459172 [details]
Patch

Good catch Basuke! I made it so the release happens after success and I called out the ownership transfer.

Comment on attachment 459172 [details]
Patch

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

For memory perspective, LGTM. Some extra note was added.

> Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:45
> +    return RSA_bits(rsa) * 8;

Does this returns same value? The previous one seems bits. It sounds RSA_bits() already returns bits, not byte. I'm not confident about this, but function names sounds confusing so that I'm confirming it.

> Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:95
> +    e.release();

How about wrapping with scope instead of releasing explicitly? It's a bit noisy to see explicit release for smart pointers. Also scoping is a standard technique to be used to narrow the effectiveness of variables, not only for smart pointers.

Comment on attachment 459172 [details]
Patch

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

>> Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:45
>> +    return RSA_bits(rsa) * 8;
> 
> Does this returns same value? The previous one seems bits. It sounds RSA_bits() already returns bits, not byte. I'm not confident about this, but function names sounds confusing so that I'm confirming it.

You're right it should `RSA_size`

From LibreSSL's implementation

int
RSA_bits(const RSA *r)
{
	return BN_num_bits(r->n);
}

vs

int
RSA_size(const RSA *r)
{
	return BN_num_bytes(r->n);
}

>> Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:95
>> +    e.release();
> 
> How about wrapping with scope instead of releasing explicitly? It's a bit noisy to see explicit release for smart pointers. Also scoping is a standard technique to be used to narrow the effectiveness of variables, not only for smart pointers.

From https://en.cppreference.com/w/cpp/memory/unique_ptr/release

Releases the ownership of the managed object, if any.

get() returns nullptr after the call.

The caller is responsible for deleting the object.

In these cases ownership is transferred to OpenSSL so we don't want the std::unique_ptr to try and delete it and we can accomplish this by using release.

Created attachment 459175 [details]
Patch

Got it. I didn't get the point of transferring. Then it makes sense.

r=me

I've yet to look into the issue but with the patch (id=459175) relevant crypt/subtle tests come to fail.

Created attachment 459257 [details]
Patch

Fixed the crash, was retrieving the wrong values in algorithm.

Created attachment 459259 [details]
Patch

Comment on attachment 459259 [details]
Patch

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

> Source/WebCore/crypto/openssl/OpenSSLUtilities.h:46
> +BIGNUMPtr convertToBigNumber(const Vector<uint8_t>& bytes);

I'm against to force programmers to use BIGNUMPtr returned by convertToBigNumber() because this force programmers to write ownership transfer code in normal codepaths (otherwise use-after-free would happen). This seems more error-prone to me than memory leak in error cases of set0, which never happens with the current libressl implementation.

We may want to have a wrapper template function for set0 family that releases BIGNUMs if set0 fails.

Pull request: https://github.com/webkit/webkit/pull/1023

Committed r295168 (251252@main): <https://commits.webkit.org/251252@main>

Reviewed commits have been landed. Closing PR #1023 and removing active labels.