Add `signature::primitive::verify_rsa` #226

djc · 2016-06-22T20:32:25Z

I agree to license my contributions to each file under the terms given
at the top of each file I changed.

briansmith · 2016-06-28T18:22:48Z

From the old IRC conversation, my understanding is that there are two reasons we need such a function:

Application need to verify an RSA signatures for public keys that were given to them in non-ASN.1-encoded form.
When we generate an RSA key pair, we need a way to extract the public key from the key pair.

For #1, I suggest that instead we just add a new function ring::signature::primitive::verify_rsa_signature(padding: &RSAPadding, msg: untrusted::Input, sig: untrusted::Input, /*public_key*/(n, e): (untrusted::Input, untrusted::Input)) -> Result<(), ()> for the verification. Then the application won't have to construct the DER-encoded key (which you don't like anyway, IIRC) and we can avoid doing memory allocation during signature verification.

For #2, I suggest that when we create a public key, we should create and store the ASN.1-encoded public key in a Vec owned by the RSAKeyPair. Then RSAKeyPair can have a public_key_bytes function like Ed25519KeyPair. This would use the code that you submitted in this PR, but it would be internal to RSAKeyPair.

WDYT?

djc · 2016-06-28T19:42:49Z

Sounds good. Please review my implementation of #1.

briansmith · 2016-06-29T00:07:22Z

It looks like you forgot to git add the test file?

djc · 2016-06-29T05:47:20Z

Yeah... I've fixed it up now.

coveralls · 2016-06-29T06:53:05Z

Coverage increased (+0.5%) to 87.16% when pulling 3e473f2e4bd97e1e34f2d963c399a1cae52bb0e7 on djc:public-key-as-der into 97bb46a on briansmith:master.

briansmith · 2016-06-30T08:48:13Z

src/rsa.rs

