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

PKCS#1 2.1 sign to be done in FIPS-186-4 compliant way #2007

Conversation

mohammad1603
Copy link
Contributor

Description

  • Change PKCS#1 2.1 signature so that it succeeds signing large hashes and does it in a FIPS-186-4 compliant way
  • Write a non-regression test to verify the issue

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

@RonEld RonEld added enhancement Arm Contribution component-crypto Crypto primitives and low-level interfaces labels Sep 5, 2018
@RonEld RonEld requested review from Patater and yanesca September 5, 2018 12:05
Copy link
Contributor

@RonEld RonEld left a comment

Choose a reason for hiding this comment

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

A couple of observations, not blocking in view. I would like to hear other comments though

library/rsa.c Outdated
if ( olen == 128 && hlen == 64 )
slen = hlen - 2;
else
slen = hlen;

if( olen < hlen + slen + 2 )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should move to the else statement before, in a block, as in case olen is 128 and hlen is 64, this is a redundant check. ( 128 is always equal to 64 + 64 - 2 + 2 )

Copy link
Contributor Author

@mohammad1603 mohammad1603 Sep 5, 2018

Choose a reason for hiding this comment

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

It is related to the encoding: zeros || hash length || salt please see step 5 in the following:
https://tools.ietf.org/html/rfc8017#page-43
So it seems to be ok where it is.

Copy link
Contributor

@RonEld RonEld Sep 5, 2018

Choose a reason for hiding this comment

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

I know, but what I mean is change this to:

    if ( olen == 128 && hlen == 64 )
    {
         slen = hlen - 2;
    }
    else
    {
        slen = hlen;
        if( olen < hlen + slen + 2 )
            return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA );
    }

This is because:
a. It is redundant to check that 128 < 128 . Although it may be possible that the compiler will optimize this out.
b. The link you referenced is an IETF RFC, which is relevant to the else statement, as the use case of olen 128 and hlen 64 is FIPS 186-4 related.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand, if you think it must be changed i'll 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.

I don't think it's a must, but I think it's more readable, and no redundant check

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@@ -376,8 +376,17 @@ pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0
RSASSA-PSS Verification Test Vector Int
pkcs1_rsassa_pss_verify:1024:16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"859eef2fd78aca00308bdc471193bf55bf9d78db8f8a672b484634f3c9c26e6478ae10260fe0dd8c082e53a5293af2173cd50c6d5d354febf78b26021c25c02712e78cd4694c9f469777e451e7f8e9e04cd3739c6bbfedae487fb55644e9ca74ff77a53cb729802f6ed4a5ffa8ba159890fc":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"8daa627d3de7595d63056c7ec659e54406f10610128baae821c8b2a0f3936d54dc3bdce46689f6b7951bb18e840542769718d5715d210d85efbb596192032c42be4c29972c856275eb6d5a45f05f51876fc6743deddd28caec9bb30ea99e02c3488269604fe497f74ccd7c7fca1671897123cbd30def5d54a2b5536ad90a747e":0

RSASSA-PSS Signing Test Vector Hash too large
pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0dced472394a0df743fe7f929e378efdb368eddff453cf007af6d948e0ade757371f8a711e278f6b":16:"c6d92b6fee7414d1358ce1546fb62987530b90bd15e0f14963a5e2635adb69347ec0c01b2ab1763fd8ac1a592fb22757463a982425bb97a3a437c5bf86d03f2f":16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA512:"d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd00":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"":MBEDTLS_ERR_RSA_BAD_INPUT_DATA
RSASSA-PSS Signing Test Vector1 Hash-512
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did you found the test vectors at the end? Is this with the slen of 62?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion with @yanesca we thought that the best way to test this is to add non-regression test vectors since there are no certified test vectors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but this means that we don't have any confirmation that our 1024 bit key is compliant to FIPS 186-4, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we don't have any NIST test vectors for FIPS-186-4.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whether the key was generated in a FIPS-compliant way is irrelevant for this test case.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to specify how the test vectors were generated, either in the commit message or as a comment in the .data file. I won't insist on it because we have a long history of not doing it. However I'd still like to know how the test vectors were generated for review purposes (e.g. “manually with openssl pkeyutl”, “manually with Cryptodome”, “I did the math in my head”, …).

Copy link
Contributor

Choose a reason for hiding this comment

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

These tests were not correct. We were testing our buggy implementation against itself. I've removed these tests and added new ones, ensuring we interoperate with OpenSSL where possible.

However, this set of tests surely had a certain intent. I'm not sure I've covered all the cases these previous tests covered in terms of, for example, mixing up various salts and md algorithms, focusing mainly on key size as the only variable. If there is some coverage provided by the old tests that you'd like to see covered, please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

The main point of this test was to verify that we return an error when the salt doesn't fit. There is no longer any test that does this. Please add at least one test case where our new minimum salt size doesn't fit.

This test case also combined two different hashes, but I think that's best kept separate. Improving test coverage when mixing different hashes (which is permitted but not recommended by PKCS#1) is out of scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Since I already have a 1016-bit key with SHA-512, no further action is necessary.

Patater
Patater previously requested changes Sep 5, 2018
@@ -376,8 +376,17 @@ pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0
RSASSA-PSS Verification Test Vector Int
pkcs1_rsassa_pss_verify:1024:16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"859eef2fd78aca00308bdc471193bf55bf9d78db8f8a672b484634f3c9c26e6478ae10260fe0dd8c082e53a5293af2173cd50c6d5d354febf78b26021c25c02712e78cd4694c9f469777e451e7f8e9e04cd3739c6bbfedae487fb55644e9ca74ff77a53cb729802f6ed4a5ffa8ba159890fc":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"8daa627d3de7595d63056c7ec659e54406f10610128baae821c8b2a0f3936d54dc3bdce46689f6b7951bb18e840542769718d5715d210d85efbb596192032c42be4c29972c856275eb6d5a45f05f51876fc6743deddd28caec9bb30ea99e02c3488269604fe497f74ccd7c7fca1671897123cbd30def5d54a2b5536ad90a747e":0

RSASSA-PSS Signing Test Vector Hash too large
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to do a "hash too large" test with the new implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

not with 1024 bit key and SHA512. In this case, the olen is 128, and hlen is 64, then slen is 62, so we won't fail in the hash too long check of olen < hlen + slen + 2 which previously failed
Any other hash larger than 64 bytes is not a NIST approved algorithm, so we should fail before checking the hash

Copy link
Contributor

Choose a reason for hiding this comment

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

@RonEld Our library is not limited to NIST-approved algorithms. We support RSA keys of any size within a “reasonable” range, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added a "Hash too large" check using a 1016-bit key with SHA-512.

library/rsa.c Outdated
@@ -1550,7 +1550,12 @@ int mbedtls_rsa_rsassa_pss_sign( mbedtls_rsa_context *ctx,
return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA );

hlen = mbedtls_md_get_size( md_info );
slen = hlen;

/* Align with FIPS-186-4 */
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you expand this comment to explain how this aligns with FIPS 186-4? Please refer to sections within the document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor

Choose a reason for hiding this comment

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

I've expanded the comment.

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

  • The changelog entry and several comments and commit messages state that the PSS function was not FIPS-compliant, but that's not accurate. There was a case where it returned an error and it now doesn't. “Was not FIPS-compliant before” implies that there was a case where it returned a success status with non-compliant output and it now either returns compliant output or an error, but this is not what happened here.
  • Also there are many mentions of “PKCS#1 v2.1” which should state “RSASSA-PSS” (or “RSA-PSS” or “PSS”) instead. “PKCS#1 v2.1” is a standard which supports two signature methods; one is known by the version of the standard that introduces it (“RSASSA-PKCS1-v1_5” following the spelling of the PKCS#1 v2.x documents), the other is known as “RSASSA-PSS” or “PSS” for short.
  • I think the choice of salt length is bad design, see my comments in rsa.c.
  • We do need at least one test case for “not enough room for the salt”.
  • (Preexisting) The documentation of mbedtls_rsa_rsassa_pss_sign in rsa.h should explain what salt length is used.
  • Please add tests of mbedtls_rsa_rsassa_pss_verify with SHA-512, a 1024-bit key and a 62-byte salt. We should test that our implementation accepts what it produces. There should be at least one good signature test and one bad signature test.

ChangeLog Outdated
@@ -62,6 +62,7 @@ Changes
mbedtls_ssl_get_record_expansion() in case of ChachaPoly ciphersuites,
or CBC ciphersuites in (D)TLS versions 1.1 or higher. Fixes #1913, #1914.
* Add support for buffering of out-of-order handshake messages.
* Change the RSA PKCS#1 V2.1 sign to be FIPS-186-4 compliant.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is too vague and wrong. The word “compliant” when applied to a whole procedure is very strong and must be used sparingly: it means that we guarantee that the procedure complies with the specification in every aspect. Also this sentence is not grammatically correct. Also the code was already compliant with FIPS 186-4 in this respect: the library now support an additional case, but previous versions did not choose a non-compliant salt size.

Extend RSASSA-PSS signature to use a 62-byte salt when the hash length is 64 bytes and the key size is 128 bytes (512-bit hash and 1024-bit key). Before, the signature returned an error in this case. This salt length remains compliant with FIPS 186-4.

library/rsa.c Outdated
/* According to FIPS-186-4 section 5.5 (e) the salt length shall
* satisfy (0 < slen <= hlen - 2) in case of modulus 1024-bits with approved
* hash output length 512-bits. This change allow the sign to be done in
* FIPS compliant way. */
Copy link
Contributor

Choose a reason for hiding this comment

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

“change” what change? Before/after comparisons belong in the commit message, not in the code. A reader of the code doesn't know how the code used to behave. The comment should explain why these salt lengths are chosen (both the olen==128 && hlen==64 case, and the other case). Suggestion:

Calculate the largest possible salt size. Normally this is the hash size, which is the maximum size the salt can have. If there is not enough room, use the maximum salt size that fits. The constraint is that the hash size plus the salt size plus 2 bytes must be at most the key size. This complies with FIPS 186-4 §5.5 (e) and RFC 8017 (PKCS#1 v2.2) §9.1.1 step 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated

library/rsa.c Outdated
* satisfy (0 < slen <= hlen - 2) in case of modulus 1024-bits with approved
* hash output length 512-bits. This change allow the sign to be done in
* FIPS compliant way. */
if ( olen == 128 && hlen == 64 )
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like making a special case like this, where you can e.g. sign a 512-bit hash with a 1024-bit key but not with a 1032-bit key. @yanesca How about changing the logic to be more in line with what I wrote in my comment above?

size_t min_slen = hlen - 2;
if( olen < 2 + hlen + min_slen )
    return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA );
else if( olen >= 2 + hlen + hlen )
    slen = hlen;
else
    slen = olen - 2 - hlen;

(If we use this exact logic, tweak the comment to explain that we additionally require at least hlen-2 bytes of salt. Note that this is a design choice in our implementation: slen=0 is permitted by both FIPS and PKCS#1.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated, although I didn't use your proposed code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've gone back to using the suggested code and associated comment. It feels like we could reduce this to fewer checks and also avoid the underflow issue of my previous implementation, but it's not worth optimizing right now. This code is good enough.

@@ -376,8 +376,17 @@ pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0
RSASSA-PSS Verification Test Vector Int
pkcs1_rsassa_pss_verify:1024:16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"859eef2fd78aca00308bdc471193bf55bf9d78db8f8a672b484634f3c9c26e6478ae10260fe0dd8c082e53a5293af2173cd50c6d5d354febf78b26021c25c02712e78cd4694c9f469777e451e7f8e9e04cd3739c6bbfedae487fb55644e9ca74ff77a53cb729802f6ed4a5ffa8ba159890fc":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"8daa627d3de7595d63056c7ec659e54406f10610128baae821c8b2a0f3936d54dc3bdce46689f6b7951bb18e840542769718d5715d210d85efbb596192032c42be4c29972c856275eb6d5a45f05f51876fc6743deddd28caec9bb30ea99e02c3488269604fe497f74ccd7c7fca1671897123cbd30def5d54a2b5536ad90a747e":0

RSASSA-PSS Signing Test Vector Hash too large
Copy link
Contributor

Choose a reason for hiding this comment

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

We still need a test with a hash that is too large. Use a 1016-bit key with a 512-bit hash, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@@ -376,8 +376,17 @@ pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0
RSASSA-PSS Verification Test Vector Int
pkcs1_rsassa_pss_verify:1024:16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"859eef2fd78aca00308bdc471193bf55bf9d78db8f8a672b484634f3c9c26e6478ae10260fe0dd8c082e53a5293af2173cd50c6d5d354febf78b26021c25c02712e78cd4694c9f469777e451e7f8e9e04cd3739c6bbfedae487fb55644e9ca74ff77a53cb729802f6ed4a5ffa8ba159890fc":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"8daa627d3de7595d63056c7ec659e54406f10610128baae821c8b2a0f3936d54dc3bdce46689f6b7951bb18e840542769718d5715d210d85efbb596192032c42be4c29972c856275eb6d5a45f05f51876fc6743deddd28caec9bb30ea99e02c3488269604fe497f74ccd7c7fca1671897123cbd30def5d54a2b5536ad90a747e":0

RSASSA-PSS Signing Test Vector Hash too large
Copy link
Contributor

Choose a reason for hiding this comment

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

@RonEld Our library is not limited to NIST-approved algorithms. We support RSA keys of any size within a “reasonable” range, for example.

@@ -376,8 +376,17 @@ pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0
RSASSA-PSS Verification Test Vector Int
pkcs1_rsassa_pss_verify:1024:16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"859eef2fd78aca00308bdc471193bf55bf9d78db8f8a672b484634f3c9c26e6478ae10260fe0dd8c082e53a5293af2173cd50c6d5d354febf78b26021c25c02712e78cd4694c9f469777e451e7f8e9e04cd3739c6bbfedae487fb55644e9ca74ff77a53cb729802f6ed4a5ffa8ba159890fc":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"8daa627d3de7595d63056c7ec659e54406f10610128baae821c8b2a0f3936d54dc3bdce46689f6b7951bb18e840542769718d5715d210d85efbb596192032c42be4c29972c856275eb6d5a45f05f51876fc6743deddd28caec9bb30ea99e02c3488269604fe497f74ccd7c7fca1671897123cbd30def5d54a2b5536ad90a747e":0

RSASSA-PSS Signing Test Vector Hash too large
pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0dced472394a0df743fe7f929e378efdb368eddff453cf007af6d948e0ade757371f8a711e278f6b":16:"c6d92b6fee7414d1358ce1546fb62987530b90bd15e0f14963a5e2635adb69347ec0c01b2ab1763fd8ac1a592fb22757463a982425bb97a3a437c5bf86d03f2f":16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA512:"d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd00":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"":MBEDTLS_ERR_RSA_BAD_INPUT_DATA
RSASSA-PSS Signing Test Vector1 Hash-512
Copy link
Contributor

Choose a reason for hiding this comment

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

Please write SHA-512 if you mean SHA-512. “Hash-512” is meaningless.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've deleted these tests.

@@ -376,8 +376,17 @@ pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0
RSASSA-PSS Verification Test Vector Int
pkcs1_rsassa_pss_verify:1024:16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"859eef2fd78aca00308bdc471193bf55bf9d78db8f8a672b484634f3c9c26e6478ae10260fe0dd8c082e53a5293af2173cd50c6d5d354febf78b26021c25c02712e78cd4694c9f469777e451e7f8e9e04cd3739c6bbfedae487fb55644e9ca74ff77a53cb729802f6ed4a5ffa8ba159890fc":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"8daa627d3de7595d63056c7ec659e54406f10610128baae821c8b2a0f3936d54dc3bdce46689f6b7951bb18e840542769718d5715d210d85efbb596192032c42be4c29972c856275eb6d5a45f05f51876fc6743deddd28caec9bb30ea99e02c3488269604fe497f74ccd7c7fca1671897123cbd30def5d54a2b5536ad90a747e":0

RSASSA-PSS Signing Test Vector Hash too large
pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0dced472394a0df743fe7f929e378efdb368eddff453cf007af6d948e0ade757371f8a711e278f6b":16:"c6d92b6fee7414d1358ce1546fb62987530b90bd15e0f14963a5e2635adb69347ec0c01b2ab1763fd8ac1a592fb22757463a982425bb97a3a437c5bf86d03f2f":16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA512:"d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd00":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"":MBEDTLS_ERR_RSA_BAD_INPUT_DATA
RSASSA-PSS Signing Test Vector1 Hash-512
Copy link
Contributor

Choose a reason for hiding this comment

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

Whether the key was generated in a FIPS-compliant way is irrelevant for this test case.

@@ -376,8 +376,17 @@ pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0
RSASSA-PSS Verification Test Vector Int
pkcs1_rsassa_pss_verify:1024:16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"859eef2fd78aca00308bdc471193bf55bf9d78db8f8a672b484634f3c9c26e6478ae10260fe0dd8c082e53a5293af2173cd50c6d5d354febf78b26021c25c02712e78cd4694c9f469777e451e7f8e9e04cd3739c6bbfedae487fb55644e9ca74ff77a53cb729802f6ed4a5ffa8ba159890fc":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"8daa627d3de7595d63056c7ec659e54406f10610128baae821c8b2a0f3936d54dc3bdce46689f6b7951bb18e840542769718d5715d210d85efbb596192032c42be4c29972c856275eb6d5a45f05f51876fc6743deddd28caec9bb30ea99e02c3488269604fe497f74ccd7c7fca1671897123cbd30def5d54a2b5536ad90a747e":0

RSASSA-PSS Signing Test Vector Hash too large
pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0dced472394a0df743fe7f929e378efdb368eddff453cf007af6d948e0ade757371f8a711e278f6b":16:"c6d92b6fee7414d1358ce1546fb62987530b90bd15e0f14963a5e2635adb69347ec0c01b2ab1763fd8ac1a592fb22757463a982425bb97a3a437c5bf86d03f2f":16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA512:"d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd00":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"":MBEDTLS_ERR_RSA_BAD_INPUT_DATA
RSASSA-PSS Signing Test Vector1 Hash-512
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to specify how the test vectors were generated, either in the commit message or as a comment in the .data file. I won't insist on it because we have a long history of not doing it. However I'd still like to know how the test vectors were generated for review purposes (e.g. “manually with openssl pkeyutl”, “manually with Cryptodome”, “I did the math in my head”, …).

@Patater Patater added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 10, 2018
@Patater
Copy link
Contributor

Patater commented Sep 10, 2018

  • The changelog entry and several comments and commit messages state that the PSS function was not FIPS-compliant, but that's not accurate. There was a case where it returned an error and it now doesn't. “Was not FIPS-compliant before” implies that there was a case where it returned a success status with non-compliant output and it now either returns compliant output or an error, but this is not what happened here.

Not yet fixed. Will require rebase. Will rebase after review.

  • Also there are many mentions of “PKCS#1 v2.1” which should state “RSASSA-PSS” (or “RSA-PSS” or “PSS”) instead. “PKCS#1 v2.1” is a standard which supports two signature methods; one is known by the version of the standard that introduces it (“RSASSA-PKCS1-v1_5” following the spelling of the PKCS#1 v2.x documents), the other is known as “RSASSA-PSS” or “PSS” for short.

Not yet entirely fixed. Will fix as part of rebase.

@Patater Patater force-pushed the IOTCRYPT-256-rsa-sign-1024-large-hash branch from 519956c to d8818f2 Compare September 11, 2018 10:37
@Patater
Copy link
Contributor

Patater commented Sep 11, 2018

@Patater Patater force-pushed the IOTCRYPT-256-rsa-sign-1024-large-hash branch from d8818f2 to 055b6cd Compare September 11, 2018 11:05
@Patater
Copy link
Contributor

Patater commented Sep 11, 2018

retest

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

There's an integer overflow. Other than that, just a few minor documentation issues.

ChangeLog Outdated
compared to the RSA key size. The salt length chosen will be the largest
possible between the hash length minus 2 bytes and the hash length
inclusive. Previously, the signature function returned an error in this
case. The chosen salt length remains compliant with FIPS 186-4.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a bit wordy and yet doesn't list the one case that matters in practice, which is SHA-512 with a 1024-bit key. It's also not clear that this is about signature, not verification. Suggestion:

Extend RSASS-PSS signature to allow slightly a smaller salt size. Previously, PSS signature always used a salt with the same length as the hash, and returned an error if this was not possible. Now the salt size may be up to two bytes shorter. This allows the library to support all hash and signature sizes that comply with FIPS 186-4, including SHA-512 with a 1024-bit key.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your suggestion is nearly as wordy, but more natural to read in any case. I'll use it. The reason I previously didn't mention 1024-bit key was because 1032-bit keys are also affected, but I can see benefit in mentioning the common use case and impetus for the change in the first place.

* size - 2 bytes of salt. The constraint is that the hash size
* plus the salt size plus 2 bytes must be at most the key
* size. This complies with FIPS 186-4 §5.5 (e) and RFC 8017
* (PKCS#1 v2.2) §9.1.1 step 3.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not completely accurate and a little confusing regarding “the constraint”, sorry. The salt can be longer than the hash, it's allowed by PKCS#1 but forbidden by FIPS. Suggestion:

This function always uses the maximum salt size slen that satisfies the following constraints:

  • slen + hlen + 2 <= siglen where hlen is the length of the hash (\p hashlen if \p md_alg is #MBEDTLS_MD_NONE, otherwise the length of md_alg) and siglen is the length of the signature (which is also the length of the key).
  • slen <= hlen.
  • slen >= hlen - 2.

The first two constraints ensure that the salt length complies with FIPS 186-4 §5.5 (e) and RFC 8017 (PKCS#1 v2.2) §9.1.1 step 3.

Or:

This function always uses the maximum possible salt size, up to the length of the payload hash. This choice of salt size complies with FIPS 186-4 §5.5 (e) and RFC 8017 (PKCS#1 v2.2) §9.1.1 step 3. Furthermore this function enforces a minimum salt size which is the hash size minus 2 bytes. If this minimum size is too large given the key size (the salt size, plus the hash size, plus 2 bytes must be no more than the key size in bytes), this function returns #MBEDTLS_ERR_RSA_BAD_INPUT_DATA.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've gone with the second suggestion. Thanks.

* be the minimum of either the hash length or the key size minus the hash
* length minus 2 bytes. The salt length is always at least the hash length
* minus 2 bytes. This complies with FIPS 186-4 §5.5 (e) and RFC 8017
* (PKCS#1 v2.2) §9.1.1 step 3. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the sentence “This complies …” before “The salt length is always at least …”, since the “at least” constraint isn't required by either FIPS or PKCS#1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved. Didn't want to imply that FIPS or PKCS#1 required the chosen minimum bound.

library/rsa.c Outdated
* length minus 2 bytes. The salt length is always at least the hash length
* minus 2 bytes. This complies with FIPS 186-4 §5.5 (e) and RFC 8017
* (PKCS#1 v2.2) §9.1.1 step 3. */
max_slen = olen - hlen - 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if olen < hlen + 2? Then max_slen is SIZE_MAX - epsilon, slen is set to hlen below, and there's a buffer overflow. Please fix and add a non-regression test case (e.g. 520-bit key with 512-bit hash).

diff --git a/tests/suites/test_suite_pkcs1_v21.data b/tests/suites/test_suite_pkcs1_v21.data
index 33e7ed3..6e99d86 100644
--- a/tests/suites/test_suite_pkcs1_v21.data
+++ b/tests/suites/test_suite_pkcs1_v21.data
@@ -376,9 +376,12 @@ pkcs1_rsassa_pss_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0
 RSASSA-PSS Verification Test Vector Int
 pkcs1_rsassa_pss_verify:1024:16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"859eef2fd78aca00308bdc471193bf55bf9d78db8f8a672b484634f3c9c26e6478ae10260fe0dd8c082e53a5293af2173cd50c6d5d354febf78b26021c25c02712e78cd4694c9f469777e451e7f8e9e04cd3739c6bbfedae487fb55644e9ca74ff77a53cb729802f6ed4a5ffa8ba159890fc":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"8daa627d3de7595d63056c7ec659e54406f10610128baae821c8b2a0f3936d54dc3bdce46689f6b7951bb18e840542769718d5715d210d85efbb596192032c42be4c29972c856275eb6d5a45f05f51876fc6743deddd28caec9bb30ea99e02c3488269604fe497f74ccd7c7fca1671897123cbd30def5d54a2b5536ad90a747e":0
 
-RSASSA-PSS Signing Test Vector Hash too large
+RSASSA-PSS Signing: RSA-1016, SHA-512: minimum salt size not met
 pkcs1_rsassa_pss_sign:1016:16:"0e3cb6845e528229e19cfb24611e6859ac1cea7d35992b6e2e796823c52affa03400e42830f90697f084499c3e3587defc19e749e72433dd7b70c28b0c8280b7":16:"0c48f9e45ae38fdb4a5143be37d79a10cd4f1f9782ef26a4848a4449c72cfd712c68350818736385cb4a9ab6db5aef8e96c551039cfcc8915821aee069ed660d":16:"00aee7874a4db2f1510044405db29f14df0f37bbcf61fcbcc994a3d31caaf858a74cc8f2a40ac9a9ce7aa9a0680f62cf9d8d4b827114533fdbf86f16fc9dfe5cbf857d86135519a4611ffc59cb7473861619a78e3ec314715e804cff82d6f32e9f57ddf390563629883bd34f40e8db413209b151cee97d817a5d65c7da54734b":16:"010001":MBEDTLS_MD_SHA512:MBEDTLS_MD_SHA512:"d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd32a7c8a05bbc90d32c49d436e99569fd00":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"":MBEDTLS_ERR_RSA_BAD_INPUT_DATA
 
+RSASSA-PSS Signing: RSA-520, SHA-512: no possible salt size
+pkcs1_rsassa_pss_sign:520:16:"0f2383ce09800d8307a3a5f2cf55df574bd2aa0a97c1195154852ca7f6e0a2a655":16:"0d63ac8b5258aa8407c832b43a6f85edf2badda25c19509642fc723c1dcedd4da9":16:"00cab2a188decaf8c4d5833b5aaa4a0db033e12af4a881288441593c9b9c12a7e079a3509b86bac3bee2c9f6f7720940e42fcd9899772dbfbb415125b56e34c85f1d":16:"010001":MBEDTLS_MD_SHA512:MBEDTLS_MD_SHA512:"":"00000000000000000000000000000000":"":MBEDTLS_ERR_RSA_BAD_INPUT_DATA
+
 RSASSA-PSS Signature Example 1_1
 pkcs1_rsassa_pss_sign:1024:16:"e7e8942720a877517273a356053ea2a1bc0c94aa72d55c6e86296b2dfc967948c0a72cbccca7eacb35706e09a1df55a1535bd9b3cc34160b3b6dcd3eda8e6443":16:"b69dca1cf7d4d7ec81e75b90fcca874abcde123fd2700180aa90479b6e48de8d67ed24f9f19d85ba275874f542cd20dc723e6963364a1f9425452b269a6799fd":16:"a56e4a0e701017589a5187dc7ea841d156f2ec0e36ad52a44dfeb1e61f7ad991d8c51056ffedb162b4c0f283a12a88a394dff526ab7291cbb307ceabfce0b1dfd5cd9508096d5b2b8b6df5d671ef6377c0921cb23c270a70e2598e6ff89d19f105acc2d3f0cb35f29280e1386b6f64c4ef22e1e1f20d0ce8cffb2249bd9a2137":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"cdc87da223d786df3b45e0bbbc721326d1ee2af806cc315475cc6f0d9c66e1b62371d45ce2392e1ac92844c310102f156a0d8d52c1f4c40ba3aa65095786cb769757a6563ba958fed0bcc984e8b517a3d5f515b23b8a41e74aa867693f90dfb061a6e86dfaaee64472c00e5f20945729cbebe77f06ce78e08f4098fba41f9d6193c0317e8b60d4b6084acb42d29e3808a3bc372d85e331170fcbf7cc72d0b71c296648b3a4d10f416295d0807aa625cab2744fd9ea8fd223c42537029828bd16be02546f130fd2e33b936d2676e08aed1b73318b750a0167d0":"dee959c7e06411361420ff80185ed57f3e6776af":"9074308fb598e9701b2294388e52f971faac2b60a5145af185df5287b5ed2887e57ce7fd44dc8634e407c8e0e4360bc226f3ec227f9d9e54638e8d31f5051215df6ebb9c2f9579aa77598a38f914b5b9c1bd83c4e2f9f382a0d0aa3542ffee65984a601bc69eb28deb27dca12c82c2d4c3f66cd500f1ff2b994d8a4e30cbb33c":0

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

RSASSA-PSS Signature 1024-bit w/SHA-512
pkcs1_rsassa_pss_sign:1024:16:"00e8f95a716c127d5147dcc241a7c1fe8d5487b3e8b6e95e48a83334d21d00c79ad0a90e29941c0c53065b20059de95e9e406061416f7ac12edca1983b9ee28cc3":16:"00d72348b297e7e5dc4329f6ab874b17982584e0ab43174070a9be983c0f040320d6f893c40d2717cb3044380cb3230b7133621eb1c55a3ea56d0e7cee694b5df3":16:"00c3c9873548543591c1f947e412c33da56b9d1b94a58c2f410a8a620e9b4f1d9197643ebf527f5f62b202b9d67a32654d05f326a9b61e0106efdf4829673c4f3d23655996e2424059916ab47aa67e406c129679e5979ca46708866608ffa21f619843b959b4442e422598a2faab54a8cef1f131992677d2cf5bcaf2b5564f7419":16:"010001":MBEDTLS_MD_SHA512:MBEDTLS_MD_SHA512:"e35c6ed98f64a6d5a648fcab8adb16331db32e5d15c74a40edf94c3dc4a4de792d190889f20f1e24ed12054a6b28798fcb42d1c548769b734c96373142092aed277603f4738df4dc1446586d0ec64da4fb60536db2ae17fc7e3c04bbfbbbd907bf117c08636fa16f95f51a6216934d3e34f85030f17bbbc5ba69144058aff081e0b19cf03c17195c5e888ba58f6fe0a02e5c3bda9719a7":"653df9730e14e03f2ffb3374d6b75295aa4a52c38540b2d501adc1eb659a4d7a050769a3d11d0d5d6f3efb734200ade241fdc271c0f5eeed85b4bf00b2327bc8":"655d1cf86a7af5113d1791ab7b6627845ea2aa7efbae82705a3563e5ba0337a1d033cb9283b38c042056e0a1d0529891173e3df6621dd8b184930caec8b3cbe4d1068524dab0ec6854f6638d86b77434cd792ddec0d02327a9eebffcd6911ffd32ad9bcb569d3237398c8169d9c62e7eea81c1b456fd36019aad1e4b268c604d":0

RSASSA-PSS Signature 1024-bit w/SHA-512 (Verify)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow existing conventions for test case descriptions. In this file, for signature verification, we use “RSASSA-PSS verify …” or “RSASSA-PSS Verification …”.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed verification tests to start with "RSASSA-PSS Verification"

@gilles-peskine-arm
Copy link
Contributor

Also note that there's a Windows build failure in https://jenkins-internal.mbed.com/job/mbed-tls-pr/173

Functions like `mbedtls_md_get_size()` and `mgf_mask()` work with
`size_t`. Use local variables with `size_t` to match.
@Patater Patater force-pushed the IOTCRYPT-256-rsa-sign-1024-large-hash branch from 055b6cd to 032bbca Compare September 11, 2018 14:54
@Patater
Copy link
Contributor

Patater commented Sep 11, 2018

Waiting for Windows test results after update...

@Patater Patater added needs-ci Needs to pass CI tests needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 11, 2018
@Patater Patater dismissed their stale review September 11, 2018 14:59

I've reworked the PR and my previous review is no longer valid.

@Patater Patater force-pushed the IOTCRYPT-256-rsa-sign-1024-large-hash branch from 032bbca to 07ce7ef Compare September 11, 2018 16:44
@Patater
Copy link
Contributor

Patater commented Sep 11, 2018

Rebased to apply review feedback as fixups

Old branch: https://github.com/Patater/mbedtls/tree/dev/Patater/salty-pss-3
Expanded branch (fixups not yet applied): https://github.com/Patater/mbedtls/tree/dev/Patater/salty-pss-4

@Patater Patater force-pushed the IOTCRYPT-256-rsa-sign-1024-large-hash branch from 07ce7ef to 04b4ee6 Compare September 11, 2018 16:47
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Looks good!

@gilles-peskine-arm gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Sep 11, 2018
Copy link
Contributor

@dgreen-arm dgreen-arm left a comment

Choose a reason for hiding this comment

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

Just one (possible) typo, otherwise it looks good to me

ChangeLog Outdated
@@ -62,6 +62,12 @@ Changes
mbedtls_ssl_get_record_expansion() in case of ChachaPoly ciphersuites,
or CBC ciphersuites in (D)TLS versions 1.1 or higher. Fixes #1913, #1914.
* Add support for buffering of out-of-order handshake messages.
* Extend RSASS-PSS signature to allow slightly a smaller salt size.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be RSASSA-PSS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, will fix via fixing up the original commit.

@Patater
Copy link
Contributor

Patater commented Sep 12, 2018

Rebased with typo fixed in ChangeLog

Previous branch at https://github.com/Patater/mbedtls/tree/dev/Patater/salty-pss-6

dgreen-arm
dgreen-arm previously approved these changes Sep 12, 2018
Copy link
Contributor

@dgreen-arm dgreen-arm left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Re-approving after a typo fix in the changelog entry.

@Patater Patater added mbed TLS team approved Design and code approved - may be waiting for CI or backports and removed Arm Contribution labels Sep 12, 2018
@simonbutcher
Copy link
Contributor

This is failing all.sh, so demoting to 'needs work'.

Extract from all.sh:

******************************************
* Testing without hash: MBEDTLS_SHA512_C
******************************************
  CC    aes.c
  CC    aesni.c
...snip!...
  Gen   test_suite_pk.c
  CC    test_suite_pk.c
test_suite_aes.cbc ................................................ PASS
test_suite_aes.cfb ................................................ PASS
test_suite_aes.ecb ................................................ PASS
test_suite_aes.ofb ................................................ PASS
test_suite_aes.rest ............................................... PASS
test_suite_aes.xts ................................................ PASS
test_suite_arc4 ................................................... PASS
test_suite_aria ................................................... PASS
test_suite_asn1write .............................................. PASS
test_suite_base64 ................................................. PASS
test_suite_blowfish ............................................... PASS
test_suite_camellia ............................................... PASS
test_suite_ccm .................................................... PASS
test_suite_chacha20 ............................................... PASS
test_suite_chachapoly ............................................. PASS
test_suite_cipher.aes ............................................. PASS
test_suite_cipher.arc4 ............................................ PASS
test_suite_cipher.blowfish ........................................ PASS
test_suite_cipher.camellia ........................................ PASS
test_suite_cipher.ccm ............................................. PASS
test_suite_cipher.chacha20 ........................................ PASS
test_suite_cipher.chachapoly ...................................... PASS
test_suite_cipher.des ............................................. PASS
test_suite_cipher.gcm ............................................. PASS
test_suite_cipher.null ............................................ PASS
test_suite_cipher.padding ......................................... PASS
test_suite_cmac ................................................... PASS
test_suite_ctr_drbg ............................................... PASS
test_suite_debug .................................................. PASS
test_suite_des .................................................... PASS
test_suite_dhm .................................................... PASS
test_suite_ecdh ................................................... PASS
test_suite_ecdsa .................................................. PASS
test_suite_ecjpake ................................................ PASS
test_suite_ecp .................................................... PASS
test_suite_entropy ................................................ PASS
test_suite_error .................................................. PASS
test_suite_gcm.aes128_de .......................................... PASS
test_suite_gcm.aes128_en .......................................... PASS
test_suite_gcm.aes192_de .......................................... PASS
test_suite_gcm.aes192_en .......................................... PASS
test_suite_gcm.aes256_de .......................................... PASS
test_suite_gcm.aes256_en .......................................... PASS
test_suite_gcm.camellia ........................................... PASS
test_suite_hkdf ................................................... PASS
test_suite_hmac_drbg.misc ......................................... PASS
test_suite_hmac_drbg.no_reseed .................................... PASS
test_suite_hmac_drbg.nopr ......................................... PASS
test_suite_hmac_drbg.pr ........................................... PASS
test_suite_md ..................................................... PASS
test_suite_mdx .................................................... PASS
test_suite_memory_buffer_alloc .................................... PASS
test_suite_mpi .................................................... PASS
test_suite_nist_kw ................................................ PASS
test_suite_pem .................................................... PASS
test_suite_pk ..................................................... PASS
test_suite_pkcs1_v15 .............................................. PASS
test_suite_pkcs1_v21 .............................................. FAIL
test_suite_pkcs5 .................................................. PASS
test_suite_pkparse ................................................ PASS
test_suite_pkwrite ................................................ PASS
test_suite_poly1305 ............................................... PASS
test_suite_rsa .................................................... PASS
test_suite_shax ................................................... PASS
test_suite_ssl .................................................... PASS
test_suite_timing ................................................. PASS
test_suite_version ................................................ PASS
test_suite_x509parse .............................................. PASS
test_suite_x509write .............................................. PASS
test_suite_xtea ................................................... PASS
------------------------------------------------------------------------
FAILED (70 suites, 6176 tests run)
Makefile:117: recipe for target 'check' failed
make[1]: *** [check] Error 1
Makefile:81: recipe for target 'check' failed
make: *** [check] Error 2
Failed test suite: MBEDTLS_SHA512_C

@simonbutcher simonbutcher added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Sep 27, 2018
It should be valid to RSASSA-PSS sign a SHA-512 hash with a 1024-bit or
1032-bit RSA key, but with the salt size being always equal to the hash
size, this isn't possible: the key is too small.

To enable use of hashes that are relatively large compared to the key
size, allow reducing the salt size to no less than the hash size minus 2
bytes. We don't allow salt sizes smaller than the hash size minus 2
bytes because that too significantly changes the security guarantees the
library provides compared to the previous implementation which always
used a salt size equal to the hash size. The new calculated salt size
remains compliant with FIPS 186-4.

We also need to update the "hash too large" test, since we now reduce
the salt size when certain key sizes are used. We used to not support
1024-bit keys with SHA-512, but now we support this by reducing the salt
size to 62. Update the "hash too large" test to use a 1016-bit RSA key
with SHA-512, which still has too large of a hash because we will not
reduce the salt size further than 2 bytes shorter than the hash size.

The RSA private key used for the test was generated using "openssl
genrsa 1016" using OpenSSL 1.1.1-pre8.

    $ openssl genrsa 1016
    Generating RSA private key, 1016 bit long modulus (2 primes)
    ..............++++++
    ....++++++
    e is 65537 (0x010001)
    -----BEGIN RSA PRIVATE KEY-----
    MIICVwIBAAKBgACu54dKTbLxUQBEQF2ynxTfDze7z2H8vMmUo9McqvhYp0zI8qQK
    yanOeqmgaA9iz52NS4JxFFM/2/hvFvyd/ly/hX2GE1UZpGEf/FnLdHOGFhmnjj7D
    FHFegEz/gtbzLp9X3fOQVjYpiDvTT0Do20EyCbFRzul9gXpdZcfaVHNLAgMBAAEC
    gYAAiWht2ksmnP01B2nF8tGV1RQghhUL90Hd4D/AWFJdX1C4O1qc07jRBd1KLDH0
    fH19WocLCImeSZooGCZn+jveTuaEH14w6I0EfnpKDcpWVAoIP6I8eSdAttrnTyTn
    Y7VgPrcobyq4WkCVCD/jLUbn97CneF7EHNspXGMTvorMeQJADjy2hF5SginhnPsk
    YR5oWawc6n01mStuLnloI8Uq/6A0AOQoMPkGl/CESZw+NYfe/BnnSeckM917cMKL
    DIKAtwJADEj55Frjj9tKUUO+N9eaEM1PH5eC7yakhIpESccs/XEsaDUIGHNjhctK
    mrbbWu+OlsVRA5z8yJFYIa7gae1mDQJABjtQ8JOQreTDGkFbZR84MbgCWClCIq89
    5R3DFZUiAw4OdS1o4ja+Shc+8DFxkWDNm6+C63g/Amy5sVuWHX2p9QI/a69Cxmns
    TxHoXm1w9Azublk7N7DgB26yqxlTfWJo+ysOFmLEk47g0ekoCwLPxkwXlYIEoad2
    JqPh418DwYExAkACcqrd9+rfxtrbCbTXHEizW7aHR+fVOr9lpXXDEZTlDJ57sRkS
    SpjXbAmylqQuKLqH8h/72RbiP36kEm5ptmw2
    -----END RSA PRIVATE KEY-----
Since we wish to generate RSASSA-PSS signatures even when hashes are
relatively large for the chosen RSA key size, we need some tests. Our
main focus will be on 1024-bit keys and the couple key sizes larger than
it. For example, we test for a signature generated using a salt length
of 63 when a 1032-bit key is used. Other tests check the boundary
conditions around other key sizes. We want to make sure we don't use a
salt length larger than the hash length (because FIPS 186-4 requires
this). We also want to make sure we don't use a salt that is too small
(no smaller than 2 bytes away from the hash length).

Test RSASSA-PSS signatures with:
 - 1024-bit key and SHA-512 (slen 62)
 - 1032-bit key and SHA-512 (slen 63)
 - 1040-bit key and SHA-512 (slen 64)
 - 1048-bit key and SHA-512 (slen 64)

The tests also verify that we can properly verify the RSASSA-PSS
signatures we've generated.

We've manually verified that OpenSSL 1.1.1-pre8 can verify the
RSASSA-PSS signatures we've generated.

    $ openssl rsa -in rsa1024.pem -pubout -out pub1024.pem
    writing RSA key
    $ openssl rsa -in rsa1032.pem -pubout -out pub1032.pem
    writing RSA key
    $ openssl rsa -in rsa1040.pem -pubout -out pub1040.pem
    writing RSA key
    $ openssl rsa -in rsa1048.pem -pubout -out pub1048.pem
    writing RSA key
    $ cat message.bin | openssl dgst -sha512 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:62 -verify pub1024.pem -signature valid1024.bin
    Verified OK
    $ cat message.bin | openssl dgst -sha512 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:63 -verify pub1032.pem -signature valid1032.bin
    Verified OK
    $ cat message.bin | openssl dgst -sha512 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:64 -verify pub1040.pem -signature valid1040.bin
    Verified OK
    $ cat message.bin | openssl dgst -sha512 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:64 -verify pub1048.pem -signature valid1048.bin
    Verified OK

We've also added a new test that ensures we can properly validate a
RSASSA-PSS 1032-bit signature with SHA-512 generated by OpenSSL. This
has been added as the "RSASSA-PSS Verify OpenSSL-generated Signature
1032-bit w/SHA-512" test. The signature to verify was generated with the
following command line.

    $ cat message.bin | openssl dgst -sha512 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:63 -sign rsa1032.pem > valid.bin

The RSA private keys used by these tests were generated with OpenSSL
1.1.1-pre8.

    $ openssl genrsa 1024
    Generating RSA private key, 1024 bit long modulus (2 primes)
    ........................................++++++
    ......++++++
    e is 65537 (0x010001)
    -----BEGIN RSA PRIVATE KEY-----
    MIICWwIBAAKBgQDDyYc1SFQ1kcH5R+QSwz2la50blKWML0EKimIOm08dkZdkPr9S
    f19isgK51noyZU0F8yapth4BBu/fSClnPE89I2VZluJCQFmRarR6pn5AbBKWeeWX
    nKRnCIZmCP+iH2GYQ7lZtEQuQiWYovqrVKjO8fExmSZ30s9byvK1Vk90GQIDAQAB
    AoGAG1BnO4i+rsaJ8DQWXoO8evJ7dZiUS+1fvo+1xGHodLCWFVcnq+O3M/avqKuC
    WruFNlpIv453ux7zogvYMt3YE+ny//kgh5gUh0O1mXPbZtF4gGxsqXdV13lMW9dK
    ZH2ltN94MwynrXl74m2P4uCHWIHLE9+ZyWRzwH/c/o1E4n0CQQDo+VpxbBJ9UUfc
    wkGnwf6NVIez6LbpXkioMzTSHQDHmtCpDimUHAxTBlsgBZ3pXp5AYGFBb3rBLtyh
    mDue4ozDAkEA1yNIspfn5dxDKfarh0sXmCWE4KtDF0Bwqb6YPA8EAyDW+JPEDScX
    yzBEOAyzIwtxM2IescVaPqVtDnzuaUtd8wJAdOP3XwUsWbgYaEkHDBank12gIMJY
    U8q8hbf7fpiStZOVsdyrO+a+wEFmIzDuRBL3L7Gr2lsGqjrK9EEfWN6uZQJAOJml
    1Ka2cfkDCpVFB3EwIe0tClbEbeecPGxSbOqeaZxIMlnd6H/yeJiYOg7NSlkGTThx
    Tt/XIEgxavBfYQBdFQJAMFmLr9DL5lWAZNAHspJ8R5NdoOcsrKV9tb24cu0YapuZ
    rSPwmebskHyinvoBsD2CthUtpSo3NE+xZ6HcfYca9w==
    -----END RSA PRIVATE KEY-----

    $ openssl genrsa 1032
    Generating RSA private key, 1032 bit long modulus (2 primes)
    ....................++++++
    .................................++++++
    e is 65537 (0x010001)
    -----BEGIN RSA PRIVATE KEY-----
    MIICYAIBAAKBggCqlKuRtMJr4lfkaVKCKMSwtrTJnnOoSicrMQGJLAdAaRE3K4Ps
    SnuBkfC6S0y0yztzIHTpbGaCl+EyO4rQgip+FRGC3vA4caZqR7cEuShFxhlBQtTu
    2hmQPgQENYH3qDXcKIEXhj0hlEw67e1RhFjxowpBx2OKpOCYqI/fLCCXJw0CAwEA
    AQKBgWcY+CFWePOvl6OrrHySm16a7uW06P5b4xSNx/naLH/XgNoxaMiVs9P6Gt7d
    x/y1oLbSdRbnt4VSun8b0ah4I6qEyk3MdfiNnhy24LlCuv0TOLbQVibjH+5Q+iP2
    995ssUrkfPa/QAA95nPVaDhcsATSh32JagE0rkItukalyc/IGQJBDfrttwmtohBS
    I+XndkpfMdB656N73HtKVsJJnhFzFHvNyxZbj7AaJSgZDLaHRlapNkkYmPyjMNuK
    9antVBcmjtcCQQwznFZ5epDGQSklYNDvZ19xrCyZ/KumJgw45PFn39F563qeJV+b
    28VJ5BgfmioZsfMKgLKS1e8a11ueZY6qb7C7AkEExSw7mmfOtrbwXNAfwry8qKBn
    TZdD4iW5eM3Zy6ZyxNOxik1vt+0T5Jy3g8igrY1LYqGsAfhFAYRm0raSTNvxPQJB
    AUcOIYfKq4n2nKZLQtUuT7IJQwpEiHx3E2SJpDUqHDbVzxrr8tzQ4BFijpwQekQC
    e94np4r0V3rJ/c/R9mQmGa0CQQrd2veAgj1F7Rma7zE4vYhvCf0XB1rshw972xGo
    BTAU4BagC7/vht1YXhhdz1FC36DrWm3veTwLLuNUQTJWsYIH
    -----END RSA PRIVATE KEY-----

    $ openssl genkey 1040
    Generating RSA private key, 1040 bit long modulus
    ........++++++
    ........++++++
    e is 65537 (0x10001)
    -----BEGIN RSA PRIVATE KEY-----
    MIICZgIBAAKBgwDSNAU4Ix3NWmHt+Dq5Sy5LOnhDlMTtNaQkwFDClBV7diX5rKgl
    jCHi0Keqm3ydtXZATmMJDbpQ2Zj5o+xysaXPKNgyUauTNBx9LBqQQD1w9nvBqeQT
    vGL6zMtSRB4kw/K8n97KGngwEucLlSgXYmBYDE4QJsWCCejcxN478/W+VWXpAgMB
    AAECgYMAutjQ1uCoKhSwPgbLtE92vBoiMvh3v99Ro/VrFDrriY4xHWlzIcUZjfMp
    Rsblk45sqabD85VHS3zQtP8YO69bkvK+r5upGfzLtzX8r5BVuO1+7oO+/jbRHLYJ
    ieBoFZbUc27YcKzR35Iv78d+KjygYIsWgt7W2Yqf6qt98r43WcrYsQJCAPx/S0kL
    TT73Kdsj+1r7tfL8YgpHI0LYuP8xDP3BJL523CKrb0vjWjjd0x8k1/ZNMQ9nqzo3
    XoP04FWeTLXcQ+h1AkIA1R6GgKtx3AHhqKaKKYY2uxZYz6uNc85SimJpdyLUhauQ
    za/F4ndot2GDn/k0IEWK5V8VppRl28DHtSTcmjhf+SUCQT4RVIJaItztiP5zc+BD
    q9BVNgxsvEA8Yg1pE1Z1WgDv2uEy3yL6ej0sWi93sRa8lujAhRjEb5lkYpjpVtYF
    lTPZAkIAlt1yB3nWMxEd5l5mZbi927iZDAF8M+N1aML7t7tvSGTIL+LjKUqwVUhx
    ffhSXxn7lh22XOKmLGcOuHussnt/7QUCQS2GWdSBu7DGZ5uIvZVj+5KPTwv3Pw9+
    uuj12Z3shojl1iVije7nsBK1q3NbrXksEiQ4QJIoK2V2quqD58O2/K8T
    -----END RSA PRIVATE KEY-----

    $ openssl genrsa 1048
    Generating RSA private key, 1048 bit long modulus (2 primes)
    ...............................++++++
    .++++++
    e is 65537 (0x010001)
    -----BEGIN RSA PRIVATE KEY-----
    MIICaQIBAAKBhADHXQ+foX0dJLk5U3pDQBfzkMZgRETDWhM2DWsfyYa69AFZuEJ1
    03uIMnjfUGTdnrDymw0yWsx5DEtZZyc32786y4j14vLVTJGcr9ByJyxJRZHVLhWJ
    kzFeceLKYLHHT+/489d4QrQV1OcXNKSYIGpc2TFch7I+WD4l60ypcFa0XJaFbQID
    AQABAoGDSqKtUa6sXze7XBnDYN/i151wluOX9qaHIKo/W4Qfu2fUBZm0z9Wfnqp+
    k+PODyX0yq5/b0WM3RhcMRksFn5fBgzYHEmAj8IHhDsjavNtiv8nIl6EF2PfuT1p
    6iEpo8IS15dp6j5AKH4Zmnq6TRYiqdaz/ry/kpQrmeJym83KksujZWUCQg85t5gJ
    UWvswuNIG2tHWEqiKZvSAnq4owO53lsK3LSl04447bjB+sPqHb1+HVC4QyPjYs/0
    3z9aUYLa+pu5IXpz1wJCDRgWT4vQ1Y0BmZjIyxfEwDVOYripRirMowgWiU+YLCrh
    FOc5k+MGmJMEN7TuxErewk0yzLy658xMn4kRseshAGhbAkIBClKPIuPbWfwfB4hI
    FkHkJ5xsNzdQJ1mMIaEd22olNcd0ylMD8s0tocuSbRGXuF9uDlVsHDE85PD43fmN
    tmKhOVUCQgG6H5c2VcEU7BUaNcGzzNudLE2RFaKPmpYWRwKtYODSdwWOyeVbmE8f
    dPrz/lodlewCyqR+cBiKtcCFD7Rr0tp+6QJCALZlz954leZ6UKkdeOiTb+fVFpsq
    DNNALCL4VJ7XcJJMpjgSKYv9sr1C43nifr1M4YDH+B3NFRF+FWdVodaseOKF
    -----END RSA PRIVATE KEY-----
Add signing tests with 528-bit and 520-bit RSA keys with SHA-512. These
selections of key and hash size should lead to an error returned, as
there is not enough room for our chosen minimum salt size of two bytes
less than the hash size. These test the boundary around an available
salt length of 0 or -1 bytes.

The RSA keys were generated with OpenSSL 1.1.1-pre8.

    $ openssl genrsa 520
    Generating RSA private key, 520 bit long modulus (2 primes)
    .............++++++++++++
    .................++++++++++++
    e is 65537 (0x010001)
    -----BEGIN RSA PRIVATE KEY-----
    MIIBPwIBAAJCANWgb4bludh0KFQBZcqWb6iJOmLipZ0L/XYXeAuwOfkWWjc6jhGd
    B2b43lVnEPM/ZwGRU7rYIjd155fUUdSCBvO/AgMBAAECQgDOMq+zy6XZEjWi8D5q
    j05zpRGgRRiKP/qEtB6BWbZ7gUV9DDgZhD4FFsqfanwjWNG52LkM9D1OQmUOtGGq
    a9COwQIhD+6l9iIPrCkblQjsK6jtKB6zmu5NXcaTJUEGgW68cA7PAiENaJGHhcOq
    /jHqqi2NgVbc5kWUD/dzSkVzN6Ub0AvIiBECIQIeL2Gw1XSFYm1Fal/DbQNQUX/e
    /dnhc94X7s118wbScQIhAMPVgbDc//VurZ+155vYc9PjZlYe3QIAwlkLX3HYKkGx
    AiEND8ndKyhkc8jLGlh8aRP8r03zpDIiZNKqCKiijMWVRYQ=
    -----END RSA PRIVATE KEY-----

    $ openssl genrsa 528
    Generating RSA private key, 528 bit long modulus (2 primes)
    .........++++++++++++
    ....++++++++++++
    e is 65537 (0x010001)
    -----BEGIN RSA PRIVATE KEY-----
    MIIBQgIBAAJDAKJVTrpxW/ZuXs3z1tcY4+XZB+hmbnv1p2tBUQbgTrgn7EyyGZz/
    ZkkdRUGQggWapbVLDPXu9EQ0AvMEfAsObwJQgQIDAQABAkJhHVXvFjglElxnK7Rg
    lERq0k73yqfYQts4wCegTHrrkv3HzqWQVVi29mGLSXTqoQ45gzWZ5Ru5NKjkTjko
    YtWWIVECIgDScqoo7SCFrG3zwFxnGe7V3rYYr6LkykpvczC0MK1IZy0CIgDFeINr
    qycUXbndZvF0cLYtSmEA+MoN7fRX7jY5w7lZYyUCIUxyiOurEDhe5eY5B5gQbJlW
    ePHIw7S244lO3+9lC12U1QIhWgzQ8YKFObZcEejl5xGXIiQvBEBv89Y1fPu2YrUs
    iuS5AiFE64NJs8iI+zZxp72esKHPXq/chJ1BvhHsXI0y1OBK8m8=
    -----END RSA PRIVATE KEY-----
@Patater Patater force-pushed the IOTCRYPT-256-rsa-sign-1024-large-hash branch from b6b50c1 to 65593d2 Compare September 27, 2018 17:24
@Patater
Copy link
Contributor

Patater commented Sep 27, 2018

Updated tests to have explicit dependency oh SHA-512 where needed.

Previous branch at https://github.com/Patater/mbedtls/tree/dev/Patater/salty-pss-7

@Patater Patater added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 27, 2018
@Patater
Copy link
Contributor

Patater commented Sep 27, 2018

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

The difference between my previous approval at b6b50c1 and the current head at 65593d2 is the addition of missing dependencies in the tests. Approved.

@Patater
Copy link
Contributor

Patater commented Sep 28, 2018

Release job says it passed.

@Patater Patater added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 29, 2018
@simonbutcher simonbutcher merged commit 65593d2 into Mbed-TLS:development Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants