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.

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

Jump to bottom

Added signature length check for RSA PSS signature verification. #11310

Closed
wants to merge 1 commit into from
Closed

Added signature length check for RSA PSS signature verification. #11310

wants to merge 1 commit into from

Conversation

avgrygoriev
Copy link

@avgrygoriev avgrygoriev commented Mar 11, 2020 •
edited

Add length check for RSA PKCS1 Decryption and Signature Verification

According to RFC8017
https://tools.ietf.org/html/rfc8017

The first step of signature verification for RSASSA-PKCS1-v1_5 and RSASSA-PSS

  1. Length checking: If the length of the signature S is not k
    octets, output "invalid signature" and stop.

The first step of Decryption
RSAES-OAEP

  1. Length checking:
    b. If the length of the ciphertext C is not k octets, output
    "decryption error" and stop.
    RSAES-PKCS1-v1_5
  2. Length checking: If the length of the ciphertext C is not k
    octets (or if k < 11), output "decryption error" and stop.

This patch

  • adds length check in rsa_ossl_public_decrypt
  • adds length check in rsa_ossl_private_decrypt
  • removes obselete SSL_R_WRONG_SIGNATURE_LENGTH
  • removes obselete RSA_R_DATA_GREATER_THAN_MOD_LEN
  • removes duplicated checks
  • added test-case that verifies X509 cert with signature with trucated leading 0's
Checklist
  • documentation is added or updated
  • tests are added or updated

@t8m
Copy link
Member

t8m commented Mar 11, 2020

I do not think the CLA: trivial is appropriate for this kind of change. Also the Travis failure is relevant.

@t8m t8m added cla: trivial One of the commits is marked as 'CLA: trivial' hold: cla required The contributor needs to submit a license agreement labels Mar 11, 2020
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Mar 11, 2020
@mattcaswell mattcaswell added the hold: cla required The contributor needs to submit a license agreement label Mar 12, 2020
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Mar 12, 2020
@avgrygoriev
Copy link
Author

I have submitted ICLA. Haven't received any confirmation.
Updated PR with a test-case.

@mattcaswell
Copy link
Member

I have submitted ICLA. Haven't received any confirmation.

I processed it about 2 minutes ago :-)

Everything is good from a CLA perspective now.

@mattcaswell mattcaswell removed the cla: trivial One of the commits is marked as 'CLA: trivial' label Mar 12, 2020
mattcaswell
mattcaswell reviewed Mar 12, 2020
View reviewed changes
crypto/rsa/rsa_pmeth.c Outdated
if (siglen != (size_t)RSA_size(rsa)) {
RSAerr(RSA_F_PKEY_RSA_VERIFY, RSA_R_WRONG_SIGNATURE_LENGTH);
return -1;
}
Copy link
Member

Choose a reason for hiding this comment

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

This check is kind of already done by the RSA_public_decrypt call below:

    /*
     * This check was for equality but PGP does evil things and chops off the
     * top '0' bytes
     */
    if (flen > num) {
        RSAerr(RSA_F_RSA_OSSL_PUBLIC_DECRYPT, RSA_R_DATA_GREATER_THAN_MOD_LEN);
        goto err;
    }

So, it fails if the data is too long - but deliberately tolerates data that is too short and assumes it to have had leading 0's removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both PKCS#1 v1.5 and PSS have the same requirement in the specification, so RSA_public_decrypt also makes more sense as where the check should live. OAEP and PKCS#1 v1.5 decryption similarly require strict length checking, while RSA_private_decrypt has a similar weak check and comment.

FWIW, BoringSSL switched these to be spec-compliant back in 2014 and I don't think I've ever seen a report of issues.

Copy link
Author

Choose a reason for hiding this comment

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

