Skip to content

Fix OpenSSL 3 reporting an OutOfMemoryException for missing private key #63804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bartonjs merged 4 commits into from
Jan 18, 2022
Merged

Fix OpenSSL 3 reporting an OutOfMemoryException for missing private key #63804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e96a2a2
Fix OpenSSL 3 reporting an OutOfMemoryException for missing private key.
vcsjones Jan 14, 2022
e4afe32
Make sure p8size is always initialized
vcsjones Jan 14, 2022
824458c
Apply suggestions from code review
vcsjones Jan 14, 2022
2a4fad5
Clear the error queue and set to a malloc error
vcsjones Jan 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPkey.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,18 +105,28 @@ internal static unsafe SafeEvpPKeyHandle DecodePkcs8PrivateKey(
}

[GeneratedDllImport(Libraries.CryptoNative)]
private static partial int CryptoNative_GetPkcs8PrivateKeySize(IntPtr pkey);
private static partial int CryptoNative_GetPkcs8PrivateKeySize(IntPtr pkey, out int p8size);

private static int GetPkcs8PrivateKeySize(IntPtr pkey)
{
int ret = CryptoNative_GetPkcs8PrivateKeySize(pkey);
const int Success = 1;
const int Error = -1;
const int MissingPrivateKey = -2;

if (ret < 0)
int ret = CryptoNative_GetPkcs8PrivateKeySize(pkey, out int p8size);

switch (ret)
{
throw CreateOpenSslCryptographicException();
case Success:
return p8size;
case Error:
throw CreateOpenSslCryptographicException();
case MissingPrivateKey:
throw new CryptographicException(SR.Cryptography_CSP_NoPrivateKey);
default:
Debug.Fail($"Unexpected return '{ret}' value from {nameof(CryptoNative_GetPkcs8PrivateKeySize)}.");
throw new CryptographicException();
}

return ret;
}

[GeneratedDllImport(Libraries.CryptoNative)]
Expand Down
2 changes: 2 additions & 0 deletions src/native/libs/System.Security.Cryptography.Native/opensslshim.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ const EVP_CIPHER* EVP_chacha20_poly1305(void);
LEGACY_FUNCTION(ERR_load_crypto_strings) \
LIGHTUP_FUNCTION(ERR_new) \
REQUIRED_FUNCTION(ERR_peek_error) \
REQUIRED_FUNCTION(ERR_peek_error_line) \
REQUIRED_FUNCTION(ERR_peek_last_error) \
FALLBACK_FUNCTION(ERR_put_error) \
REQUIRED_FUNCTION(ERR_reason_error_string) \
Expand Down Expand Up @@ -728,6 +729,7 @@ FOR_ALL_OPENSSL_FUNCTIONS
#define ERR_load_crypto_strings ERR_load_crypto_strings_ptr
#define ERR_new ERR_new_ptr
#define ERR_peek_error ERR_peek_error_ptr
#define ERR_peek_error_line ERR_peek_error_line_ptr
#define ERR_peek_last_error ERR_peek_last_error_ptr
#define ERR_put_error ERR_put_error_ptr
#define ERR_reason_error_string ERR_reason_error_string_ptr
Expand Down
45 changes: 42 additions & 3 deletions src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,20 +152,59 @@ EVP_PKEY* CryptoNative_DecodePkcs8PrivateKey(const uint8_t* buf, int32_t len, in
return key;
}