+                  (n, e): (untrusted::Input, untrusted::Input))
+                  -> Result<(), ()> {
+        let n = n.as_slice_less_safe();
+        let e = e.as_slice_less_safe();


I think instead we should change parse_public_key so that it returns untrusted::Inputs and then have super::verify take (n, e): (untrusted::Input, untrusted::Input). This removes the duplication of the as_slice_less_safe and fits the pattern I'm trying to promote wherein we avoid using slices for untrusted inputs when possible.

briansmith · 2016-06-30T09:05:26Z

We need to decide how/where we're going to expose the low-level primitives. My original suggestion was that we have ring::signature::primitive::verify_rsa or similar. But, that's not what was implemented here. I'm open to suggestions of doing things a different way. But, if I'm understanding the PR correctly, it is adding a verify function exposed from the ring module directly? ring::verify? That doesn't make sense to me. I'd prefer to have the primitive things tucked away in their "use these if you don't have any other choice" corner.

Also, I think we need to update the module-level documentation of ring::signature to tell people that it is recommended to use the original higher-level interface, but if the ASN.1-encoded public keys don't work for them, they can use this low-level interface instead. Otherwise, I think we'll confuse people and/or they won't find this new API.

djc · 2016-06-30T09:31:01Z

Ah, sorry if I implemented the API wrong. I'm not yet well-versed in the intricacies of Rust's module system and how it interacts with public APIs. Will try to rejigger so that it exposes ring::signature::primitive::verify_rsa(), with an implementation in ring::rsa, including some documentation.

briansmith · 2016-06-30T10:05:29Z

Suggestion: do cargo doc -p ring --open to preview what your API changes end up looking like. (--open doesn't always work for me.)

djc · 2016-06-30T19:54:16Z

This should be better, let me know what you think!

coveralls · 2016-06-30T21:55:10Z

Coverage increased (+0.09%) to 87.474% when pulling c50f1683897da8287ebb780ac98012277d5b8d6a on djc:public-key-as-der into 752c8c6 on briansmith:master.

coveralls · 2016-07-01T11:25:19Z

Coverage increased (+0.03%) to 87.863% when pulling 9f7dd3f6ef0f15f9377938abad96d06350700389 on djc:public-key-as-der into e4e175d on briansmith:master.

briansmith · 2016-07-05T06:18:15Z

src/rsa.rs

+                  signature: untrusted::Input,
+                  (n, e): (untrusted::Input, untrusted::Input))
+                  -> Result<(), ()> {
+        super::verify(padding, msg, signature, (n, e), 2048)


Please see my comments in the other PR where you renamed RSA_PKCS1_2048_8192_SHA*_VERIFY to RSA_PKCS1_SHA*_2048_8192. I had landed that change, and then I saw this change and decided to revert that change because this helped me see that we're going in the wrong direction.

As discussed previously, our goal is to eventually develop an API that treats ECDSA, RSA, etc. uniformly or at least as similarly as possible. In ECDSA, the only parameters you have are curve and digest algorithm. In RSA, you have min bits, max bits, digest algorithm, exponent, and padding scheme. In ring, I've chosen to use the following mapping:

ECDSA RSA EdDSA

Algorithm = ECDSA Algorithm = (RSA, Padding Scheme) Algorithm = EdDSA

Curve modulus min bits, modulus max bits, exponent min bits, exponent max bits) Curve

Digest Alg. Digest Alg. implicit

Encoding (ASN.1, PKCS#11/WebCrypto) implicit implicit

Note that the exponent range is fixed at (2 bits, 33 bits). But, I'm thinking of changing that so that for the 3072-8192 variant (Suite B), at least, the minimum exponent length will be 16 bits.

So, anyway, I'm a little unsure how important it is to be explicit about the allowed exponent range, but at least this new primitive API should allow the caller to be explicit about the modulus range--at least the minimum modulus length.

Going back to your other PR, the issue you had was that RSAPaddings are named RSA_<padding_scheme>_<digest_alg> but RSA SignatureAlgorithms are named similarly, but they put <min_modulus_bits>_<max_modulus_bits> in between. And, that affects this API because it takes RSAPadding as a parameter. But, maybe this API shouldn't take RSAPadding.

Ideally, this API should take RSA_PKCS1_2048_8192_SHA256, etc. as parameters instead, like verify() does, but with a type that isn't VerificationAlgorithm but instead something like RSAParameters or something. After all, this API is exactly like verify except that it takes its public key argument pre-parsed.

But, I'm not sure how to do that without making VerificationAlgorithm a trait, which I don't want to do for the reasons I explained before. If I have to choose between the main verify API forcing people to use traits or the primitive API forcing people to use traits, I'd prefer to put that inconvenience on the primitive API.

Maybe it is possible to change the type of RSA_PKCS1_2048_8192_SHA256 et al. from &'static VerificationAlgorithm to &'static <VerificationAlgorithm + RSAParameters> and make RSAParameters a trait? I've never tried to do that before so i'm not sure.

Or, if that's all too complicated, then we can punt on trying to create a really uniform API and just, at least, make the minimum key size a parameter of this new function. The main thing I want to avoid doing is making the key size implicit like it is in this patch, because one of the most important design goals for ring is to avoid such implicit security-critical parameters.

So we just have RSAParameters that carries a reference to an RSAPadding, and then a SignatureVerificationAlgorithm that carries a reference to an RSAParameters? For now, RSAParameters can also carry min_bits, and then you could add more stuff later.

So we just have RSAParameters that carries a reference to an RSAPadding, and then a SignatureVerificationAlgorithm that carries a reference to an RSAParameters? For now, RSAParameters can also carry min_bits, and then you could add more stuff later.

That seems like one reasonable way of doing it. TBH, it's a little hard for me to tell w/o seeing some code for it.

briansmith · 2016-08-03T09:56:35Z

@samscott89 Your work on PSS might benefit from looking at this.

djc · 2016-08-03T11:06:07Z

Pushed the latest version for reference.

coveralls · 2016-08-03T12:39:55Z

Coverage increased (+0.02%) to 89.463% when pulling 7b614c64a3b6a11f1513fb5e88cefb5545a2b8da on djc:public-key-as-der into 0ab853a on briansmith:master.

briansmith · 2016-08-11T21:49:29Z

Rebased: https://github.com/briansmith/ring/commits/public-key-as-der

briansmith · 2016-08-11T21:59:37Z

src/rsa.rs

+impl signature::VerificationAlgorithm for RSAParameters {
+    fn verify(&self, public_key: untrusted::Input, msg: untrusted::Input,
+              signature: untrusted::Input) -> Result<(), ()> {
+        verify(self, msg, signature, public_key)


The arguments to the new verify function should be in the same order as the arguments to the existing verify functions: "public_key, msg, signature".

briansmith · 2016-08-27T21:39:30Z

src/lib.rs

@@ -90,6 +89,8 @@
    while_true,
 )]

+#![cfg_attr(not(feature = "nightly"), deny(drop_with_repr_extern))]


I didn't see this until I'd already written a different patch. This is smarter than what I did. I don't think we need to do this though, because in 6 weeks we'd have to remove it. Thanks though!

briansmith · 2016-08-27T21:50:31Z

I updated https://github.com/briansmith/ring/commits/public-key-as-der with your patches rebased on top of the previous version and on top of the current master.

I agree to license my contributions to each file under the terms given at the top of each file I changed.

djc · 2016-09-04T13:08:37Z

The latest should address all your comments, and has your comment extension folded in. I hope we're converging!

coveralls · 2016-09-04T16:22:56Z

Coverage decreased (-2.8%) to 88.783% when pulling 96437b9d24faa306efed62fb7c03174a70af35de on djc:public-key-as-der into fcbf905 on briansmith:master.

djc · 2016-09-04T18:28:46Z

So my change to BN_bin2bn() in d77a179 appears to have broken RSA signing, where I guess there can be leading zeroes coming out of the parsing of DER-encoded ASN.1. I tried to remedy it with something like this, but it doesn't seem to work. Any suggestions?

diff --git a/src/rsa/rsa.rs b/src/rsa/rsa.rs
index f540c3b..7d2870e 100644
--- a/src/rsa/rsa.rs
+++ b/src/rsa/rsa.rs
@@ -126,8 +126,14 @@ impl PositiveInteger {
     fn from_der(input: &mut untrusted::Reader)
                 -> Result<PositiveInteger, error::Unspecified> {
         let bytes = try!(der::positive_integer(input)).as_slice_less_safe();
+        let raw_bytes = if bytes[0] == 0 {
+                &bytes[1..]
+            } else {
+                &bytes[..]
+            };
         let res = unsafe {
-            GFp_BN_bin2bn(bytes.as_ptr(), bytes.len(), core::ptr::null_mut())
+            GFp_BN_bin2bn(raw_bytes.as_ptr(), raw_bytes.len(),
+                          core::ptr::null_mut())
         };
         if res.is_null() {
             return Err(error::Unspecified);

briansmith · 2016-09-04T18:50:48Z

So my change to BN_bin2bn() in d77a179 appears to have broken RSA signing, where I guess there can be leading zeroes coming out of the parsing of DER-encoded ASN.1. I tried to remedy it with something like this, but it doesn't seem to work. Any suggestions?

der::positive_integer already strips leading zeros; if it doesn't, that's a bug that I'll fix ASAP. Your patch to PositiveInteger::from_der doesn't account for the case of [0], i..e. in the case of the actual value zero, the result shouldn't be an empty slice.

But, also, the point of this PR is to completely bypass the need for DER decoding in the first place, right? I think instead we need to add the leading zero detection within verify_rsa. Maybe we should have a (Rust?) function that acts like a wrapper around BN_bin2bn that checks that there isn't a leading zero before calling BN_bin2bn? Something like the following (untested; I'm traveling):

impl PositiveInteger {
    fn from_be_bytes(input: untrusted::Input) -> Result<PositiveInteger, error::Unspecified> {
        // Check that there's no leading zero but allow a single zero byte.
        if input.len() > 1 {
            try!(input.read_all(error::Unspecified, |input| {
                if try!(input.read_byte()) == 0 {
                    return Err(error::Unspecified);
                }
                input.skip_to_end();
                Ok(())
            });
        }
        let res = GFp_BN_bin2bn(input.as_slice_less_safe().as_ptr(), input.len(),
                                core::ptr::null_mut());
       [...]
    }
}

Note that PositiveInteger::from_der could probably be then be implemented as a call to der::positive_integer() followed by a call to such a from_be_bytes() function.

djc · 2016-09-04T19:22:45Z

Well, the problem doesn't show up for verification, only for signing, so I'm not sure it makes sense to add something to verify_rsa()? I can try out your BN_bin2bn() wrapper and see if it resolves the issue.

Moves the conversion from untrusted::Input (via slice) to BIGNUM from C to Rust using the PositiveInteger struct we already use for signing. Adds a check to error out when encountering a number that starts with zero. I agree to license my contributions to each file under the terms given at the top of each file I changed.

I agree to license my contributions to each file under the terms given at the top of each file I changed.

…ication. I agree to license my contributions to each file under the terms given at the top of each file I changed.

djc · 2016-09-05T19:48:27Z

I followed through on your suggestion, in 18fdc13. The result seems sane, and passes the tests for me even with rsa_signing enabled. Note that this required moving some code in rsa::rsa out from behind the rsa_signing guard.

coveralls · 2016-09-06T01:56:27Z

Coverage increased (+0.03%) to 91.62% when pulling 5da946f on djc:public-key-as-der into fcbf905 on briansmith:master.

coveralls · 2016-09-06T01:56:27Z

Coverage increased (+0.03%) to 91.62% when pulling 5da946f on djc:public-key-as-der into fcbf905 on briansmith:master.

coveralls · 2016-09-06T01:56:27Z

Coverage increased (+0.03%) to 91.62% when pulling 5da946f on djc:public-key-as-der into fcbf905 on briansmith:master.

coveralls · 2016-09-06T01:56:27Z

Coverage increased (+0.2%) to 91.803% when pulling 5da946f on djc:public-key-as-der into fcbf905 on briansmith:master.

briansmith · 2016-09-11T02:36:54Z

This looks good. I made some adjustments for the new PositiveInteger constructor. Please see the last commit at https://github.com/briansmith/ring/tree/public-key-as-der. WDYT?

djc · 2016-09-11T05:47:41Z

It looks good to me.

So you want tests for both the low-level and higher-level APIs, even if the lower-level API is covered by tests for the higher-level one?

What's the merit of Reader::peek() over just indexing into the slice here?

Also, note that "indention" is an archaic word for "indentation".

When this is merged, would you mind bumping ring on crates.io?

briansmith · 2016-09-11T06:55:15Z

So you want tests for both the low-level and higher-level APIs, even if the lower-level API is covered by tests for the higher-level one?

I try to add tests where they are helpful, without getting carried away. In this case, I think there was a bug where [] was accepted as zero even though it shouldn't have been. It was easier to test that this way.

What's the merit of Reader::peek() over just indexing into the slice here?

Reader::peek() cannot panic. In general, we avoid indexing into slices whenever possible in ring, by using untrusted instead.

Also, note that "indention" is an archaic word for "indentation".

:) Too late, I guess. Thanks though.

When this is merged, would you mind bumping ring on crates.io?

Sure!

briansmith · 2016-09-11T08:37:04Z

https://crates.io/crates/ring/0.4.1
https://briansmith.org/rustdoc/ring/signature/primitive/index.html

5778edb 0.4.1 release.
771dbda Clarify encoding of n and e in verify_rsa.
c1ad0ba PositiveInteger::from_be_bytes: Add minimal tests; reject empty inputs.
4637a96 Add tests for zero-padding public key components with primitive verification.
15cd1d0 Test misuse-resistance for signature::primitive::verify_rsa().
0f5cfd5 Use rsa::PositiveInteger for public key parts in verification.
931800b Expand historical context of testing of verify_rsa.
f3452ef Expose rsa::verify as ring::signature::verify_rsa.
09484b1 Fix indention in src/rsa/rsa.rs.
80e96e0 Change rsa::parse_public_key to return components as Input.
9c85dfd Extract rsa::verify_rsa function from RSAParameters impl.

Thanks a lot for contributing this!

djc force-pushed the public-key-as-der branch from 69e4d23 to ca517e1 Compare June 28, 2016 19:42

djc force-pushed the public-key-as-der branch from ca517e1 to 3e473f2 Compare June 29, 2016 05:46

briansmith reviewed Jun 30, 2016
View reviewed changes

djc force-pushed the public-key-as-der branch from 3e473f2 to c50f168 Compare June 30, 2016 19:53

djc force-pushed the public-key-as-der branch from c50f168 to 9f7dd3f Compare July 1, 2016 09:10

briansmith changed the title ~~Add rsa_public_key_as_der() function to signature API.~~ Add rsa::primitive::verify Jul 3, 2016

briansmith reviewed Jul 5, 2016
View reviewed changes

briansmith force-pushed the master branch from cf3284e to 5fd8301 Compare July 7, 2016 02:57

djc mentioned this pull request Jul 19, 2016

Use enum to dispatch to signature verification algorithm handlers. #255

Closed

djc force-pushed the public-key-as-der branch from 9f7dd3f to 7b614c6 Compare August 3, 2016 11:05

briansmith reviewed Aug 11, 2016
View reviewed changes

djc force-pushed the public-key-as-der branch from 2c12b61 to 8ecb0d1 Compare August 27, 2016 14:25

briansmith reviewed Aug 27, 2016
View reviewed changes

briansmith mentioned this pull request Aug 28, 2016

forked. Have features, want to contribute. Keats/jsonwebtoken#13

Closed

djc added 2 commits September 4, 2016 13:00

Extract utility rsa::verify() function from RSAParameters impl.

bad3fba

I agree to license my contributions to each file under the terms given at the top of each file I changed.

Change rsa::parse_public_key() to return components as Input.

957a9d9

I agree to license my contributions to each file under the terms given at the top of each file I changed.

djc force-pushed the public-key-as-der branch from 8ecb0d1 to 96437b9 Compare September 4, 2016 13:07

djc added 4 commits September 5, 2016 21:44

Expose rsa::verify_rsa() as signature::primitive::verify_rsa().

da511d4

I agree to license my contributions to each file under the terms given at the top of each file I changed.

Test misuse-resistance for signature::primitive::verify_rsa().

0e0ea3f

I agree to license my contributions to each file under the terms given at the top of each file I changed.

Add tests for zero-padding public key components with primitive verif…

5da946f

…ication. I agree to license my contributions to each file under the terms given at the top of each file I changed.

djc force-pushed the public-key-as-der branch from 96437b9 to 5da946f Compare September 5, 2016 19:45

briansmith closed this Sep 11, 2016

ECDSA	RSA	EdDSA
Algorithm = ECDSA	Algorithm = (RSA, Padding Scheme)	Algorithm = EdDSA
Curve	modulus min bits, modulus max bits, exponent min bits, exponent max bits)	Curve
Digest Alg.	Digest Alg.	implicit
Encoding (ASN.1, PKCS#11/WebCrypto)	implicit	implicit

Add signature::primitive::verify_rsa #226

Add signature::primitive::verify_rsa #226

Conversation

djc commented Jun 22, 2016

briansmith commented Jun 28, 2016

djc commented Jun 28, 2016

briansmith commented Jun 29, 2016

djc commented Jun 29, 2016

coveralls commented Jun 29, 2016 • edited

briansmith Jun 30, 2016

Choose a reason for hiding this comment

briansmith commented Jun 30, 2016

djc commented Jun 30, 2016 • edited

briansmith commented Jun 30, 2016

djc commented Jun 30, 2016

coveralls commented Jun 30, 2016 • edited

coveralls commented Jul 1, 2016 • edited

briansmith Jul 5, 2016

Choose a reason for hiding this comment

djc Jul 5, 2016

Choose a reason for hiding this comment

briansmith Jul 8, 2016

Choose a reason for hiding this comment

briansmith commented Aug 3, 2016

djc commented Aug 3, 2016

coveralls commented Aug 3, 2016 • edited

briansmith commented Aug 11, 2016

briansmith Aug 11, 2016

Choose a reason for hiding this comment

briansmith Aug 27, 2016

Choose a reason for hiding this comment

briansmith commented Aug 27, 2016

djc commented Sep 4, 2016

coveralls commented Sep 4, 2016

djc commented Sep 4, 2016 • edited

briansmith commented Sep 4, 2016 • edited

djc commented Sep 4, 2016

djc commented Sep 5, 2016

coveralls commented Sep 6, 2016

coveralls commented Sep 6, 2016

coveralls commented Sep 6, 2016

coveralls commented Sep 6, 2016 • edited

briansmith commented Sep 11, 2016

djc commented Sep 11, 2016

briansmith commented Sep 11, 2016

briansmith commented Sep 11, 2016

Add `signature::primitive::verify_rsa` #226

Add `signature::primitive::verify_rsa` #226

coveralls commented Jun 29, 2016 •

edited

djc commented Jun 30, 2016 •

edited

coveralls commented Jun 30, 2016 •

edited

coveralls commented Jul 1, 2016 •

edited

coveralls commented Aug 3, 2016 •

edited

djc commented Sep 4, 2016 •

edited

briansmith commented Sep 4, 2016 •

edited

coveralls commented Sep 6, 2016 •

edited