Implement RSASSA-PKCS1-v1_5 as specified. by davidben · Pull Request #1474 · openssl/openssl

davidben · 2016-08-20T19:14:02Z

RFC 3447, section 8.2.2, steps 3 and 4 states that verifiers must encode
the DigestInfo struct and then compare the result against the public key
operation result. This implies that one and only one encoding is legal.

OpenSSL instead parses with crypto/asn1, then checks that the encoding
round-trips, and allows some variations for the parameter. Sufficient
laxness in this area can allow signature forgeries, as described in
https://www.imperialviolet.org/2014/09/26/pkcs1.html

Although there aren't known attacks against OpenSSL's current scheme,
this change makes OpenSSL implement the algorithm as specified. This
avoids the uncertainty and, more importantly, helps grow a healthy
ecosystem. Laxness beyond the spec, particularly in implementations
which enjoy wide use, risks harm to the ecosystem for all. A signature
producer which only tests against OpenSSL may not notice bugs and
accidentally become widely deployed. Thus implementations have a
responsibility to honor the specification as tightly as is practical.

In some cases, the damage is permanent and the spec deviation and
security risk becomes a tax all implementors must forever pay, but not
here. Both BoringSSL and Go successfully implemented and deployed
RSASSA-PKCS1-v1_5 as specified since their respective beginnings, so
this change should be compatible enough to pin down in future OpenSSL
releases.

See also https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00

As a bonus, by not having to deal with sign/verify differences, this
version is also somewhat clearer. It also more consistently enforces
digest lengths in the verify_recover codepath. The NID_md5_sha1 codepath
wasn't quite doing this right.

kroeckx · 2016-08-20T19:35:34Z

Could you add some minimal documentation to the new functions?

davidben · 2016-08-20T19:43:18Z

Done. Is something like that close to the preferred style? (There's only one new function, I think. Just the helper function which now is shared between sign and verify.)

kroeckx · 2016-08-20T19:44:35Z

So one thing I'm confused about is that you're talking about PKCS #1 v1.5, while the man page of RSA_sign() talks about PKCS #1 v2.0

davidben · 2016-08-20T19:46:30Z

Huh. I think that's a mistake in RSA_sign.pod. Or at least is confusing. PKCS#1 v2.0 specifies the v1.5 algorithm in it, alongside with the v2.0 one. v2.0 is PSS which takes a lot more parameters than RSA_sign.

davidben · 2016-08-20T19:50:07Z

Oh, sorry, I got that wrong. v2.0 still only specifies RSASSA-PKCS1-v1_5. v2.1 specifies two algorithms, RSASSA-PSS and RSASSA-PKCS1-v1_5.

I'll update RSA_sign.pod to be clearer.

davidben · 2016-08-20T19:52:21Z

(Updated)

kroeckx · 2016-08-20T20:14:06Z

crypto/rsa/rsa_sign.c

I think this only does step 2?

kroeckx · 2016-08-21T09:54:40Z

So travis complains about:
crypto/rsa/rsa_sign.c: In function 'encode_pkcs1':
crypto/rsa/rsa_sign.c:59:5: error: ISO C90 forbids mixed declarations and code [-Werror=edantic]
crypto/rsa/rsa_sign.c: In function 'int_rsa_verify':
crypto/rsa/rsa_sign.c:174:44: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
cc1: all warnings being treated as errors

davidben · 2016-08-21T15:24:39Z

Huh, I wonder why that didn't flag in my local build. Fixed.

davidben · 2016-09-09T06:35:07Z

Rebased, largely to rerun the CI. I can't reproduce the failure from the last run and, having dug into it, it looks very much like TLSProxy is just flaky since the divergence happens long after any PKCS1 bits happen. We'll see if a rerun does it again.

davidben · 2016-09-10T04:07:04Z

(Travis appears to be happy now.)

ekasper · 2016-09-26T13:07:16Z

ping @kroeckx

kroeckx · 2016-10-23T20:10:05Z

crypto/rsa/rsa_sign.c

I really dislike undocumented functions. Can you add something short there?

Can you add something about the return values?

kroeckx · 2016-10-23T21:56:04Z

crypto/rsa/rsa_sign.c

EVP_MD_size() can actually handle NULL, but it seems to be a bad style to first call a function with NULL and then later check that it the variable was NULL and give an error.

kroeckx · 2016-10-23T21:58:00Z

crypto/rsa/rsa_sign.c

I know you didn't name this variable, but care to rename it?

Done. Also tweaked a few other variables to be more consistent and fixed some style problems (I managed to get tab characters in there apparently).

kroeckx · 2016-10-23T22:40:37Z

crypto/rsa/rsa_sign.c

This check doesn't seem to happen anymore, not sure it's important.

Oops, fixed. Also fixed the missing equivalent check in the MDC2 special-case.

kroeckx · 2016-10-29T18:58:54Z

@davidben: ping

davidben · 2016-10-29T20:55:44Z

Updated. (Let me know if you'd like this squashed or rebased. I'm not sure what the usual preference is.)

RFC 3447, section 8.2.2, steps 3 and 4 states that verifiers must encode the DigestInfo struct and then compare the result against the public key operation result. This implies that one and only one encoding is legal. OpenSSL instead parses with crypto/asn1, then checks that the encoding round-trips, and allows some variations for the parameter. Sufficient laxness in this area can allow signature forgeries, as described in https://www.imperialviolet.org/2014/09/26/pkcs1.html Although there aren't known attacks against OpenSSL's current scheme, this change makes OpenSSL implement the algorithm as specified. This avoids the uncertainty and, more importantly, helps grow a healthy ecosystem. Laxness beyond the spec, particularly in implementations which enjoy wide use, risks harm to the ecosystem for all. A signature producer which only tests against OpenSSL may not notice bugs and accidentally become widely deployed. Thus implementations have a responsibility to honor the specification as tightly as is practical. In some cases, the damage is permanent and the spec deviation and security risk becomes a tax all implementors must forever pay, but not here. Both BoringSSL and Go successfully implemented and deployed RSASSA-PKCS1-v1_5 as specified since their respective beginnings, so this change should be compatible enough to pin down in future OpenSSL releases. See also https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00 As a bonus, by not having to deal with sign/verify differences, this version is also somewhat clearer. It also more consistently enforces digest lengths in the verify_recover codepath. The NID_md5_sha1 codepath wasn't quite doing this right.

PKCS openssl#1 v2.0 is the name of a document which specifies an algorithm RSASSA-PKCS1-v1_5, often referred to as "PKCS openssl#1 v1.5" after an earlier document which specified it. This gets further confusing because the document PKCS openssl#1 v2.1 specifies two signature algorithms, RSASSA-PKCS1-v1_5 and RSASSA-PSS. RSA_sign implements RSASSA-PKCS1-v1_5. Refer to the document using the RFC number which is easier to find anyway, and refer to the algorithm by its name.

davidben · 2016-10-29T20:57:28Z

(And rebased since GitHub was complaining about RSA_sign.pod conflicts.)

kroeckx · 2016-10-29T22:23:51Z

crypto/rsa/rsa_sign.c

Can you add something about the return values?

richsalz · 2016-11-06T22:59:13Z

gee, I dunnot about the -.006% coverage drop :)
(Yes i know it's probably the test program)
+1

davidben · 2016-11-06T23:28:46Z

Probably not, actually. I didn't touch the test program.

I'm not entirely clear on how to read the coveralls output. It lists even apps/s_client.c changed which doesn't make sense. I'm probably reading it wrong. Regardless, the only file that could have had meaningful coverage change is rsa_sign.c. That got reworked so relative coverage numbers probably aren't that important. But in terms absolute numbers, looking here, there's something to be desired:
https://coveralls.io/builds/8573107/source?filename=crypto%2Frsa%2Frsa_sign.c

Various length checks for the non-DigestInfo-wrapped special-cases could do with tests. Also the special cases in the first place.
verify-recover mode does not appear to have tests at all.
No coverage for RSA_R_WRONG_SIGNATURE_LENGTH

I can see about adding tests for these.

MD5/SHA1 and MDC-2 have special-case logic beyond the generic DigestInfo wrapping. Test that each of these works, including hash and length mismatches (both input and signature). Also add VerifyRecover tests. It appears 5824cc2 added support for VerifyRecover, but forgot to add the test data.

davidben · 2016-11-07T00:31:27Z

There. That should make the coverage bot happier. :-)

kroeckx · 2016-11-07T19:31:26Z

@richsalz: 1.1.0 + master?

richsalz · 2016-11-07T19:32:02Z

yes, both!

RFC 3447, section 8.2.2, steps 3 and 4 states that verifiers must encode the DigestInfo struct and then compare the result against the public key operation result. This implies that one and only one encoding is legal. OpenSSL instead parses with crypto/asn1, then checks that the encoding round-trips, and allows some variations for the parameter. Sufficient laxness in this area can allow signature forgeries, as described in https://www.imperialviolet.org/2014/09/26/pkcs1.html Although there aren't known attacks against OpenSSL's current scheme, this change makes OpenSSL implement the algorithm as specified. This avoids the uncertainty and, more importantly, helps grow a healthy ecosystem. Laxness beyond the spec, particularly in implementations which enjoy wide use, risks harm to the ecosystem for all. A signature producer which only tests against OpenSSL may not notice bugs and accidentally become widely deployed. Thus implementations have a responsibility to honor the specification as tightly as is practical. In some cases, the damage is permanent and the spec deviation and security risk becomes a tax all implementors must forever pay, but not here. Both BoringSSL and Go successfully implemented and deployed RSASSA-PKCS1-v1_5 as specified since their respective beginnings, so this change should be compatible enough to pin down in future OpenSSL releases. See also https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00 As a bonus, by not having to deal with sign/verify differences, this version is also somewhat clearer. It also more consistently enforces digest lengths in the verify_recover codepath. The NID_md5_sha1 codepath wasn't quite doing this right. Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Rich Salz <[email protected]> GH: #1474

PKCS #1 v2.0 is the name of a document which specifies an algorithm RSASSA-PKCS1-v1_5, often referred to as "PKCS #1 v1.5" after an earlier document which specified it. This gets further confusing because the document PKCS #1 v2.1 specifies two signature algorithms, RSASSA-PKCS1-v1_5 and RSASSA-PSS. RSA_sign implements RSASSA-PKCS1-v1_5. Refer to the document using the RFC number which is easier to find anyway, and refer to the algorithm by its name. Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Rich Salz <[email protected]> GH: #1474

MD5/SHA1 and MDC-2 have special-case logic beyond the generic DigestInfo wrapping. Test that each of these works, including hash and length mismatches (both input and signature). Also add VerifyRecover tests. It appears 5824cc2 added support for VerifyRecover, but forgot to add the test data. Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Rich Salz <[email protected]> GH: #1474

RFC 3447, section 8.2.2, steps 3 and 4 states that verifiers must encode the DigestInfo struct and then compare the result against the public key operation result. This implies that one and only one encoding is legal. OpenSSL instead parses with crypto/asn1, then checks that the encoding round-trips, and allows some variations for the parameter. Sufficient laxness in this area can allow signature forgeries, as described in https://www.imperialviolet.org/2014/09/26/pkcs1.html Although there aren't known attacks against OpenSSL's current scheme, this change makes OpenSSL implement the algorithm as specified. This avoids the uncertainty and, more importantly, helps grow a healthy ecosystem. Laxness beyond the spec, particularly in implementations which enjoy wide use, risks harm to the ecosystem for all. A signature producer which only tests against OpenSSL may not notice bugs and accidentally become widely deployed. Thus implementations have a responsibility to honor the specification as tightly as is practical. In some cases, the damage is permanent and the spec deviation and security risk becomes a tax all implementors must forever pay, but not here. Both BoringSSL and Go successfully implemented and deployed RSASSA-PKCS1-v1_5 as specified since their respective beginnings, so this change should be compatible enough to pin down in future OpenSSL releases. See also https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00 As a bonus, by not having to deal with sign/verify differences, this version is also somewhat clearer. It also more consistently enforces digest lengths in the verify_recover codepath. The NID_md5_sha1 codepath wasn't quite doing this right. Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Rich Salz <[email protected]> GH: #1474 (cherry picked from commit 608a026)

PKCS #1 v2.0 is the name of a document which specifies an algorithm RSASSA-PKCS1-v1_5, often referred to as "PKCS #1 v1.5" after an earlier document which specified it. This gets further confusing because the document PKCS #1 v2.1 specifies two signature algorithms, RSASSA-PKCS1-v1_5 and RSASSA-PSS. RSA_sign implements RSASSA-PKCS1-v1_5. Refer to the document using the RFC number which is easier to find anyway, and refer to the algorithm by its name. Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Rich Salz <[email protected]> GH: #1474 (cherry picked from commit aa90ca1)

MD5/SHA1 and MDC-2 have special-case logic beyond the generic DigestInfo wrapping. Test that each of these works, including hash and length mismatches (both input and signature). Also add VerifyRecover tests. It appears 5824cc2 added support for VerifyRecover, but forgot to add the test data. Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Rich Salz <[email protected]> GH: #1474 (cherry picked from commit f320555)

kroeckx reviewed Aug 20, 2016
View reviewed changes

mattcaswell added this to the Post 1.1.0 milestone Aug 22, 2016

kroeckx added the CLA OK label Aug 27, 2016

davidben force-pushed the pkcs1-v1_5-per-spec branch from 05414b4 to fa56e99 Compare September 9, 2016 06:32

kroeckx requested changes Oct 23, 2016

View reviewed changes

kroeckx reviewed Oct 23, 2016

View reviewed changes

davidben added 3 commits October 29, 2016 16:56

Review comments.

4a23ebf

davidben force-pushed the pkcs1-v1_5-per-spec branch from 1eacf71 to 4a23ebf Compare October 29, 2016 20:57

kroeckx requested changes Oct 29, 2016

View reviewed changes

crypto/rsa/rsa_sign.c Outdated

Copy link

Member

kroeckx Oct 29, 2016

Choose a reason for hiding this comment

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

Can you add something about the return values?

Review comment

0a56c40

kroeckx approved these changes Oct 30, 2016

View reviewed changes

kroeckx self-assigned this Oct 30, 2016

kroeckx added the reviewed label Oct 30, 2016

richsalz added the approval: done This pull request has the required number of approvals label Nov 6, 2016

richsalz approved these changes Nov 7, 2016

View reviewed changes

kroeckx closed this Nov 7, 2016

Uh oh!

Comments

Conversation

davidben commented Aug 20, 2016

Uh oh!

kroeckx commented Aug 20, 2016

Uh oh!

davidben commented Aug 20, 2016

Uh oh!

kroeckx commented Aug 20, 2016

Uh oh!

davidben commented Aug 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidben commented Aug 20, 2016

Uh oh!

davidben commented Aug 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx commented Aug 21, 2016

Uh oh!

davidben commented Aug 21, 2016

Uh oh!

davidben commented Sep 9, 2016

Uh oh!

davidben commented Sep 10, 2016

Uh oh!

ekasper commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx commented Oct 29, 2016

Uh oh!

davidben commented Oct 29, 2016

Uh oh!

davidben commented Oct 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richsalz commented Nov 6, 2016

Uh oh!

davidben commented Nov 6, 2016

Uh oh!

davidben commented Nov 7, 2016

Uh oh!

kroeckx commented Nov 7, 2016

Uh oh!

richsalz commented Nov 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidben commented Aug 20, 2016 •

edited

Loading