int32_t CryptoNative_GetPkcs8PrivateKeySize(EVP_PKEY* pkey)
int32_t CryptoNative_GetPkcs8PrivateKeySize(EVP_PKEY* pkey, int32_t* p8size)
{
assert(pkey != NULL);
assert(p8size != NULL);

*p8size = 0;
ERR_clear_error();

PKCS8_PRIV_KEY_INFO* p8 = EVP_PKEY2PKCS8(pkey);

if (p8 == NULL)
{
// OpenSSL 1.1 and 3 have a behavioral change with EVP_PKEY2PKCS8
// with regard to handling EVP_PKEYs that do not contain a private key.
//
// In OpenSSL 1.1, it would always succeed, but the private parameters
// would be missing (thus making an invalid PKCS8 structure).
// Over in the managed side, we detect these invalid PKCS8 blobs and
// convert that to a "no private key" error.
//
// In OpenSSL 3, this now correctly errors, with the error
// ASN1_R_ILLEGAL_ZERO_CONTENT. We want to preserve allocation failures
// as OutOfMemoryException. So we peek at the error. If it's a malloc
// failure, -1 is returned to indcate "throw what is on the error queue".
// If the error is not a malloc failure, return -2 to mean "no private key".
// If OpenSSL ever changes the error to something more to explicitly mean
// "no private key" then we should test for that explicitly. Until then,
// we treat all errors, except a malloc error, to mean "no private key".

const char* file = NULL;
int line = 0;
unsigned long error = ERR_peek_error_line(&file, &line);

// If it's not a malloc failure, assume it's because the private key is
// missing.
if (ERR_GET_REASON(error) != ERR_R_MALLOC_FAILURE)
{
ERR_clear_error();
return -2;
}

// It is a malloc failure. Clear the error queue and set the error
// as a malloc error so it's the only error in the queue.
ERR_clear_error();
ERR_put_error(ERR_GET_LIB(error), 0, ERR_R_MALLOC_FAILURE, file, line);

return -1;
Copy link
Member

@bartonjs bartonjs Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the malloc error case here, do we need to manipulate the queue in any way? (e.g. is this checking the same end of the queue that we use as the basis of a throw?)

Copy link
Member Author

@vcsjones vcsjones Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. is this checking the same end of the queue that we use as the basis of a throw?

It's not, because that is what we are trying not to do. If we check the same end as the basis of the throw, then we are going to throw an OutOfMemoryException. We can't tell the difference between an OOM and "no private key". I though the next best thing we could do is check to see if the most-recent error thrown is an OOM. In that case, since we are peeking, it Weill just get thrown as-is.

Unfortunately I could not find a lot of flexibility in OpenSSL's error APIs. There is no way to examine the queue without consuming it, beyond peeking at the most recent item. Also, there is no way to determine the depth of the queue, as far as I could tell.

Copy link
Member

@bartonjs bartonjs Jan 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're talking about opposing conditions 😄

If the error isn't malloc, we return -2. The caller special cases that throw.

If the error is malloc, we return -1. The caller then throws an exception. I believe we want it to throw OOM, and I'm just making sure that the error reported by ERR_peek_error() is the one we're going to end up with in the throw... but I believe that we currently throw ERR_peek_last_error().

We could bridge that gap with something like

const char* file;
const char* line;
ERR_peek_error_line(&file, &line);
ERR_put_error(ERR_GET_LIB(error), ERR_GET_FUNC(error), ERR_GET_REASON(err), file, line);
return -1;

which would copy the head to the tail (until we revisit our exception model)

Copy link
Member Author

@vcsjones vcsjones Jan 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(until we revisit our exception model)

Ah. Okay. I was predicating my fix on that eventually happening. If we want to explicitly make sure we throw the OOM until then, that's fine.

Copy link
Member

@bartonjs bartonjs Jan 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. Hmm. I think we should be defensive now, just so we don't end up getting preview bug reports for exceptions that make no sense.

Alternatively, make the managed caller just explicitly throw OOM on -1, and TODO it against #63804

Copy link
Member Author

@vcsjones vcsjones Jan 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, ERR_put_error is deprecated in OpenSSL 3 and it seems silly to use it in an OpenSSL 3-only code path.

<sigh>.

Copy link
Member Author

@vcsjones vcsjones Jan 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we don't really need to preserve the line. That never appears in the OutOfMemoryException.

Copy link
Member Author

@vcsjones vcsjones Jan 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I noticed that we have our own ERR_put_error. I think that is what you're looking for? (ERR_GET_FUNC was removed in OpenSSL 3).

}

int ret = i2d_PKCS8_PRIV_KEY_INFO(p8, NULL);
*p8size = i2d_PKCS8_PRIV_KEY_INFO(p8, NULL);
PKCS8_PRIV_KEY_INFO_free(p8);
return ret;

return *p8size < 0 ? -1 : 1;
}

int32_t CryptoNative_EncodePkcs8PrivateKey(EVP_PKEY* pkey, uint8_t* buf)
Expand Down
8 changes: 6 additions & 2 deletions src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,13 @@ Requres a non-null buf, and len > 0.
PALEXPORT EVP_PKEY* CryptoNative_DecodePkcs8PrivateKey(const uint8_t* buf, int32_t len, int32_t algId);

/*
Reports the number of bytes rqeuired to encode an EVP_PKEY* as a Pkcs8PrivateKeyInfo, or a negative value on error.
Gets the number of bytes rqeuired to encode an EVP_PKEY* as a Pkcs8PrivateKeyInfo.

On success, 1 is returned and p8size contains the size of the Pkcs8PrivateKeyInfo.
On failure, -1 is used to indicate the openssl error queue contains the error.
On failure, -2 is used to indcate that the supplied EVP_PKEY* is possibly missing a private key.
*/
PALEXPORT int32_t CryptoNative_GetPkcs8PrivateKeySize(EVP_PKEY* pkey);
PALEXPORT int32_t CryptoNative_GetPkcs8PrivateKeySize(EVP_PKEY* pkey, int32_t* p8size);

/*
Encodes the EVP_PKEY* as a Pkcs8PrivateKeyInfo, writing the encoded value to buf.
Expand Down