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

Fix pss sign/verify when key length is multiple of 8 + 1 bits. #263

Merged

Conversation

dfabregat
Copy link
Contributor

In the unusual case of having a key of length k*8 + 1 bits, em_len will be 1 byte shorter than key_len. The call to raw_encryption_primitive requires a buffer key_len bytes long.

Vice versa, we would be generating a one-byte too-long salt in generate_salt.

Finally, emsa_pss_verify_pre would fail by shifting 1 byte left by 8 bits.

@dignifiedquire
Copy link
Member

is there an actual use case for keys of that size, or should we just restrict the sizes?

@dfabregat
Copy link
Contributor Author

I personally do not remember encountering any special use case of this type. I've generally seen multiples of 512. On the other hand, OpenSSL, for instance, seems to support all key lengths (within whichever range they allow).

In the end, it is a design decision. I personally would rather support this case, but it is just my opinion. Alternatively, one can restrict and see if anyone ever raises a request for this. Then, we learn in which context would one have such a need :)

@dignifiedquire
Copy link
Member

@tarcieri any thoughts on this?

@tarcieri
Copy link
Member

I think it'd be good to eliminate panics regardless of what invariants we choose to uphold, and if we have _unchecked APIs, that means we don't really have invariants.

All that said, I think the default (checked) APIs should probably at least check the key size is an even number.

Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@dignifiedquire dignifiedquire merged commit 3061c9c into RustCrypto:master Feb 14, 2023
@dignifiedquire
Copy link
Member

@tarcieri agreed, merging this, as this in all cases improves the status quo

thanks for catching and fixing @dfabregat

@dfabregat
Copy link
Contributor Author

Always glad to contribute. And thank you guys for your feedback 😃

@tarcieri tarcieri mentioned this pull request Mar 2, 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.

3 participants