AFAIU, PKCS# v1.5 signature length is checked here
int_rsa_verify:wrong signature length:crypto/rsa/rsa_sign.c:297

    if (siglen != (size_t)RSA_size(rsa)) {
        RSAerr(RSA_F_INT_RSA_VERIFY, RSA_R_WRONG_SIGNATURE_LENGTH);
        return 0;
    }
    //...
    len = RSA_public_decrypt((int)siglen, sigbuf, d

That's why I decided to add similar check for PSS above RSA_public_decrypt

Do you insist to move this check into RSA_public_decrypt (meaning rsa_ossl_public_decrypt) ?

I have also viewed BoringSSL. It has this check in PSS verify function, not in generic decrypt function.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIU, PKCS# v1.5 signature length is checked here
int_rsa_verify:wrong signature length:crypto/rsa/rsa_sign.c:297

Ah, interesting. Hopefully it's then safe to tighten RSA_public_decrypt and perform the check in a more common place, since that means OpenSSL already rejects truncated PKCS#1 v1.5 signatures.

I have also viewed BoringSSL. It has this check in PSS verify function, not in generic decrypt function.

It's right here:
https://boringssl.googlesource.com/boringssl/+/959c76d928c4df2827635ba68faac160edce10d8/crypto/fipsmodule/rsa/rsa_impl.c#594

Copy link
Member

Choose a reason for hiding this comment

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

Yes - given that we already check this in PKCS1 v1.5, I think I favour just changing the check that is already there in RSA_public_decrypt which I noted above to be for strict equality and remove that comment. That comment has been there since before OpenSSL was born (i.e. from SSLeay) and is hence over 20 years old. I guess issues it was working around at the time have long since ceased to be a problem.

Copy link
Author

Choose a reason for hiding this comment

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

I have also viewed BoringSSL. It has this check in PSS verify function, not in generic decrypt function.

It's right here:
https://boringssl.googlesource.com/boringssl/+/959c76d928c4df2827635ba68faac160edce10d8/crypto/fipsmodule/rsa/rsa_impl.c#594

Oh! Didn't look here. Thank you very much.

I was talking about that code.
https://boringssl.googlesource.com/boringssl/+/959c76d928c4df2827635ba68faac160edce10d8/crypto/fipsmodule/rsa/rsa.c#645

I have updated patch and updated check to rsa_ossl_public_decrypt

According to documentation RSA_private_decrypt

but may be smaller, when leading zero bytes are in the ciphertext
therefore, I didn't change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Ah, yeah, that check is something else. That's about the output of the RSA operation and should never fail. That check is because RSA_verify_PKCS1_PSS_mgf1 is terrible and uses an implicit length. For safety's sake, I like transitions from explicit to implicit length to be checked so it's locally obvious it's not a memory error.)

I would advocate fixing RSA_private_decrypt for the same reason (the spec mandates it) and fixing the documentation, but I'll defer to the OpenSSL folks whether they want to do that. BoringSSL's RSA_private_decrypt has also been spec-compliant here since 2014 without issues.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated patch with added length check for rsa_ossl_private_decrypt
Updated documentation and removed unused RSA_R_DATA_GREATER_THAN_MOD_LEN

@avgrygoriev
Copy link
Author

Is that error "The job exceeded the maximum time limit for jobs, and has been terminated." something typical/random?
https://travis-ci.org/github/openssl/openssl/jobs/661647382?utm_medium=notification&utm_source=github_status

I was running same script on my machine, all tests passed.
Please, advice.

@richsalz
Copy link
Contributor

Yes it's not unexpected.

@mattcaswell
Copy link
Member

Timeouts are an irritatingly frequent problem:

#11319

@avgrygoriev avgrygoriev force-pushed the master branch 2 times, most recently from 9a82c63 to 1332ffa Compare March 13, 2020 15:20
@avgrygoriev
Add length check for RSA PKCS1 Decryption and Signature Verification
e4d6634 
According to RFC8017
https://tools.ietf.org/html/rfc8017

The first step of signature verification for RSASSA-PKCS1-v1_5 and RSASSA-PSS
1.  Length checking: If the length of the signature S is not k
          octets, output "invalid signature" and stop.

The first step of Decryption
RSAES-OAEP
1.  Length checking:
    b.  If the length of the ciphertext C is not k octets, output
        "decryption error" and stop.
RSAES-PKCS1-v1_5
1.  Length checking: If the length of the ciphertext C is not k
    octets (or if k < 11), output "decryption error" and stop.

This patch
- adds length check in rsa_ossl_public_decrypt
- adds length check in rsa_ossl_private_decrypt
- removes obselete SSL_R_WRONG_SIGNATURE_LENGTH
- removes obselete RSA_R_DATA_GREATER_THAN_MOD_LEN
- removes duplicated checks
- added test-case that verifies X509 cert with signature with trucated leading 0's
@avgrygoriev
Copy link
Author

Looking into travis failures.
So far could not reproduce locally.

mattcaswell
mattcaswell requested changes Mar 18, 2020
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

This is strictly speaking a breaking change. Does this need an OMC vote or do we think it is sufficiently unlikely that we won't actually break anything?

@openssl/omc

Either way we should have a CHANGES entry to describe this change I think.

@iamamoose iamamoose added the hold: need omc decision The OMC needs to make a decision label Jun 2, 2020
@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 30 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 61 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 92 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 123 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 154 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 185 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 216 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 247 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 278 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 309 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 340 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 371 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/omc but the last update was 402 days ago

@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Jul 23, 2021
@t8m t8m added this to the Post 3.0.0 milestone Jul 23, 2021
@mattcaswell
Copy link
Member

OMC: Referring to OTC

@mattcaswell mattcaswell added hold: need otc decision The OTC needs to make a decision and removed hold: need omc decision The OMC needs to make a decision labels Jan 19, 2022
@t8m
Copy link
Member

t8m commented Jan 25, 2022

OTC: The low level change in RSA code is OK for master, there should be a CHANGES entry

(The high level checks removal is subject to normal review process.)

@t8m t8m removed the hold: need otc decision The OTC needs to make a decision label Jan 25, 2022
@t8m
Copy link
Member

t8m commented Jan 25, 2022

Could you please rebase?

@openssl-machine
Copy link
Collaborator

This PR is waiting for the creator to make requested changes but it has not been updated for 48 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

@openssl-machine
Copy link
Collaborator

This PR is waiting for the creator to make requested changes but it has not been updated for 79 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

@openssl-machine
Copy link
Collaborator

This PR has been closed. It was waiting for the creator to make requested changes but it has not been updated for 90 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidben davidben davidben left review comments

@mattcaswell mattcaswell mattcaswell requested changes

Assignees
No one assigned
Labels
branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Milestone
Post 3.0.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@avgrygoriev @t8m @mattcaswell @richsalz @openssl-machine @davidben @iamamoose