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

Remove 'unsalted' PSS handling #294

Merged
merged 3 commits into from
Apr 17, 2023
Merged

Conversation

lumag
Copy link
Contributor

@lumag lumag commented Apr 15, 2023

The 'unsalted' PSS handling is not behaving like one would expect. It doesn't use salt of 0 length, but insteaad it uses some defaults which are not properly documented. Make the salt_len mandatatory for both singing and verification.

Copy link
Member

@tarcieri tarcieri 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!

I don't understand the potential real-world usages of unsalted PSS, but from a theoretical perspective it makes no sense: the purpose of having a salt is to increase the tightness of the security proof. I don't understand why one would want to use PSS in a way which defeats the security proof.

So that said, I'm all for removing the unsalted API.

I think we could further improve the API by having one where the default salt length equals the output length of the digest function, but I'm happy to open a followup PR to add that.

@lumag
Copy link
Contributor Author

lumag commented Apr 15, 2023

@tarcieri sounds good to me.

cc @roblabla @dignifiedquire

@tarcieri
Copy link
Member

tarcieri commented Apr 15, 2023

@lumag alternatively, you could implement ^^^ proposed API in this PR, which would avoid the need to deprecate anything.

Instead, change new to be salted, but infer the salt size from the digest function.

(Or I'm still happy to do that as a followup, which would simplify the overall changes and help organize discussion perhaps)

@lumag
Copy link
Contributor Author

lumag commented Apr 15, 2023

Sure, I'll do that in a minute

Current new() and random() functions cause confusion. There is the
default from ASN.1 encoding of RSAPSS parameters (20). There is also
another default of (mod_size - 2 - hash_size). And there is a
recommendation to use salt_len of hash_size.

Drop old defaults and always use digest output size as the salt_len.
Clearly document new default.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
All RSA PSS standards (e.g. RFC 8017) clearly specify that RSA PSS
verification has an explicit salt length parameter (rather than
determining it from the message). Drop our 'automagic' code and pass
salt length when verifying the message. Old functions now default to
digest output size as a hash length.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
The emsa_pss_get_salt() is possibly non-constant-time op. Change it to
be a contant-time operation.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
@lumag
Copy link
Contributor Author

lumag commented Apr 15, 2023

done

@tarcieri tarcieri merged commit e7201ed into RustCrypto:master Apr 17, 2023
9 checks passed
@tarcieri tarcieri mentioned this pull request Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants