-
Notifications
You must be signed in to change notification settings - Fork 157
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
Integrate OAEP and PSS into existing interface #43
Conversation
Removes the PublicKey trait, using Deref to provide the RSAPublicKey methods to RSAPrivateKey.
Test assertion is manual and done with openssl
Hmm, pulling in 2462f1d from #18 goes in the opposite direction to #42. It looks like the main thing it does (remove How painful would it be to just drop that commit entirely? It would likely make the history clearer. |
Hmm, the change in that commit to store an |
Yeah, I changed a lot of things during the merge, which is..unfortunate. So at this point I am thinking of squashing this into two commits at the end likely (1) for oaep and (2) for pss, after doing/undoing all things we want to do. |
@str4d I made the OAEP and PSS impls generic over the Key traits now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey that looks great! I have a couple suggestions
write!(f, "PaddingScheme::PKCS1v15({:?})", hash) | ||
} | ||
PaddingScheme::OAEP { ref label, .. } => { | ||
// TODO: How to print the digest name? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could make it Box<dyn DynDigest + Debug>
if really necessary. AFAIK all the digest crates have debug unconditionally implemented on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that doesn't work, the compiler is upset about more than one non auto trait :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, it's possible to make your own trait (e.g. trait MyDynDigest: DynDigest + Debug {}
) and then use that as a Box<dyn MyDynDigest>
, but at this point it's too much boilerplate just for the sake of getting the digest name to show IMO.
Relevant to the PSS use case: final two week comment period for shipping |
@@ -94,6 +97,13 @@ impl Drop for RSAPrivateKey { | |||
} | |||
} | |||
|
|||
impl Deref for RSAPrivateKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have PrivateKey: PublicKeyParts
, this impl Deref
should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, removing this, removes the ability to call .verify
on a private key
if pad_size < ciphertext.len() { | ||
return Err(Error::Verification); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check also be in raw_decryption_primitive
?
@str4d thanks for the review, I think I addressed all important things |
/// Encryption and Decryption using PKCS1v15 padding. | ||
PKCS1v15Encrypt, | ||
/// Sign and Verify using PKCS1v15 padding. | ||
PKCS1v15Sign { hash: Option<Hash> }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hash: None
case appears to still be necessary for unpadded signatures (at least, that's what the test case in src/pkcs1v15.rs
leads me to believe).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, which is why it is still an Option
, not sure what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just noting this because it wasn't immediately obvious during review that the Option<Hash>
was necessary besides enabling the prior encryption case to specify None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a close review pass over the RSAES-OAEP implementation, comparing it against RFC 8017 section 7.1.
Co-Authored-By: str4d <[email protected]>
@str4d applied the new round of CR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are causing the current CI failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a close review pass over the RSASSA-PSS implementation. The relevant sections of RFC 8017 were already inlined as comments.
None => (0..=em_len - (h_len + 2)) | ||
.rev() | ||
.try_fold(None, |state, i| match (state, db[em_len - h_len - i - 2]) { | ||
(Some(i), _) => Ok(Some(i)), | ||
(_, 1) => Ok(Some(i)), | ||
(_, 0) => Ok(None), | ||
_ => Err(Error::Verification), | ||
})? | ||
.ok_or(Error::Verification)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there precedent for detecting the salt length? I know it is possible due to the structure of DB
, but there is no mention of doing so in RFC 8017. I can't read the IEEE documents that reference different possible salt lengths, which maybe discusses this. As is, we never exercise the explicit-salt-length case below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know @roblabla I believe you wrote this code initially, do you have any answer to this?
salt_len: Option<usize>, | ||
digest: &mut dyn DynDigest, | ||
) -> Result<Vec<u8>> { | ||
let salt_len = salt_len.unwrap_or_else(|| priv_key.size() - 2 - digest.output_size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there precedent for this default salt length? The RFC mentions that common choices are digest.output_size()
and 0
, while this is selecting the salt to be as large as could fit. I can't read the IEEE documents that reference different possible salt lengths. In any case, I don't think there's a problem with this, given that we are detecting the salt length in verify
.
// 1. If the length of M is greater than the input limitation for the | ||
// hash function (2^61 - 1 octets for SHA-1), output "message too | ||
// long" and stop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Length is not checked, but with the current API we never actually see the message.
// 1. If the length of M is greater than the input limitation for the | ||
// hash function (2^61 - 1 octets for SHA-1), output "inconsistent" | ||
// and stop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Length is not checked, but with the current API we never actually see the message.
src/pss.rs
Outdated
// 5. Let maskedDB be the leftmost emLen - hLen - 1 octets of EM, and | ||
// let H be the next hLen octets. | ||
let (db, h) = em.split_at_mut(em_len - h_len - 1); | ||
let h = &mut h[..(em_len - 1) - (em_len - h_len - 1)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be clearer (and closer to the spec):
let h = &mut h[..(em_len - 1) - (em_len - h_len - 1)]; | |
let h = &mut h[..h_len]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
None => (0..=em_len - (h_len + 2)) | ||
.rev() | ||
.try_fold(None, |state, i| match (state, db[em_len - h_len - i - 2]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None => (0..=em_len - (h_len + 2)) | |
.rev() | |
.try_fold(None, |state, i| match (state, db[em_len - h_len - i - 2]) { | |
None => (0..em_len - h_len - 1) | |
.try_fold(None, |state, i| match (state, db[i]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change results in failures, haven't debugged why, gonna leave the logic as is for now
src/pss.rs
Outdated
for e in &db[..em_len - h_len - s_len - 2] { | ||
if *e != 0x00 { | ||
return Err(Error::Verification); | ||
} | ||
} | ||
if db[em_len - h_len - s_len - 2] != 0x01 { | ||
return Err(Error::Verification); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for e in &db[..em_len - h_len - s_len - 2] { | |
if *e != 0x00 { | |
return Err(Error::Verification); | |
} | |
} | |
if db[em_len - h_len - s_len - 2] != 0x01 { | |
return Err(Error::Verification); | |
} | |
let (zeroes, rest) = db.split_at(em_len - h_len - s_len - 2); | |
if zeroes.iter().any(|e| *e != 0x00) || rest[0] != 0x01 { | |
return Err(Error::Verification); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/pss.rs
Outdated
let h0 = hash.result_reset(); | ||
|
||
// 14. If H = H', output "consistent." Otherwise, output "inconsistent." | ||
if Into::<bool>::into(h0.ct_eq(h)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Into::<bool>::into(h0.ct_eq(h)) { | |
if h0.ct_eq(h).into() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/pss.rs
Outdated
return Err(Error::Internal); | ||
} | ||
|
||
let (db, h) = em.split_at_mut(em_len - s_len - h_len - 2 + 1 + s_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let (db, h) = em.split_at_mut(em_len - s_len - h_len - 2 + 1 + s_len); | |
let (db, h) = em.split_at_mut(em_len - h_len - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
// 8. Let DB = PS || 0x01 || salt; DB is an octet string of length | ||
// emLen - hLen - 1. | ||
db[em_len - s_len - h_len - 2] = 0x01; | ||
db[em_len - s_len - h_len - 1..].copy_from_slice(salt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're assuming here that the em
buffer passed into emsa_pss_encode
is already zeroed. This happens to be the case, but given that emsa_pss_encode
is only called from one place, and this PR isn't attempting to be no-std compatible, it would be clearer if em
was constructed and returned from this function. Non-blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
This integrates #18 and #26 by expanding the use of
PaddingScheme
to capture all optional values for the different schemes.It is not ideal, but it works well and keeps in line with the existing API for now. I think we should still continue to improve on the api and think through #34 more, but in the meantime this a version that folks can start to use, and isn't too different from the existing api