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

Let ECDSA_verify return -1 for ASN1 parsing fail #1935

Closed

Conversation

samuel40791765
Copy link
Contributor

@samuel40791765 samuel40791765 commented Oct 18, 2024

Issues:

Addresses CryptoAlg-2697

Description of changes:

Ruby 3.1 has a test which expects a fatal failure when parsing arbitrary ASN1 data during signature verification. I pinned this down to Ruby's call to EVP_PKEY_verify which calls ECDSA_verify for EC operations. It turns out OpenSSL returns a -1 on any ASN1 parsing errors and only returns 0/1 to indicate signature verification failure/success. Ruby was dependent on the fatal error to determine whether to indicate an actual signature verification failure/success.
Tweaking the return code in ECDSA_verify in our code allows this test to pass.

Call-outs:

Also tweaked the function documentation, this aligns with OpenSSL's documented behavior: https://github.com/openssl/openssl/blob/f4c467452694e1211395d17c2c027d99c35ee1e1/include/openssl/ec.h#L1455-L1467

Testing:

N/A

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@samuel40791765 samuel40791765 requested a review from a team as a code owner October 18, 2024 22:19
@samuel40791765 samuel40791765 enabled auto-merge (squash) October 18, 2024 22:28
@samuel40791765 samuel40791765 changed the title let ECDSA_verify return -1 for ASN1 parsing fail Let ECDSA_verify return -1 for ASN1 parsing fail Oct 18, 2024
@samuel40791765 samuel40791765 enabled auto-merge (squash) October 18, 2024 22:30
Copy link

@ctz ctz left a comment

Choose a reason for hiding this comment

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

Please reconsider the reintroduction of badly-designed OpenSSL API practices. In this case, refer to CVE-2013-2944 for an example. There is no semantic difference between an ASN1 encoding error, an invalid signature, or indeed any other error verifying a signature.

I also think this may be introducing the same vulnerability into EVP_DigestVerify and callers, which uses EVP_PKEY_verify in boolean context.

@samuel40791765
Copy link
Contributor Author

Hi, really appreciate the feedback! I agree that this was a poorly designed OpenSSL API and the CVE does give more context on how this could this be a dangerous change. This could also break any other callers checking for !ECDSA_verify that thought this was OK to do with AWS-LC.

I hadn't given too much thought in tweaking this just yet, we've been drilling into these test failures and making tweaks for seemingly less harmful places. Given the potential push back on this, I think we can patch Ruby to get around this instead. I'll close this PR and leave it as a reference when we try to make a contribution upstream.

@samuel40791765 samuel40791765 deleted the fix-ruby-expect-error branch October 18, 2024 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants