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

Create backwards-compatible RSA PSS sigs by default #431

Closed
wants to merge 3 commits into from
Closed

Create backwards-compatible RSA PSS sigs by default #431

wants to merge 3 commits into from

Conversation

trishankatdatadog
Copy link
Contributor

Fixes: #430

Description of the changes being introduced by the pull request:

This PR fixes #430 by creating RSA PSS signatures with a salt length that defaults to size of the output of the hash function used in the signature. End-user libraries/applications such as python-tuf and python-in-toto are able to override this default if so desired.

Please verify and check that the pull request fulfils the following requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@trishankatdatadog trishankatdatadog mentioned this pull request Sep 22, 2022
Merged
yzhan289
yzhan289 approved these changes Sep 22, 2022
View reviewed changes
Copy link

@yzhan289 yzhan289 left a comment

Choose a reason for hiding this comment

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

🚀

adityasaky
adityasaky approved these changes Sep 22, 2022
View reviewed changes
tests/test_rsa_keys.py Show resolved Hide resolved
@trishankatdatadog
Copy link
Contributor Author

Should we also update interface.(create|verify)_signature to pass a salt length?

@adityasaky
Copy link
Member

Should we also update interface.(create|verify)_signature to pass a salt length?

So that should go in securesystemslib.keys but I'm wondering if that should just stick to the default since this would only apply to RSA keys while the API is for all types of keys. What do you think?

@SantiagoTorres
Copy link
Collaborator

I wonder if we want to at all touch the saltlength. I'm unsure why the underlying api had to change (not in sslib, but cryptography). Is there any backstory to it?

@jku
Copy link
Collaborator

jku commented Sep 26, 2022

This PR fixes #430 by creating RSA PSS signatures with a salt length that defaults to size of the output of the hash function used in the signature. End-user libraries/applications such as python-tuf and python-in-toto are able to override this default if so desired.

So I'm trying to understand the complete picture:

  • originally we created sigs with salt length that matches digest length. Originally we verified sigs assuming salt length of digest length
  • then in Fix RSA PSS salt lengths #422 we started creating sigs with "maximum salt length" and verifying sigs with "automatic" salt length. This was not backwards compatible as old verifier code expects "digest length"
  • this PR creates sigs by default with digest length (so reverts to original), and verifies sigs by default with "automatic" (keeps the Fix RSA PSS salt lengths #422 functionality). In addition it passes the buck to caller by adding salt length as an argument

So end result seems to be

  • verifiers can now work with RSA PSS sigs produced with different salt length ✔️
  • theoretically callers can specify the salt length ... but will they?
    • for verify, shouldn't "automatic" always work? what is the purpose of specifying the length?
    • for sign, the same problem applies to calling code as you encountered: if old code can't handle different salt lengths then python-tuf and intoto can't change the default either, until all old code is dead and buried

So I suppose my question is, is the added API surface needed? Could sslib just create sigs like it used to originally before #422 (and file an issue for future maintainers to change it when old code really is dead) and just let verify always use automatic length?

@trishankatdatadog
Copy link
Contributor Author

  • for verify, shouldn't "automatic" always work? what is the purpose of specifying the length?

Reasons:

  1. To simplify testing.
  2. For end-users who are interested in fixing the salt length if they so wish (because I believe the salt length is attacker-controlled since it's part of the signature, although the impact is only a DoS attack unless there is some strange crypto attack which wouldn't surprise me).

Could sslib just create sigs like it used to originally before #422

That's exactly what the signing does by default now.

I don't see any harm in merging this PR as it is: users who override the defaults presumably know what they are doing. Any objections?

@trishankatdatadog
Copy link
Contributor Author

The biggest Q is whether we are OK with the default salt length (digest length). I think it's since, since nobody knows how to break even empty salt lengths apparently, but I think it's not a bad idea to allow those who know what they are doing to override defaults if they so wish.

OTOH, I'm not against simply for now: (1) fixing signatures to the digest length, and (2) setting salt length automatically during verification. Votes?

@lukpueh
Copy link
Member

lukpueh commented Oct 4, 2022

(1) fixing signatures to the digest length, and (2) setting salt length automatically during verification. Votes?

+1

@trishankatdatadog
Copy link
Contributor Author

+1

Ok, so no overriding of these defaults, correct?

@lukpueh
Copy link
Member

lukpueh commented Oct 5, 2022

I intend to create a release next week to get out a different feature (#231). Happy to include a fix for this issue here if provided.

@trishankatdatadog
Copy link
Contributor Author

I intend to create a release next week to get out a different feature (#231). Happy to include a fix for this issue here if provided.

@yzhan289 Could you please help take care of this? Thanks!

lukpueh added a commit to lukpueh/securesystemslib that referenced this pull request Oct 13, 2022
@lukpueh
Revert to digest sized salts in rsassa-pss signing
dd42718 
In secure-systems-lab#422 we changed `create_rsa_signature` to use the maximum salt
length available, instead of the digest length, when creating
rsassa-pss signatures, and adapted `verify_rsa_signature` to infer
the salt length automatically.

This made it impossible for users of the old verify function, which
could only handle digest sized salts, to verify signatures created
by users of the new signing function (see secure-systems-lab#430).

Since the advantage of max salt lengths is mostly academic, this
patch reverts  `create_rsa_signature` to use digest sized salts (as
agreed in secure-systems-lab#431).

Note that we now use the `padding.PSS.DIGEST_LENGTH` constant
instead of passing the actual digest length, as we did before.
Using the constant has the same result, but is recommended by the
library documentation.

Also note that the patch does not revert the `verify_rsa_signature`
part of secure-systems-lab#422. This allows verifying signatures created with the
securesystemslib release that used max salt lengths, or created
outside of securesystemslib.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh lukpueh mentioned this pull request Oct 13, 2022
Merged
@lukpueh
Copy link
Member

lukpueh commented Oct 13, 2022

Closing in favor of #436

@lukpueh lukpueh closed this Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SantiagoTorres SantiagoTorres SantiagoTorres left review comments

@adityasaky adityasaky adityasaky approved these changes

@yzhan289 yzhan289 yzhan289 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Maintain backwards-compatibility by default in RSA PSS signatures
6 participants
@trishankatdatadog @adityasaky @SantiagoTorres @jku @lukpueh @yzhan289