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

Providing salt length input to RSA_SSA_PSS verifty message via PSA APIs #4946

Closed
akshayupendran opened this issue Sep 15, 2021 · 5 comments · Fixed by #5010
Closed

Providing salt length input to RSA_SSA_PSS verifty message via PSA APIs #4946

akshayupendran opened this issue Sep 15, 2021 · 5 comments · Fixed by #5010
Assignees
Labels
bug component-crypto Crypto primitives and low-level interfaces enhancement size-s Estimated task size: small (~2d)

Comments

@akshayupendran
Copy link

akshayupendran commented Sep 15, 2021

Suggested enhancement

I am trying to use PSA APIs to implement RSASSA_PSS singature verification.
I am referring to the documentation PSA Cryptography API 1.0

I am generating a signature with a salt length(slen) = hash_len.
When I try to verify this signature via RSASSA-PSS, I find that the salt length check is altogether skipped.
I am calling:

                status = psa_verify_message(key, PSA_ALG_RSA_PSS(PSA_ALG_SHA_256), p_PlainTextPtr_pU8,
                                            p_PlainTextLength_U32, p_SigPtr_pU8, p_SigLength_U32);

This internally sets some PSA elements and calls:

    return( mbedtls_rsa_rsassa_pss_verify_ext( ctx, f_rng, p_rng, mode,
                                       md_alg, hashlen, hash,
                                       mgf1_hash_id, MBEDTLS_RSA_SALT_LEN_ANY,
                                       sig ) );

When I read the documentation, in the description of PSA_ALG_RSA_PSS (macro), there is a generic statement:

The salt length is equal to the length of the hash.

Hence can we instead of avoiding the salt length check altogether, can we either accept salt length as an argument to a PSA API or at least check for salt_len=hash_len.

Justification

Mbed TLS needs this because RSASSA_PSS is slowing gaining pace and we would require a salt length check in PSA APIs so that we do not revert back to MBEDTLS only APIs like mbedtls_rsa_rsassa_pss_verify_ext.

@gilles-peskine-arm
Copy link
Contributor

We had an internal discussion on the PSS salt length and decided to go into a different direction for the salt length. The soon-to-be-published draft of version 1.0.2 of the PSA Crypto API specification states:

  • When creating a signature, the salt length is equal to the length of the hash, or the largest possible salt length for the algorithm and key size if that is smaller than the hash length.
  • When verifying a signature, any salt length permitted by the RSASSA-PSS signature algorithm is accepted.

We decided to go this route because the salt is not a critical security parameter in PSS: I'm not aware of any reason to prefer a salt other than a slightly tighter proof. PSA implementations need to be able to verify PSS signatures produced by a non-PSA implementation, and without salt. Another reason we went this route is that it matches the behavior of the reference implementation.

Why do you want to verify the salt length? Do you have a compliance requirement to reject other salt lengths?

If there is demand for it, we may add a variant of PSA_ALG_RSA_PSS for which verification requires a specific salt length (probably just the hash-size-up-to-what-fits, and not the flexibility of arbitrary salt lengths, because that's what gets used in practice).

@gilles-peskine-arm
Copy link
Contributor

I pushed a documentation fix (because in any case, it's a bug that the documentation and the behavior of Mbed TLS do not match): #4949

@akshayupendran
Copy link
Author

Hello @gilles-peskine-arm

Thanks a lot for the prompt response and the quick fix of the documentation.

Why do you want to verify the salt length? Do you have a compliance requirement to reject other salt lengths?
Sadly we do have a compliance requirement to check if salt_length=hash_length. The test case for the requirements requests us to check if at least salt_len !=0. I do not have the expertise to know the background behind the compliance requirements.

If there is any possiblity of the salt check to be added as a variant of PSA_ALG_RSA_PSS please do keep me informed,
As this is not something which can be added immediately, I would temporarily change the rsa.c in my fork to adapt to this requirement.

To be precise i would change the function mbedtls_rsa_rsassa_pss_verify in rsa.c from:

    return( mbedtls_rsa_rsassa_pss_verify_ext( ctx, f_rng, p_rng, mode,
                                       md_alg, hashlen, hash,
                                       mgf1_hash_id, MBEDTLS_RSA_SALT_LEN_ANY,
                                       sig ) );

to:

    return( mbedtls_rsa_rsassa_pss_verify_ext( ctx, f_rng, p_rng, mode,
                                       md_alg, hashlen, hash,
                                       mgf1_hash_id, hashlen,
                                       sig ) );

P.S:
If there is a demand for the requirement and it is being added, I would love to get a notification. Thank you so much for your time and support.

@gilles-peskine-arm
Copy link
Contributor

We had an internal discussion and we've decided to change the upcoming version of the specification. On closer inspection, we felt that it was too big a change. The current published specification is:

The salt length is equal to the length of the hash.

We intend to amend that to something like:

The salt length is equal to the length of the hash if possible, or the largest possible salt length for the algorithm and key size if that is smaller than the hash length.

And we'll add a variant, tentatively called PSA_ALG_RSA_PSS_ANY_SALT, which has the same signature behavior but allows any salt (including an empty salt) during verification.

For Mbed TLS, this means that the current behavior will become the implementation of PSA_ALG_RSA_PSS_ANY_SALT, and we'll change PSA_ALG_RSA_PSS to be strict on verification.

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Sep 20, 2021

This is a small behavior change which is applicable to Mbed TLS 2.2x so we should schedule it in time for the 2.2x LTS (anticipating the official release of the updated PSA specification).

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Oct 4, 2021
PSA_ALG_RSA_PSS algorithm now accepts only the same salt length for
verification that it produces when signing, as documented.

Fixes Mbed-TLS#4946.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm self-assigned this Oct 4, 2021
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Oct 5, 2021
PSA_ALG_RSA_PSS algorithm now accepts only the same salt length for
verification that it produces when signing, as documented.

Fixes Mbed-TLS#4946.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Oct 5, 2021
PSA_ALG_RSA_PSS algorithm now accepts only the same salt length for
verification that it produces when signing, as documented.

Fixes Mbed-TLS#4946.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Oct 27, 2021
PSA_ALG_RSA_PSS algorithm now accepts only the same salt length for
verification that it produces when signing, as documented.

Fixes Mbed-TLS#4946.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Oct 28, 2021
PSA_ALG_RSA_PSS algorithm now accepts only the same salt length for
verification that it produces when signing, as documented.

Fixes Mbed-TLS#4946.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@mpg mpg closed this as completed in #5010 Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-crypto Crypto primitives and low-level interfaces enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants