Skip to content
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.

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

Handle zero value modulus for OpenSSL 1.1 #78339

Merged
merged 4 commits into from
Nov 29, 2022

Conversation

vcsjones
Copy link
Member

Fixes #78293

This PR introduces a check when importing an RSA public key that the modulus is not zero. For OpenSSL 1.1, the import succeeded and later fails at key usage time with a less-than-helpful error. This check brings consistency with other platforms, where a zero modulus fails at key import time.

@ghost
Copy link

ghost commented Nov 14, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #78293

This PR introduces a check when importing an RSA public key that the modulus is not zero. For OpenSSL 1.1, the import succeeded and later fails at key usage time with a less-than-helpful error. This check brings consistency with other platforms, where a zero modulus fails at key import time.

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: -

@vcsjones
Copy link
Member Author

@bartonjs this is what I ended up with as a fix for this for OpenSSL 1.x and would like your feedback on the overall approach.

BN_is_zero is a macro in OpenSSL 1.0. I think I shimmed this correctly but would appreciate extra eyes over that.

@vcsjones
Copy link
Member Author

I think I shimmed this correctly

Nope. Haha.

@vcsjones
Copy link
Member Author

@bartonjs Seems like Windows 7 CSP doesn't fail at import, it fails at key usage. Given that

  1. This only happens with a legacy implementation RSACryptoServiceProvider, not RSACng or RSA.Create
  2. Windows 7 is very near EOL.

I opted to skip the test for Windows 7 instead of fixing RSACryptoServiceProvider. I'm not strongly opposed to fix it, but it would be adding Windows 7 specific code for an OS that is going to be EOL by the time .NET 8 is released.

@vcsjones vcsjones marked this pull request as ready for review November 15, 2022 15:21
@vcsjones vcsjones requested a review from bartonjs November 29, 2022 17:34
@bartonjs bartonjs merged commit fe49b0a into dotnet:main Nov 29, 2022
@vcsjones vcsjones deleted the openssl11-rsa-zero-mod branch November 29, 2022 17:41
@akoeplinger
Copy link
Member

@vcsjones this seems to cause an error on the Linux x86 build:

 /__w/1/s/src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c:114:47: error: use of undeclared identifier 'EVP_R_INVALID_KEY'
                ERR_put_error(ERR_LIB_EVP, 0, EVP_R_INVALID_KEY, __FILE__, __LINE__);

I guess we have an older OpenSSL in that image? it uses mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-18.04-cross-x86-linux

@vcsjones
Copy link
Member Author

@akoeplinger Looking.

@vcsjones
Copy link
Member Author

docker run -it mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-18.04-cross-x86-linux bash
root@9c730a75779d:/# openssl version
OpenSSL 1.1.1 11 Sep 2018

This is... confusing.

And it was definitely present in OpenSSL 1.1.1:

https://github.com/openssl/openssl/blob/OpenSSL_1_1_1a/include/openssl/evperr.h#L153

@bartonjs Any thoughts? It might be a while before I can really dig in to this. I suppose we can revert if this is blocking the build while I try to figure this out.

@bartonjs
Copy link
Member

It's present in 1.1.0: https://github.com/openssl/openssl/blob/OpenSSL_1_1_0-stable/include/openssl/evp.h#L1559 (# define EVP_R_INVALID_KEY 163)

And 1.1.1: https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/include/openssl/evperr.h#L162 (still 163)

And 3.0: https://github.com/openssl/openssl/blob/openssl-3.0/include/openssl/evperr.h#L66

It's also present in 1.0.2: https://github.com/openssl/openssl/blob/OpenSSL_1_0_2-stable/crypto/evp/evp.h#L1587, though back then it was 171.

So... I'm fairly well confused. Unless somehow we're building against 1.0.1 headers somewhere...

@vcsjones vcsjones restored the openssl11-rsa-zero-mod branch November 30, 2022 00:33
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RSA.VerifyData terminates with "Out of memory." on Linux when passing an all-zero modulus
4 participants