Modifications to PR #262 made during review. #344

briansmith · 2016-11-14T05:11:32Z

@samscott89 Could you please take a look at these changes I am proposing for your PR?

briansmith · 2016-11-14T05:13:18Z

src/rsa/padding.rs

-               try!(em.read_byte()) != 1 {
-                return Err(error::Unspecified);
-            }
+        let em = m;



:sigh: All of these changes are just supposed to be un-indenting and re-wrapping the code.

briansmith · 2016-11-14T05:13:52Z

src/rsa/padding.rs

-            let em_bits = mod_bits - 1;
-            let em_len = (em_bits + 7) / 8;
-            let top_byte_mask = 0xffu8 >> ((8 * em_len) - em_bits);
+        // RSASSA-PSS-VERIFY Step 2(c). The `m` this function is given is the


Ditto. I'll verify that I didn't actually change the code in any significant way other than reformatting it.

coveralls · 2016-11-14T09:15:13Z

Changes Unknown when pulling 1fa60373cab9636811ee4fffffc52228712ef211 on padding into * on master*.

samscott89

Changes look great. Left a few minor formatting comments. Other than that I think the only thing to be addressed is the lack of PSS padding tests.

samscott89 · 2016-11-14T12:08:49Z

src/rsa/padding.rs

-                return Err(error::Unspecified);
-            }
+        if try!(em.read_byte()) != 0 ||
+            try!(em.read_byte()) != 1 {


This condition fits fine on one line.

I've now split it into two conditions so that the code coverage can see whether we're testing both checks.

samscott89 · 2016-11-14T12:10:43Z

src/rsa/padding.rs

+        // the out buffer, and then XOR the value of db.
+
+        // Step 9. First output the mask into the out buffer.
+       let (mut masked_db, mut digest_terminator) =


Space missing from start of the line.

(Next line would need an additional space as well)

samscott89 · 2016-11-14T12:16:10Z

src/rsa/padding.rs

+
+        let digest_alg = self.digest_alg;
+        let decoded_digest =
+            try!(em.skip_and_get_input(digest_alg.output_len));


This also fits in one 80-char line now.

samscott89 · 2016-11-14T12:18:56Z

src/rsa/padding.rs

+        let em_bits = mod_bits - 1;
+        let em_len = (em_bits + 7) / 8;
+        let leading_zero_bits = (8 * em_len) - em_bits;
+        assert!(leading_zero_bits < 8);


Out of curiosity: why assert over debug_assert or returning an error?

I changed it to a debug_assert!.

samscott89 · 2016-11-14T12:35:21Z

src/rsa/padding.rs

+            // Step 8.
+            for i in 1..db.len() {
+                db[i] ^= try!(masked_bytes.read_byte());
+            }


Since you've added iter in untrusted 0.3.2, is it worth updating this code to use that instead, similarly to how you've written the equivalent code in encode?

Yes, that's exactly what I want to happen.

samscott89 · 2016-11-14T12:52:02Z

src/rsa/signing.rs

            return Err(error::Unspecified);
        }

-        try!(padding_alg.encode(msg, signature));
+        try!(padding_alg.encode(msg, signature, mod_len*8, rng));


Spaces around the operator: mod_len * 8?

samscott89 · 2016-11-14T12:53:02Z

src/rsa/rsa.rs



+// Maximum RSA modulus size supported for signature verification (in bits).
+const PUBLIC_MODULUS_MAX_LEN: usize = 8192;


As you commented in the other thread, PUBLIC_MODULUS_MAX_BITS is a better name for this.

samscott89 · 2016-11-14T13:00:42Z

src/rsa/pss_verify_tests.txt

@@ -0,0 +1,60 @@
+# Test vectors for RSA-PSS padding for variable public modulus lengths


Outstanding tasks for this file:

Rename to rsa_pss_verify_padding_tests.txt.

Add failing cases.

Cover more digest types.

Would it be simpler to add these cases to the existing rsa_pss_verify_tests.txt file instead? The only reason I could see that these should have independent tests is if the padding functions were made public.

I can make a start on adding failing cases.

samscott89 · 2016-11-14T13:02:40Z

src/rsa/verification.rs

-    params.padding_alg.verify(msg, untrusted::Input::from(decoded))
+    untrusted::Input::from(decoded).read_all(
+        error::Unspecified,
+        |m| params.padding_alg.verify(msg, m, n.length_in_bits()))


Should the closing bracket be on the line below, at the same indentation level as line 127?

samscott89 · 2016-11-14T13:03:33Z

third-party/NIST/README.md

@@ -1,5 +1,8 @@
 # NIST Test Vectors

