Implement RSA PSS verification and signing. #262
Conversation
|
Coverage increased (+0.04%) to 89.485% when pulling ec6e043bba170635191ca8c8848a77c34eda6092 on samscott89:padding into 0ab853a on briansmith:master. |
|
@djc, any chance you could take a look at this? I think you probably have some ideas already. |
| rsa_pss_padding!(RSA_PSS_SHA512, &digest::SHA512, | ||
| "Signing using RSA with PSS padding and SHA-512.", 0); | ||
|
|
||
| rsa_pss_padding!(RSA_PSS_SHA1_20, &digest::SHA1, |
There was a problem hiding this comment.
This is particularly tedious and not very flexible. Could make salt length an additional input to the encode/verify algorithms, but this is not something like to be shared by other padding schemes.
Alternatively, we enforce only a small number of options, e.g., salt length = digest length.
This current list is to cover all of the NIST tests in rsa_pss_{sign,verify}_tests.txt
There was a problem hiding this comment.
Yes, the salt length should always be equal to the digest function output length, and the digest for the MGF should always be the same digest used for the hash. Basically, we are constraining ourselves to what TLS 1.3 and other recent IETF specs require: #115 (comment)
There was a problem hiding this comment.
Simply omit the NIST test vectors that use a different salt length. Add a note to rsa_pss_{sign,verify}_tests.txt that they are omitted and why.
|
As is this is pretty hard to review. @samscott89, do you think it's feasible to split this into at least 4 commits? I see the following:
I think it's probably also time to put all RSA stuff in separate directory, so that's |
| -> Result<(), ()>; | ||
| } | ||
|
|
||
| pub struct PKCS1v15Padding { |
I agree
I also prefer this split, but I would prefer the tests for PSS verification to be included in the PSS verification commit, and for the tests for PSS signing to be in the PSS signing commit.
If we're going to have |
|
Thanks @djc for providing the great feedback. Also, thanks @samscott89 for an awesome contribution here. I hope my feedback above is enough for you to make forward progress. If not, let me know. |
|
|
||
| // 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.
Regarding these comments that quote the spec: At a minimum, let's get rid of the excess indention at the start and rewrap the text to 80 chars. However, I actually would prefer them to be abbreviated to just "Steps 1-3". The reader can read the actual steps in the spec. This way, we don't have to ensure that the text of the actual quotes are correct. See ring::ec::suite_b::ecdh::ecdh for an example of the style I mean.
|
Thank you both for the fantastic feedback, exactly what I was looking for. I'll have another go with the above in mind, and should have a somewhat nicer PR up shortly (by which I mean I'll update this thread). |
samscott89
force-pushed
the
padding
branch
2 times, most recently
from
c077a5e
to
9426ac1
Compare
|
Ok, that should be a bit easier to digest/read. I've assumed the length in bits of the RSA moduli is always a multiple of 8. Is there somewhere this can be enforced? |
|
Coverage decreased (-0.4%) to 89.662% when pulling 9426ac19559d5357b7a6461d3c8ccf37a3b9f7e6 on samscott89:padding into 5f7e741 on briansmith:master. |
| @@ -124,7 +124,7 @@ mod init; | |||
| pub mod pbkdf2; | |||
|
|
|||
| pub mod rand; | |||
| #[cfg(feature = "use_heap")] mod rsa; | |||
| #[path = "rsa/rsa.rs"] #[cfg(feature = "use_heap")] mod rsa; | |||
There was a problem hiding this comment.
This should probably go on multiple lines at this point.
I agree to license my contributions to each file under the terms given at the top of each file I changed.
- Add tests for RSA-PSS verification from NIST test cases. I agree to license my contributions to each file under the terms given at the top of each file I changed.
- Refactor Encoding trait to be randomised. - Add RSA-PSS signing test cases. I agree to license my contributions to each file under the terms given at the top of each file I changed.
|
Ok, I think I've addressed the suggestions/problems from before and added tests for PSS verification for moduli of variable bit lengths mod 8. |
2 similar comments
|
Changes Unknown when pulling 6b277dc on samscott89:padding into ** on briansmith:master**. |
|
gentle bump. I could really use this for rustls TLS1.3 support. |
| def parse(fn): | ||
| DIGEST_OUTPUT_LENGTHS = { | ||
| 'SHA1': 80, | ||
| 'SHA224': 112, |
There was a problem hiding this comment.
'SHA224` should be removed since it's not a supported algorithm and it is never used, IIUC.
| # Can only support moduli which are a whole number of bytes | ||
| if n.bit_length() % 8 != 0: | ||
| debug("Skipping due to modulus (not a whole number of bytes).", DEBUG) | ||
| return |
There was a problem hiding this comment.
Sorry, I gave you bad advice. In the review of #283 we did the math and found out that even if we require the private key primes p and q to be multiples of 8 bits or even multiples of 1024 bits, it's still possible and even pretty likely that the public modulus will be 2047 bits; i.e. n.bit_length() % 8 == 7. So, we in fact do need to support public moduli that aren't an even number of bytes, even for signing. Sorry!
| getattr(hashes, case['SHAAlg'])()) | ||
| hex_sig = ''.join('{:02x}'.format(ord(c)) for c in sig) | ||
| assert hex_sig == case['S'] | ||
| elif padding_alg != "PSS": |
There was a problem hiding this comment.
Please write this like this:
elif padding_alg == "PSS":
# PSS is randomised, can't recompute this way
pass
else:
print "Invalid padding algorithm"
quit()| assert hex_sig == case['S'] | ||
| if n.bit_length() < 2048: | ||
| debug("Skipping due to modulus length (too small).", DEBUG) | ||
| continue |
There was a problem hiding this comment.
This check should be n.bit_length() // 8 < 2048 // 8. In particular, ring considers any modulus where the length in bytes * 8 == 2048 to be large enough.
| debug("Skipping due to modulus length (too small).", DEBUG) | ||
| continue | ||
|
|
||
| if n.bit_length() > 4096: |
There was a problem hiding this comment.
This should only be done for signing tests, not verification tests, right?
|
FYI, I'm updating the test vector conversion script now. I found some additional things that should be changed when I tested it. I'll commit the updated version and then finish the review of the remaining stuff. |
| @@ -0,0 +1,60 @@ | |||
| # Test vectors for RSA-PSS padding for variable public modulus lengths | |||
There was a problem hiding this comment.
I'll move these test vectors into the same file as the NIST test vectors. In the PKCS#1 test vectors and other places we already mix imported test vectors with others that we've created, so there's always one file per algorithm (basically). It makes the regeneration of the converted-from-NIST files less convenient but it's not worth having separate files.
Plus, this file doesn't have the right naming convention.
There was a problem hiding this comment.
OK, I misunderstood the tests. They don't contain a public key so I can't merge them into rsa_pss_verify_tests.txt.
Instead, let's rename the file rsa_pss_verify_padding_tests.txt to match the other files that all start with rsa_.
There was a problem hiding this comment.
In addition to these test vectors, we will still need to have end-to-end tests of RSA-PSS with public moduli that aren't a multiple of 8, both positive and negative.
| let signature = signature.as_slice_less_safe(); | ||
| let mut decoded = [0u8; (MAX_BITS + 7) / 8]; | ||
| let mut decoded = [0u8; (PUBLIC_MODULUS_MAX_LEN + 7) / 8]; |
There was a problem hiding this comment.
I'll rename this variable to PUBLIC_MODULUS_MAX_BITS for clarity. I expect *_LEN to be in terms of array elements, which can't be bits.
| @@ -0,0 +1,338 @@ | |||
| # NIST RSA PSS Test Vectors for FIPS 186-4 from SigVerPSS_186-3.rsp in | |||
There was a problem hiding this comment.
I'll regenerate this file using the script that was updated with my changes.
| @@ -0,0 +1,60 @@ | |||
| # Test vectors for RSA-PSS padding for variable public modulus lengths | |||
| # | |||
There was a problem hiding this comment.
All the test vectors here are passing test vectors. We'll need to add some failing test vectors for these cases too.
|
I moved the script that converts the NIST test vectors to src/rsa/convert_nist_rsa_test_vectors.py because (1) the old name implied that it was for the signing tests only, but actually it is for both signing and verification, and (2) to keep all the RSA-related stuff together. I landed your updated NIST test vector conversion script in bbbb2fe. I removed the unused copies of the original NIST files. My tweaks, which address the issue I noted above and more, are in 563c2c3. I also added the SHA-384 digest of the NIST test vector files to the output. I redid the existing PKCS#1 1.5 test vectors based on the new script: 1045c8f. Let me know if anything looks off. Otherwise, I think we're done with the Python coding in this PR. |
|
I forgot to fold the SHA-384 digest part of the test vector conversion script into my aforementioned commit. It's now in 03ce40d. |
| Digest = SHA256 | ||
| Msg = e002377affb04f0fe4598de9d92d31d6c786040d5776976556a2cfc55e54a1dcb3cb1b126bd6a4bed2a184990ccea773fcc79d246553e6c64f686d21ad4152673cafec22aeb40f6a084e8a5b4991f4c64cf8a927effd0fd775e71e8329e41fdd4457b3911173187b4f09a817d79ea2397fc12dfe3d9c9a0290c8ead31b6690a6 | ||
| Encoded = c9cbb20ff49558e10505fea39e85e30f64f7b71a6307e51d58d1b09139e623ca00800224f6024fb84c7f5407f3d4045e4474def670e32d62590861e10612567eb8f51ba7d8b095553c214d75bb9960f33aa1aa173d020558e35d2b6d7385bd04223b707083d3311b37fea10f73fcb88ec5a9f1920fdef4dba6965756cf78a89ba4cea0fc29b834849c05fa6f8ba823e9c47df8b563f56aaa9a20f69dbee63bb56306c467963703e6671e1eb2f33021404f8ee9332b364f33b6468272afe85dd0ff8f94e7eb2a28876a975d51ec87af3e5fba4a50374daa2a0cde9eb8b675d9fab3a8ec5deee1cace4acb6e0df2ae12f6c63cf28c96492f1a742dcf91186cc0bc | ||
| Len = 2049 |
There was a problem hiding this comment.
If I understand correctly, Encoded is 256 bytes but the length in bits of the public modulus n is 2049 bits. However, the encoded message always needs to be large enough to hold a value up to n - 1. That is, it must always be the case that 8*Encoded.len() >= Len. Therefore, this test case seems to be invalid.
In other words, this test case is verifying the buggy behavior mentioned above, rather than the correct behavior.
To fix it so that it is testing the correct behavior, a leading zero byte needs to be added to it. I have a patch that does this.
There was a problem hiding this comment.
It has been a while now since I wrote that code, but I think it's correct in this form (per the spec).
From https://tools.ietf.org/html/rfc3447#section-8.1.1:
Apply the EMSA-PSS encoding operation (Section
9.1.1) to the message M to produce an encoded message EM of length
\ceil ((modBits - 1)/8)octets such that the bit length of the
integer OS2IP (EM) (see Section 4.2) is at mostmodBits - 1, where
modBits is the length in bits of the RSA modulus n:
EM = EMSA-PSS-ENCODE (M, modBits - 1).
Note that the octet length of EM will be one less than k if
modBits - 1 is divisible by 8 and equal to k otherwise.
(Here k is the length in bytes of the public modulus).
Using your example values, modBits is 2049, k is 257, so the encoded messages should be \ceil((2049 - 1)/8) = 256 bytes. This is is the exceptional case noted in the last sentence.
There was a problem hiding this comment.
I'm open to the idea that I'm wrong, but here's my thinking:
PKCS#1 and PSS both get the same inputs, namely for a 2049-bit modulus they are both going to get a value of encoded that is 257 bytes long. Note in particular that the caller of Verification::verify() always converts the BIGNUM value M into a big-endian value of length k. Now, for PSS the value of M only requires emLen bytes, which, as you say, might be one less than k. But, the caller of Verification::verify() is going to left-pad encoded with an extra zero byte, and so in order to get the value EM the implementation of Verification::verify() must strip off one zero byte if and only if (modBits - 1) % 8 == 0.
WDYT?
There was a problem hiding this comment.
BTW, we'll know which interpretation is correct by adding a completely test vector for a key with a 2049-bit modulus in rsa_pss_verify_tests.txt.
Regarding the tests in pss_verify_tests.txt, I see that there is a longish value of Msg that is used in all the tests: e002377affb04f0fe4598de9d92d31d6c786040d5776976556a2cfc55e54a1dcb3cb1b126bd6a4bed2a184990ccea773fcc79d246553e6c64f686d21ad4152673cafec22aeb40f6a084e8a5b4991f4c64cf8a927effd0fd775e71e8329e41fdd4457b3911173187b4f09a817d79ea2397fc12dfe3d9c9a0290c8ead31b6690a6. What does this encode? Traditionally we've used the value "" (the empty string) in other test vectors we've created, when the value of the message doesn't matter. This way, we don't have to worry about any hidden (unwanted) messages in the test vectors or malicious test vectors (specially constructed so that the tests pass when they would fail for most other values), amongst other things.
There was a problem hiding this comment.
PKCS#1 and PSS both get the same inputs, namely for a 2049-bit modulus they are both going to get a value of encoded that is 257 bytes long.
Ah because the upper layer producing the encoded message is always going to return 257 bytes. That makes sense. This is covered step 2c in the spec section 8.1.2, which technically should be handled before calling verify, but definitely looks more sensible to include it within the body of verify.
I think we should probably put a small comment to that effect somewhere.
There was a problem hiding this comment.
The test case was simply one of those used in the NIST tests. Empty string seems like a sensible alternative.
| // Number of bytes required to encode message. The maximum length is | ||
| // given by the length of the public modulus in bits minus 1. | ||
| let em_len = 1 + (public_modulus_len_in_bits - 2) / 8; | ||
| assert_eq!(encoded.len(), em_len); |
There was a problem hiding this comment.
I don't think this is correct. Let's say we have a 2049 bit modulus. Then encoded.len() == (2049 + 7) / 8, which is 257. However, emBits == modLen - 1 == 2048, and emLen == ceil(emBits / 8), which is 256. Thus we have encoded.len() != em_len here.
I have a patch that fixes this.
There was a problem hiding this comment.
See my other comment for why I think this is correct. It should be the number of bytes needed to represent a public_modulus_len_in_bits - 1-bit value. Could probably be simplified to public_modulus_len_in_bits + 6) / 8 however.
There was a problem hiding this comment.
Also note that we will need to do something similar for the encode function. If (modbits - 1) % 8 ==0 then the padding will need to set the first byte to be 0 and output the padding starting from byte 1.
|
I spent quite a bit of time reviewing this PR. In the course of doing so, I've written some patches that (1) factor out the duplicate logic between verification and encoding, and (2) align the code more closely to the specification. I still need to create more test vectors, including in particular negative test vectors. |
|
Thanks for the review. The python changes all look good. It's a shame there aren't more test vectors out there we can use to clarify the length issue. Let me know if there's anything more I can do otherwise. Small nit in python file: filename in the usage help string is incorrect (here). |
|
For people following along, I posted some review comments in the form of commits in #344, with the expectation that those commits will get incorporated back into this PR. |
|
Thanks very much for doing this. I landed all these changes plus the additional changes in #344 onto master. There is some follow-up work to do before the 0.6.0 release regarding testing and clarifying the code further, but I'd rather do that in separate PRs that are easier to manage. Let's continue discussion in #115 (PSS verification) and #213 (PSS signing). 4e51451 <- From this PR. |
Steps taken so far:
This is a bit rough at the moment. There are some fairly large changes made by this so I thought it would be best to put it out there for feedback before proceeding.
Relevant issues: #115 and #213