[OpenSSL] Treat types as opaque

[Don Olmstead]

OpenSSL types should not be accessed directly. Instead use functions to access and modify instances.

LibreSSL made the types completely opaque in 3.5.x release so WebKit won't build with an updated library.

[Don Olmstead]

Created attachment 458533 [details]
Patch

[Don Olmstead]

Created attachment 458535 [details]
Patch

[Yoshiaki Jitsukawa]

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

LGTM besides style warings.

[Don Olmstead]

Created attachment 458613 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/92805748>

[Don Olmstead]

Created attachment 459081 [details]
Patch

[Basuke Suzuki]

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?

[Don Olmstead]

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.

[Basuke Suzuki]

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.

[Don Olmstead]

Created attachment 459139 [details]
Patch

[Basuke Suzuki]

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.

[Don Olmstead]

Created attachment 459172 [details]
Patch

[Don Olmstead]

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

[Basuke Suzuki]

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.

[Don Olmstead]

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.

[Don Olmstead]

Created attachment 459175 [details]
Patch

[Basuke Suzuki]

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

r=me

[Yoshiaki Jitsukawa]

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

[Don Olmstead]

Created attachment 459257 [details]
Patch

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

[Don Olmstead]

Created attachment 459259 [details]
Patch

[Yoshiaki Jitsukawa]

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.

[Yoshiaki Jitsukawa]

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

[Don Olmstead]

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

[EWS]

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

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