+[CAVP](CAVP) contains a number of test cases from the [NIST Individual Component


These lines can be deleted since we not longer store the NIST tests here.

briansmith · 2016-11-14T23:43:16Z

@samscott89 Please check out the new commits I appended to this series to address your concerns. Let me know if you want me to squash them into the prior commits before or after you review them.

Pay special attention to the change I made to the PSS encoding, passing in the actual bit length of the public modulus instead of 8 * mod_len. I think this was another bug, triggered by me saying that we only needed to support moduli that are even multiples of 8 for signing.

Also, do you want to take over the remaining tasks? I definitely don't want to steal this from you.

coveralls · 2016-11-15T04:06:02Z

Changes Unknown when pulling 5f139ea on padding into * on master*.

coveralls · 2016-11-15T04:06:02Z

Changes Unknown when pulling 5f139ea on padding into * on master*.

samscott89 · 2016-11-15T07:29:19Z

src/bits.rs

+use error;
+
+#[derive(Clone, Copy, Eq, PartialEq)]
+pub struct BitLength {


Should this be called just Length since it will presumably always be used as bits::Length vs bits::BitLength?

Also, is this not practically the canonical example for using newtypes?

I agree it's weird to have bits::BitLength but I also think bits::Length or bit::Length is too clever. I found out that these names that only make sense with module prefixes don't work well in rustdoc (see error::Unspecified which rustdoc presents as just Unspecified, which is confusing).

I think we probably should create a "prelude" of common types that are used internally in ring and use those types (only) unprefixed. I think BitLength is a prime candidate for that.

Regarding newtypes, see #350. The problem is that we have to constructors: from bits, and from bytes. A newtype would make the from-bits constructor implicit, but I'd rather have the constructors be used explicitly. However, we may want to switch to newtype style if it helps work around the lack of const fn.

samscott89 · 2016-11-15T08:03:45Z

Yup, all looks good to me. The BitLength additional particularly seems like a good idea project-wide. Would it make sense to (in a separate PR) try and type all lengths? So that there is ByteLength(usize) and BitLength(usize) or something? Or is it okay to assume that all non-BitLength lengths are in bytes?

Also, do you want to take over the remaining tasks? I definitely don't want to steal this from you.

I'm more than happy to, and allow you to move onto something else. I think you've given me enough help at this point for me to get it across the finish line. I'm not worried about it being 'stolen' from me though, I'm just looking to learn/help as much as I can.

If you could squash it down to a reasonable number of commits (perhaps original commit + review or something?), that would be great. I'll then pull the padding branch and continue in the original PR.

coveralls · 2016-11-15T08:14:08Z

Changes Unknown when pulling 5f139ea on padding into * on master*.

coveralls · 2016-11-15T08:14:08Z

Changes Unknown when pulling 5f139ea on padding into * on master*.

briansmith · 2016-11-15T19:52:48Z

Yup, all looks good to me. The BitLength additional particularly seems like a good idea project-wide.

I filed #350 for this.

Would it make sense to (in a separate PR) try and type all lengths? So that there is ByteLength(usize) and BitLength(usize) or something? Or is it okay to assume that all non-BitLength lengths are in bytes?

I think we should just assume all non-BitLength lengths are in bytes. Otherwise we'd have to move ByteLength (at least) to untrusted.

Also, do you want to take over the remaining tasks? I definitely don't want to steal this from you.
I'm more than happy to, and allow you to move onto something else. I think you've given me enough help at this point for me to get it across the finish line. I'm not worried about it being 'stolen' from me though, I'm just looking to learn/help as much as I can.

If you could squash it down to a reasonable number of commits (perhaps original commit + review or something?), that would be great. I'll then pull the padding branch and continue in the original PR.

Will do.

briansmith · 2016-11-15T20:48:49Z

I squashed and landed all these changes onto master. See discussion in #262.

briansmith commented Nov 14, 2016

View reviewed changes

samscott89 reviewed Nov 14, 2016

View reviewed changes

briansmith force-pushed the padding branch from 1fa6037 to e59bb11 Compare November 14, 2016 23:04

briansmith mentioned this pull request Nov 15, 2016

Implement RSA PSS verification and signing. #262

Closed

samscott89 reviewed Nov 15, 2016

View reviewed changes

briansmith closed this Nov 15, 2016

briansmith force-pushed the padding branch from 5f139ea to 5fb1fc8 Compare November 15, 2016 20:47

briansmith deleted the padding branch November 15, 2016 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifications to PR #262 made during review. #344

Modifications to PR #262 made during review. #344

briansmith commented Nov 14, 2016

briansmith Nov 14, 2016

briansmith Nov 14, 2016

coveralls commented Nov 14, 2016

samscott89 left a comment

samscott89 Nov 14, 2016

briansmith Nov 14, 2016

samscott89 Nov 14, 2016

samscott89 Nov 14, 2016

briansmith Nov 14, 2016

samscott89 Nov 14, 2016

briansmith Nov 14, 2016

samscott89 Nov 14, 2016

briansmith Nov 14, 2016

samscott89 Nov 14, 2016

briansmith Nov 14, 2016

samscott89 Nov 14, 2016

briansmith Nov 14, 2016

samscott89 Nov 14, 2016

samscott89 Nov 14, 2016

samscott89 Nov 14, 2016

samscott89 Nov 14, 2016

samscott89 Nov 14, 2016

briansmith Nov 14, 2016

briansmith commented Nov 14, 2016

coveralls commented Nov 15, 2016

coveralls commented Nov 15, 2016

samscott89 Nov 15, 2016

briansmith Nov 15, 2016

briansmith Nov 15, 2016

samscott89 commented Nov 15, 2016

coveralls commented Nov 15, 2016

coveralls commented Nov 15, 2016

briansmith commented Nov 15, 2016

briansmith commented Nov 15, 2016



		// Maximum RSA modulus size supported for signature verification (in bits).
		const PUBLIC_MODULUS_MAX_LEN: usize = 8192;

		@@ -0,0 +1,60 @@
		# Test vectors for RSA-PSS padding for variable public modulus lengths

		@@ -1,5 +1,8 @@
		# NIST Test Vectors

		[CAVP](CAVP) contains a number of test cases from the [NIST Individual Component

Modifications to PR #262 made during review. #344

Modifications to PR #262 made during review. #344

Conversation

briansmith commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 14, 2016

samscott89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith commented Nov 14, 2016

coveralls commented Nov 15, 2016

coveralls commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samscott89 commented Nov 15, 2016

coveralls commented Nov 15, 2016

coveralls commented Nov 15, 2016

briansmith commented Nov 15, 2016

briansmith commented Nov 15, 2016