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 1 commit
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
Prev Previous commit
Clear the error queue and set to a malloc error
  • Loading branch information
@vcsjones
vcsjones committed Jan 15, 2022
commit 2a4fad5682c23625a7edfc88671ba1a57503c123
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
10 changes: 9 additions & 1 deletion 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 @@ -180,7 +180,10 @@ int32_t CryptoNative_GetPkcs8PrivateKeySize(EVP_PKEY* pkey, int32_t* p8size)
// 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".
unsigned long error = ERR_peek_error();

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.
Expand All @@ -190,6 +193,11 @@ int32_t CryptoNative_GetPkcs8PrivateKeySize(EVP_PKEY* pkey, int32_t* p8size)
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).

}

Expand Down