Skip to content

der, x509-cert: support for signed bigints, negative serial numbers #823

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.

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

Merged
merged 6 commits into from
Dec 30, 2022

Conversation

woodruffw
Copy link
Contributor

@woodruffw woodruffw commented Dec 28, 2022

WIP; needs unit tests. Just pushing up for visibility.

N.B.: My editor made some automatic reformatting decisions (I use cargo +nightly fmt by default), so let me know if you'd like me to revert them. They should be a superset of the existing cargo fmt style, however.

Fixes #821.

Comment on lines -37 to +56
/// Get the encoded length for the given unsigned integer serialized as bytes.
/// Get the encoded length for the given signed integer serialized as bytes.
#[inline]
pub(super) fn encoded_len(bytes: &[u8]) -> Result<Length> {
Length::try_from(strip_leading_ones(bytes).len())
}

/// Strip the leading all-ones bytes from the given byte slice.
fn strip_leading_ones(mut bytes: &[u8]) -> &[u8] {
pub(crate) fn strip_leading_ones(mut bytes: &[u8]) -> &[u8] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

N.B.: I updated these comments; based on the context and behavior it looks like they were incorrectly copy-pasted before.

@woodruffw woodruffw marked this pull request as ready for review December 28, 2022 22:26
@woodruffw
Copy link
Contributor Author

woodruffw commented Dec 28, 2022

This should be good for a review; I've added some basic tests and confirmed that no other tests have broken with SerialNumber using an Int as its backing representation, but I'll also add the actual negative SerialNumber example in #821 as a smoke test.

Edit: Done.

@woodruffw woodruffw changed the title der: initial support for signed bigints der, x509-cert: support for signed bigints, negative serial numbers Dec 28, 2022
@tarcieri tarcieri merged commit 1c001b3 into RustCrypto:master Dec 30, 2022
@woodruffw woodruffw deleted the ww/signed-bigint branch December 30, 2022 03:29
@baloo
Copy link
Member

baloo commented Dec 30, 2022

I'm a bit late but, here is the relevant rfc piece I found about this https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.2,

anyway, this is in line with the rfc:

   Note: Non-conforming CAs may issue certificates with serial numbers
   that are negative or zero.  Certificate users SHOULD be prepared to
   gracefully handle such certificates.

@woodruffw
Copy link
Contributor Author

Yep! I probably should have included a comment to that effect 🙂

@tarcieri
Copy link
Member

I can ask some relevant X.509 experts on recommendations for interpreting that SHOULD.

It would probably be best to have constructors only accept Uint, even if it can decode Int.

@woodruffw
Copy link
Contributor Author

It would probably be best to have constructors only accept Uint, even if it can decode Int.

Yeah, that makes sense to me. The API change I'm thinking about in #826 (comment) would address this I think -- Int would become a sum type over signed/unsigned, and the SerialNumber::new ctor could then only accept the Int(Unsigned(...)) variant.

tarcieri added a commit that referenced this pull request Jan 2, 2023
Per #823 and #827, making PKCS#7 work interoperably will involve
supporting at limited number of BER productions, one of which is
indefinite lengths.

The current built-in `Length` type rejects them, as a proper DER parser
is expected to.

This commit adds a separate `IndefiniteLength` type as a newtype of
`Option<Length>` with support for parsing indefinite lengths.
tarcieri added a commit that referenced this pull request Jan 2, 2023
Per #823 and #827, making PKCS#7 work interoperably will involve
supporting at limited number of BER productions, one of which is
indefinite lengths.

The current built-in `Length` type rejects them, as a proper DER parser
is expected to.

This commit adds a separate `IndefiniteLength` type as a newtype of
`Option<Length>` with support for parsing indefinite lengths.
tarcieri added a commit that referenced this pull request Jan 2, 2023
Per #823 and #827, making PKCS#7 work interoperably will involve
supporting at limited number of BER productions, one of which is
indefinite lengths.

The current built-in `Length` type rejects them, as a proper DER parser
is expected to.

This commit adds a separate `IndefiniteLength` type as a newtype of
`Option<Length>` with support for parsing indefinite lengths.
@tarcieri tarcieri mentioned this pull request Feb 27, 2023
@tarcieri tarcieri mentioned this pull request Mar 19, 2023
@tarcieri tarcieri mentioned this pull request Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x509-cert: parse failure on publicly known certificate
3 participants