Use a more specific error instead of unspecified::Error for DER encoding errors, as per #379 #384

marshallpierce · 2016-12-19T21:26:45Z

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

marshallpierce · 2016-12-19T21:27:53Z

src/ec/suite_b/ops/ops.rs

@@ -338,7 +338,7 @@ pub struct PublicScalarOps {
 impl PublicScalarOps {
    pub fn scalar_parse(&self, input: &mut untrusted::Reader)
                        -> Result<Scalar, error::Unspecified> {
-        let encoded_value = try!(der::positive_integer(input));
+        let encoded_value = try!(der::positive_integer(input).map_err(|_| error::Unspecified));


There were only a few places that needed to map_err so I chose to not implement From<ASN1Error>, but I can do that if you prefer.

It's OK to punt on that for now.

marshallpierce · 2016-12-19T21:28:56Z

src/der.rs

+/// An error in ASN1 parsing. Normal hygiene applies: if parsing secret
+/// DER, do not differentiate this from any other error.
+#[derive(Clone, Copy)]
+pub struct ASN1Error;


Asn1Error? DerError?

I think ASN1Error is fine even because I can't stand Asn1Error or similar capitalization of acronyms.

coveralls · 2016-12-21T19:40:11Z

Coverage increased (+0.001%) to 93.24% when pulling 29b459c2926e94effa245d32dbe90fcf8deaa6ea on marshallpierce:more-specific-error-for-der into 8596c78 on briansmith:master.

coveralls · 2016-12-22T18:41:02Z

Coverage decreased (-0.3%) to 92.952% when pulling 607e5520c9f1f9d36aab70c6121eb84cb7287403 on marshallpierce:more-specific-error-for-der into 98b46d8 on briansmith:master.

coveralls · 2016-12-24T01:48:01Z

Coverage increased (+0.001%) to 92.884% when pulling 22c57624c635d2a5cbe50a79de54d26ca7fcf228 on marshallpierce:more-specific-error-for-der into e3a804a on briansmith:master.

marshallpierce · 2016-12-24T15:45:50Z

Does appveyor normally take this long? It's been a day and they still haven't run their builds.

marshallpierce · 2016-12-27T15:05:20Z

I think appveyor is stuck; is there a way to trigger new builds without pushing a bogus commit?

briansmith

Please add the contributor's statement from https://github.com/briansmith/ring#contributing to the end of each commit. Also, please rebase the changes on top of the new changes in #378. (It may make sense to wait for #378 to land first).

briansmith · 2017-02-07T21:59:38Z

src/der.rs


 pub const CONSTRUCTED: u8 = 1 << 5;
 pub const CONTEXT_SPECIFIC: u8 = 2 << 6;

+/// An error in ASN1 parsing. Normal hygiene applies: if parsing secret


I think we should the “Normal hygiene applies...” comment. There's no API that we expose that applies to secret ASN.1 data being parsed anyway.

briansmith · 2017-02-07T22:00:15Z

src/der.rs

+/// An error in ASN1 parsing. Normal hygiene applies: if parsing secret
+/// DER, do not differentiate this from any other error.
+#[derive(Clone, Copy)]
+pub struct ASN1Error;


I think ASN1Error is fine even because I can't stand Asn1Error or similar capitalization of acronyms.

briansmith · 2017-02-07T22:00:33Z

src/der.rs


 pub const CONSTRUCTED: u8 = 1 << 5;
 pub const CONTEXT_SPECIFIC: u8 = 2 << 6;

+/// An error in ASN1 parsing. Normal hygiene applies: if parsing secret
+/// DER, do not differentiate this from any other error.
+#[derive(Clone, Copy)]


Should we derive Debug as well?

briansmith · 2017-02-07T22:02:30Z

src/der.rs

+
+impl From<untrusted::EndOfInput> for ASN1Error {
+    fn from(_: untrusted::EndOfInput) -> Self { ASN1Error }
+}


I don't think this is correct. It is true that in this module, any time we encounter end-of-input, it's because of bad ASN.1. But, in generally it doesn't make sense to convert any or all EndOfInput errors to ASN1Error. Instead it has to be decided on a case-by-case basis.

Instead, we probably need to create wrapper functions around Reader::read_byte() and friends that map EndOfInput to ASN1Error, and use them here instead of using Input's methods directly.

briansmith · 2017-02-07T22:09:13Z

src/der.rs

@@ -117,26 +125,26 @@ fn nonnegative_integer<'a>(input: &mut untrusted::Reader<'a>, min_value: u8)

    let value = try!(expect_tag_and_get_value(input, Tag::Integer));

-    value.read_all(error::Unspecified, |input| {
+    value.read_all(ASN1Error, |input| {


I don't think all the errors returned by this function are ASN.1 decoding errors. Some of them are value-out-of-range errors. Since the point of this PR is to allow us to differentiate different kinds of errors, it seems like we should differentiate out-of-range vs. bad-DER. WDYT?

The end-goal of this specific PR is to make it possible to rebase the webpki crate on top of the new version of ring that has #378 integrated, right? Presumably, in the future we'll change some public APIs in ring to return more specific errors, by extending the changes in this PR, but that's not a goal of the present PR, right?

Or is the purpose different? The intended purpose matters as far as this review is concerned because (1) this change may not be necessary for rebasing webpki on top of #378 and/or (2) this PR may not be sufficient for solving the intended problem if it doesn't differentiate bad-DER from other kinds of errors.

Since the point of this PR is to allow us to differentiate different kinds of errors, it seems like we should differentiate out-of-range vs. bad-DER. WDYT?

I would love to; the only reason I didn't do so was to hew more closely to the "expose as little as possible" goal. More on this below.

The end-goal of this specific PR is to make it possible to rebase the webpki crate on top of the new version of ring that has #378 integrated, right?

It would be possible, though a trifle code-smelly, to use this (plus #378) in webpki. The issue is the, ahem, approximate error handling like this, which is currently residing in #378:

try!(der::expect_tag_and_get_value(input, der::Tag::Sequence) .map_err(|_| ParseSPKIError::BadDER));

It would be nice to not map Unspecified to BadDER just because it seems like that's probably what's going on... and sure enough, webpki has its own way of doing this:

#[inline(always)] pub fn expect_tag_and_get_value<'a>(input: &mut untrusted::Reader<'a>, tag: Tag) -> Result<untrusted::Input<'a>, Error> { ring::der::expect_tag_and_get_value(input, tag).map_err(|_| Error::BadDER) }

Mapping Unspecified to BadDER may be correct, but it doesn't feel very good.

Anyway, consider webpki's parse_spki_value:

fn parse_spki_value(input: untrusted::Input) -> Result<SubjectPublicKeyInfo, Error> { input.read_all(Error::BadDER, |input| { let algorithm_id_value = try!(der::expect_tag_and_get_value(input, der::Tag::Sequence)); let key_value = try!(der::bit_string_with_no_unused_bits(input)); ...

In practice, the only Error variant this will return is BadDER. So, when webpki uses ring for its spki parsing needs and effectively replaces that function, it could quite neatly map ParseSPKIError::BadDER to Error::BadDER and call it a day, with or without a better breakdown of ASN.1 errors that would presumably all get mapped to ParseSPKIError::BadDER (maybe with a field in that variant to store which specific ASN1Error it was?).

In summary: no, I don't think we need more detailed errors to make it possible to use this in webpki. However, I wholeheartedly agree that it is not a very nice state of the world to report an value-out-of-bounds error the same way as an unexpected EOF, so I would happily make ASN1Error an enum with UnexpectedEOF, ValueOutOfBounds, etc variants if you're OK with that (or some other error scheme, though that's what came to mind). Based on your other comments, it sounds like parsing ASN.1 is not itself considered sensitive, so more detailed information about its parsing should not be a problem, and you know how much I love building to allow testability... :)

…ing errors, as per briansmith#379 I agree to license my contributions to each file under the terms given at the top of each file I changed.

implementing From<untrusted::EndOfInput>. Also, other minor PR feedback. I agree to license my contributions to each file under the terms given at the top of each file I changed.

marshallpierce · 2017-02-13T05:46:38Z

src/der.rs

        },
    };

-    let inner = try!(input.skip_and_get_input(length));
+    let inner = try!(input.skip_and_get_input(length).map_err(|_| ASN1Error));


There was only one usage of skip_and_get_input so I just mapped the error inline rather than with a wrapper function.

coveralls · 2017-02-13T07:15:31Z

Coverage decreased (-0.006%) to 93.565% when pulling 66b8e6b on marshallpierce:more-specific-error-for-der into 191fcd2 on briansmith:master.

coveralls · 2017-02-13T11:05:50Z

Coverage decreased (-0.006%) to 93.565% when pulling 66b8e6b on marshallpierce:more-specific-error-for-der into 191fcd2 on briansmith:master.

briansmith · 2018-12-16T04:18:53Z

@marshallpierce Only now have I had time to return to this. Are you interested in bringing this PR up to date? If so, I would be able to review and merge the updated PR right away.

FWIW, I have decided to start using the Asn1 vs. ASN1 capitalization like everybody else. I already modified RSA key loading to report more specific errors in 9968baa.

marshallpierce · 2018-12-16T17:29:00Z

I'm interested, but pretty busy. I'll get to it but it might be months...

briansmith · 2019-02-05T21:45:01Z

Thanks @marshallpierce. I have a branch that does this that we can use instead. It was a mistake not to merge your PR before. Sorry!

marshallpierce commented Dec 19, 2016

View reviewed changes

briansmith added this to the 0.7.1 milestone Dec 22, 2016

This was referenced Jan 23, 2017

Can't run example with system certificate rustls/rustls#26

Closed

Restrict RSA *private* keys to public moduli that are a multiple of 1024 bits. #401

Closed

briansmith requested changes Feb 7, 2017

View reviewed changes

Use a more specific error instead of unspecified::Error for DER encod…

f30d82f

…ing errors, as per briansmith#379 I agree to license my contributions to each file under the terms given at the top of each file I changed.

marshallpierce force-pushed the more-specific-error-for-der branch from 22c5762 to f30d82f Compare February 13, 2017 04:38

Wrap read_byte() calls and map error to ASN1Error instead of

66b8e6b

implementing From<untrusted::EndOfInput>. Also, other minor PR feedback. I agree to license my contributions to each file under the terms given at the top of each file I changed.

marshallpierce commented Feb 13, 2017

View reviewed changes

briansmith mentioned this pull request Mar 6, 2017

.der enocded RSA public key in test does not appear to be a valid key #470

Closed

briansmith modified the milestones: 0.8.n, 0.7.n Apr 10, 2017

heartsucker mentioned this pull request May 6, 2017

Replace error::Unspecified with something just a little more useful #529

Closed

briansmith closed this Feb 5, 2019

juliankrieger mentioned this pull request Jul 15, 2024

Better documentation on how to use / convert OpenSSL generated EC keys with ring #2125

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a more specific error instead of unspecified::Error for DER encoding errors, as per #379 #384

Use a more specific error instead of unspecified::Error for DER encoding errors, as per #379 #384

marshallpierce commented Dec 19, 2016

marshallpierce Dec 19, 2016

briansmith Dec 21, 2016

marshallpierce Dec 19, 2016

briansmith Feb 7, 2017

coveralls commented Dec 21, 2016 •

edited

Loading

coveralls commented Dec 22, 2016 •

edited

Loading

coveralls commented Dec 24, 2016 •

edited

Loading

marshallpierce commented Dec 24, 2016

marshallpierce commented Dec 27, 2016

briansmith left a comment

briansmith Feb 7, 2017

briansmith Feb 7, 2017

briansmith Feb 7, 2017

briansmith Feb 7, 2017

briansmith Feb 7, 2017

marshallpierce Feb 13, 2017

marshallpierce Feb 13, 2017

coveralls commented Feb 13, 2017 •

edited

Loading

coveralls commented Feb 13, 2017 •

edited

Loading

briansmith commented Dec 16, 2018

marshallpierce commented Dec 16, 2018

briansmith commented Feb 5, 2019

Use a more specific error instead of unspecified::Error for DER encoding errors, as per #379 #384

Use a more specific error instead of unspecified::Error for DER encoding errors, as per #379 #384

Conversation

marshallpierce commented Dec 19, 2016

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

coveralls commented Dec 21, 2016 • edited Loading

coveralls commented Dec 22, 2016 • edited Loading

coveralls commented Dec 24, 2016 • edited Loading

marshallpierce commented Dec 24, 2016

marshallpierce commented Dec 27, 2016

briansmith 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

coveralls commented Feb 13, 2017 • edited Loading

coveralls commented Feb 13, 2017 • edited Loading

briansmith commented Dec 16, 2018

marshallpierce commented Dec 16, 2018

briansmith commented Feb 5, 2019

coveralls commented Dec 21, 2016 •

edited

Loading

coveralls commented Dec 22, 2016 •

edited

Loading

coveralls commented Dec 24, 2016 •

edited

Loading

coveralls commented Feb 13, 2017 •

edited

Loading

coveralls commented Feb 13, 2017 •

edited

Loading