Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f30d82f
Use a more specific error instead of unspecified::Error for DER encod…
marshallpierce Dec 19, 2016
66b8e6b
Wrap read_byte() calls and map error to ASN1Error instead of
marshallpierce Feb 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 42 additions & 35 deletions src/der.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@
//! This module contains the foundational parts of an ASN.1 DER parser.

use untrusted;
use error;

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

/// An error in ASN1 parsing.
#[derive(Clone, Copy, Debug)]
pub struct ASN1Error;

#[derive(Clone, Copy, PartialEq)]
#[repr(u8)]
pub enum Tag {
Expand All @@ -43,49 +46,49 @@ pub enum Tag {
pub fn expect_tag_and_get_value<'a>(input: &mut untrusted::Reader<'a>,
tag: Tag)
-> Result<untrusted::Input<'a>,
error::Unspecified> {
ASN1Error> {
let (actual_tag, inner) = try!(read_tag_and_get_value(input));
if (tag as usize) != (actual_tag as usize) {
return Err(error::Unspecified);
return Err(ASN1Error);
}
Ok(inner)
}

pub fn read_tag_and_get_value<'a>(input: &mut untrusted::Reader<'a>)
-> Result<(u8, untrusted::Input<'a>),
error::Unspecified> {
let tag = try!(input.read_byte());
ASN1Error> {
let tag = try!(read_expected_byte(input));
if (tag & 0x1F) == 0x1F {
return Err(error::Unspecified); // High tag number form is not allowed.
return Err(ASN1Error); // High tag number form is not allowed.
}

// If the high order bit of the first byte is set to zero then the length
// is encoded in the seven remaining bits of that byte. Otherwise, those
// seven bits represent the number of bytes used to encode the length.
let length = match try!(input.read_byte()) {
let length = match try!(read_expected_byte(input)) {
n if (n & 0x80) == 0 => n as usize,
0x81 => {
let second_byte = try!(input.read_byte());
let second_byte = try!(read_expected_byte(input));
if second_byte < 128 {
return Err(error::Unspecified); // Not the canonical encoding.
return Err(ASN1Error); // Not the canonical encoding.
}
second_byte as usize
},
0x82 => {
let second_byte = try!(input.read_byte()) as usize;
let third_byte = try!(input.read_byte()) as usize;
let second_byte = try!(read_expected_byte(input)) as usize;
let third_byte = try!(read_expected_byte(input)) as usize;
let combined = (second_byte << 8) | third_byte;
if combined < 256 {
return Err(error::Unspecified); // Not the canonical encoding.
return Err(ASN1Error); // Not the canonical encoding.
}
combined
},
_ => {
return Err(error::Unspecified); // We don't support longer lengths.
return Err(ASN1Error); // We don't support longer lengths.
},
};

let inner = try!(input.skip_and_get_input(length));
let inner = try!(input.skip_and_get_input(length).map_err(|_| ASN1Error));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ok((tag, inner))
}

Expand All @@ -100,15 +103,15 @@ pub fn nested<'a, F, R, E: Copy>(input: &mut untrusted::Reader<'a>, tag: Tag,
}

fn nonnegative_integer<'a>(input: &mut untrusted::Reader<'a>, min_value: u8)
-> Result<untrusted::Input<'a>, error::Unspecified> {
-> Result<untrusted::Input<'a>, ASN1Error> {
// Verify that |input|, which has had any leading zero stripped off, is the
// encoding of a value of at least |min_value|.
fn check_minimum(input: untrusted::Input, min_value: u8)
-> Result<(), error::Unspecified> {
input.read_all(error::Unspecified, |input| {
let first_byte = try!(input.read_byte());
-> Result<(), ASN1Error> {
input.read_all(ASN1Error, |input| {
let first_byte = try!(read_expected_byte(input));
if input.at_end() && first_byte < min_value {
return Err(error::Unspecified);
return Err(ASN1Error);
}
let _ = input.skip_to_end();
Ok(())
Expand All @@ -117,26 +120,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| {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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... :)

// Empty encodings are not allowed.
let first_byte = try!(input.read_byte());
let first_byte = try!(read_expected_byte(input));

if first_byte == 0 {
if input.at_end() {
// |value| is the legal encoding of zero.
if min_value > 0 {
return Err(error::Unspecified);
return Err(ASN1Error);
}
return Ok(value);
}

let r = input.skip_to_end();
try!(r.read_all(error::Unspecified, |input| {
let second_byte = try!(input.read_byte());
try!(r.read_all(ASN1Error, |input| {
let second_byte = try!(read_expected_byte(input));
if (second_byte & 0x80) == 0 {
// A leading zero is only allowed when the value's high bit
// is set.
return Err(error::Unspecified);
return Err(ASN1Error);
}
let _ = input.skip_to_end();
Ok(())
Expand All @@ -147,7 +150,7 @@ fn nonnegative_integer<'a>(input: &mut untrusted::Reader<'a>, min_value: u8)

// Negative values are not allowed.
if (first_byte & 0x80) != 0 {
return Err(error::Unspecified);
return Err(ASN1Error);
}

let _ = input.skip_to_end();
Expand All @@ -160,10 +163,10 @@ fn nonnegative_integer<'a>(input: &mut untrusted::Reader<'a>, min_value: u8)
/// numeric value. This is typically used for parsing version numbers.
#[inline]
pub fn small_nonnegative_integer(input: &mut untrusted::Reader)
-> Result<u8, error::Unspecified> {
-> Result<u8, ASN1Error> {
let value = try!(nonnegative_integer(input, 0));
value.read_all(error::Unspecified, |input| {
let r = try!(input.read_byte());
value.read_all(ASN1Error, |input| {
let r = try!(read_expected_byte(input));
Ok(r)
})
}
Expand All @@ -172,28 +175,32 @@ pub fn small_nonnegative_integer(input: &mut untrusted::Reader)
/// any leading zero byte.
#[inline]
pub fn positive_integer<'a>(input: &mut untrusted::Reader<'a>)
-> Result<untrusted::Input<'a>, error::Unspecified> {
-> Result<untrusted::Input<'a>, ASN1Error> {
nonnegative_integer(input, 1)
}

/// Maps EOF to ASN1Error as a convenience when EOF is not expected.
fn read_expected_byte<'a>(input: &mut untrusted::Reader<'a>)
-> Result<u8, ASN1Error>{
input.read_byte().map_err(|_| ASN1Error)
}

#[cfg(test)]
mod tests {
use error;
use super::*;
use untrusted;

fn with_good_i<F, R>(value: &[u8], f: F)
where F: FnOnce(&mut untrusted::Reader)
-> Result<R, error::Unspecified> {
let r = untrusted::Input::from(value).read_all(error::Unspecified, f);
-> Result<R, ASN1Error> {
let r = untrusted::Input::from(value).read_all(ASN1Error, f);
assert!(r.is_ok());
}

fn with_bad_i<F, R>(value: &[u8], f: F)
where F: FnOnce(&mut untrusted::Reader)
-> Result<R, error::Unspecified> {
let r = untrusted::Input::from(value).read_all(error::Unspecified, f);
-> Result<R, ASN1Error> {
let r = untrusted::Input::from(value).read_all(ASN1Error, f);
assert!(r.is_err());
}

Expand Down
2 changes: 1 addition & 1 deletion src/ec/suite_b/ops/ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK to punt on that for now.

let limbs = try!(parse_big_endian_value_in_range(
encoded_value, 1,
&self.public_key_ops.common.n.limbs[
Expand Down
2 changes: 1 addition & 1 deletion src/rsa/bigint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ impl Positive {
// Parses a single ASN.1 DER-encoded `Integer`, which most be positive.
pub fn from_der(input: &mut untrusted::Reader)
-> Result<Positive, error::Unspecified> {
Self::from_be_bytes(try!(der::positive_integer(input)))
Self::from_be_bytes(try!(der::positive_integer(input).map_err(|_| error::Unspecified)))
}

// Turns a sequence of big-endian bytes into a Positive Integer.
Expand Down
4 changes: 2 additions & 2 deletions src/rsa/rsa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ fn parse_public_key(input: untrusted::Input)
error::Unspecified> {
input.read_all(error::Unspecified, |input| {
der::nested(input, der::Tag::Sequence, error::Unspecified, |input| {
let n = try!(der::positive_integer(input));
let e = try!(der::positive_integer(input));
let n = try!(der::positive_integer(input).map_err(|_| error::Unspecified));
let e = try!(der::positive_integer(input).map_err(|_| error::Unspecified));
Ok((n, e))
})
})
Expand Down
3 changes: 2 additions & 1 deletion src/rsa/signing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ impl RSAKeyPair {
-> Result<RSAKeyPair, error::Unspecified> {
input.read_all(error::Unspecified, |input| {
der::nested(input, der::Tag::Sequence, error::Unspecified, |input| {
let version = try!(der::small_nonnegative_integer(input));
let version = try!(der::small_nonnegative_integer(input)
.map_err(|_| error::Unspecified));
if version != 0 {
return Err(error::Unspecified);
}
Expand Down
10 changes: 5 additions & 5 deletions src/rsa/verification.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ pub fn verify_rsa(params: &RSAParameters,
mod tests {
// We intentionally avoid `use super::*` so that we are sure to use only
// the public API; this ensures that enough of the API is public.
use {der, error, signature, test};
use {der, signature, test};
use untrusted;

#[test]
Expand All @@ -178,8 +178,8 @@ mod tests {
// Sanity check that we correctly DER-encoded the originally-
// provided separate (n, e) components. When we add test vectors
// for improperly-encoded signatures, we'll have to revisit this.
assert!(public_key.read_all(error::Unspecified, |input| {
der::nested(input, der::Tag::Sequence, error::Unspecified,
assert!(public_key.read_all(der::ASN1Error, |input| {
der::nested(input, der::Tag::Sequence, der::ASN1Error,
|input| {
let _ = try!(der::positive_integer(input));
let _ = try!(der::positive_integer(input));
Expand Down Expand Up @@ -222,8 +222,8 @@ mod tests {
// Sanity check that we correctly DER-encoded the originally-
// provided separate (n, e) components. When we add test vectors
// for improperly-encoded signatures, we'll have to revisit this.
assert!(public_key.read_all(error::Unspecified, |input| {
der::nested(input, der::Tag::Sequence, error::Unspecified,
assert!(public_key.read_all(der::ASN1Error, |input| {
der::nested(input, der::Tag::Sequence, der::ASN1Error,
|input| {
let _ = try!(der::positive_integer(input));
let _ = try!(der::positive_integer(input));
Expand Down