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 #79013

Merged
merged 14 commits into from
Dec 7, 2022

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Nov 30, 2022

This reverts the revert. Original PR.

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.

@akoeplinger can you please point me to a pipeline run that reproduces the failure you observed?

Fixes #78293

@ghost
Copy link

ghost commented Nov 30, 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

This reverts the revert. Original PR.

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.

@akoeplinger can you please point me to a pipeline run that reproduces the failure you observed?

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: -

@vcsjones
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@akoeplinger
Copy link
Member

@akoeplinger can you please point me to a pipeline run that reproduces the failure you observed?

Sorry for the late reply, the coreclr change you made should've triggered the job due to the condition in

but looks like it didn't? Can you try removing the condition there and see if that triggers it?

@akoeplinger
Copy link
Member

The log says

-- Found OpenSSL: /crossrootfs/x86/usr/lib/i386-linux-gnu/libcrypto.so (found version "1.0.2g")  

and from what I can find the additional key was introduced in 1.0.2k so that at least explains why we're not finding it. I think we get the old package somehow for the cross root.

@akoeplinger
Copy link
Member

I think I figured it out: in https://github.com/dotnet/dotnet-buildtools-prereqs-docker/blob/a5a8882abc169ebbd30e01ee58c1fbd578affa80/src/ubuntu/18.04/cross/x86-linux/hooks/pre-build#LL5C3-L5C3 we're passing linux to the script which builds the rootfs, but we should be passing bionic (all other architectures do this).

This means that the build-rootfs.sh script defaults to Ubuntu 16.04 xenial, which indeed only got 1.0.2g as the last openssl:

openssl (1.0.2g-1ubuntu4.19) [security]

I'll send a PR to fix the Docker container. This shouldn't have any impact on the actual product since we don't officially support Linux x86: #7335

akoeplinger added a commit to dotnet/dotnet-buildtools-prereqs-docker that referenced this pull request Dec 7, 2022
The `linux` argument doesn't exist in the arcade build-rootfs.sh script, we should be passing the Ubuntu codename. It defaulted to xenial which caused outdated packages to be in the crossrootfs, see dotnet/runtime#79013 (comment)
@akoeplinger
Copy link
Member

Btw. do we need to do something about the mismatching value that @bartonjs noticed in #78339 (comment) ?

@vcsjones
Copy link
Member Author

vcsjones commented Dec 7, 2022

@akoeplinger for what it's worth, I need to fix my PR to work against 1.0.2g (and prior) since 1.0.2 is still supported. I apparently looked at the latest 1.0.2 and not 1.0.2a.

akoeplinger added a commit to dotnet/dotnet-buildtools-prereqs-docker that referenced this pull request Dec 7, 2022
The `linux` argument doesn't exist in the arcade build-rootfs.sh script, we should be passing the Ubuntu codename. It defaulted to xenial which caused outdated packages to be in the crossrootfs, see dotnet/runtime#79013 (comment)
@akoeplinger
Copy link
Member

I pushed a change to use the staging Docker image tag which includes my change just to verify the issue is fixed, please feel free to revert it when you push your other changes.

@vcsjones
Copy link
Member Author

vcsjones commented Dec 7, 2022

@bartonjs okay. I settled on EVP_R_DECODE_ERROR since that one is close (enough IMO) for this particular corner case. It is present and has a value of 114 across all versions of OpenSSL.

@vcsjones vcsjones marked this pull request as ready for review December 7, 2022 16:17
@vcsjones vcsjones merged commit 9e557f2 into dotnet:main Dec 7, 2022
@vcsjones vcsjones deleted the openssl11-rsa-zero-mod branch December 7, 2022 18:34
@mmitche
Copy link
Member

mmitche commented Dec 7, 2022

@vcsjones
Copy link
Member Author

vcsjones commented Dec 7, 2022

@mmitche @MichaelSimons I have a draft change that I believe will fix this, just working on validating it locally first. Apologies for the trouble.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 7, 2023
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