Adds SignedData type to `pkcs7` #813

smndtrl · 2022-12-20T15:26:54Z

Adds SignedData type to ContentInfo
Two examples + test cases (Apple MDM Signature and SCEP Request from Apple device) originally BER encoded but reencoded in DER
Requires x509-cert now for some types

Missing

Some types (like AttributeCertificateV2) I haven't encountered
More asserts in the new tests. Currently only tests if e_content ambiguity in PKCS#7 vs CMS standard was handled
ValueOrd implementation.. what makes sense?

pkcs7/src/certificate_choices.rs

woodruffw · 2022-12-20T17:18:23Z

Thanks for opening this! I'll try it with some PKCS7 blobs I have locally.

I had a similar line of work prepared (https://github.com/RustCrypto/formats/compare/master...woodruffw-forks:formats:ww/signeddata?expand=1), but this looks like it's closer to complete.

woodruffw · 2022-12-20T17:19:48Z

pkcs7/src/signed_data_content.rs

+/// ```text
+/// DigestAlgorithmIdentifiers ::= SET OF DigestAlgorithmIdentifier
+/// ```
+type DigestAlgorithmIdentifiers<'a> = SetOfVec<DigestAlgorithmIdentifier<'a>>;


Maybe I did something weird locally, but I'm surprised this isn't complaining about the lack of the alloc feature from the der crate. Did you build with all features enabled locally?

Now that you say it I'm wondering as well. I ran everything without additional features enabled.

Yeah, it built for me locally without additional features as well, strange.

You're pulling in the x509-cert crate which unconditionally activates the alloc feature, and feature unification means it's activated here too.

~~It would probably be good to factor the relevant parts into a common crate which can be shared. We had the x501 crate for this purpose at one point, although I'm not sure that's quite what you need.~~ Edit: oh wait, you need Certificate. Never mind.

Since that's the case, you might as well unconditionally link liballoc and activate the alloc feature of the der crate directly for clarity.

Thanks for explaining!

If there is interest in maintaining a non-alloc part of pkcs7, this is how I did it in my own changes:

[features] alloc = ["der/alloc"] std = ["alloc"] signed-data = ["alloc", "dep:x509-cert"]

(with x509-cert then being marked as optional.)

I'm guessing that, like #765, this crate should probably move to all owned types and make alloc a hard dependency, since heapless support probably won't be practical for real-world use cases

What should we do for now? I don't want to mix this PR with the larger effort of doing something like #765 for this crate as well.

I'm +1 on doing the owned refactor in a follow-up (I can help with that) to keep the diff small 🙂

pkcs7/src/encapsulated_content_info.rs

woodruffw · 2022-12-20T17:32:53Z

With the change in #813 (comment), this gets through most of the MS authroot PKCS7 blob I'm testing with:

$ ./target/debug/examples/demo ~/Downloads/authroot.stl
thread 'main' panicked at 'failed to load CTL: Der(Error { kind: Length { tag: Tag(0x02: INTEGER) }, position: Some(Length(165291)) })', examples/demo.rs:12:29
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

$ wc -c ~/Downloads/authroot.stl
  165282 /Users/william/Downloads/authroot.stl

It seems to jump off the end of the file, though.

For context, here's the file (zipped): authroot.stl.zip

pkcs7/src/cms_version.rs

tarcieri · 2022-12-20T18:18:11Z

@woodruffw weird, I'll take a look when I get a chance, but can you put up the relevant document at https://lapo.it/asn1js/ or something and I can take a look?

It looks like the length isn't being correctly decoded, although I would expect an error in the event it's encountering a BER production it doesn't understand, rather than misdecoding the length.

woodruffw · 2022-12-20T18:26:25Z

weird, I'll take a look when I get a chance, but can you put up the relevant document at https://lapo.it/asn1js/ or something and I can take a look?

It's around 165KB, so it looks like it won't fit into a shareable URL 😅

I narrowed it down to the SET OF SignerInfo, which I've screencapped below:

(The only integers seem to be the CMSVersion for the SignerInfo itself and a 150-bit integer for the SubjectKeyIdentifier).

carl-wallace · 2022-12-20T18:51:12Z

I was able to parse your blob using my fork after changing EncapsulatedContentInfo to use AnyRef instead of OctetStringRef: carl-wallace@6988578. From: William Woodruff ***@***.***> Reply-To: RustCrypto/formats ***@***.***> Date: Tuesday, December 20, 2022 at 1:26 PM To: RustCrypto/formats ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [RustCrypto/formats] Adds SignedData type to `pkcs7` (PR #813) weird, I'll take a look when I get a chance, but can you put up the relevant document at https://lapo.it/asn1js/ or something and I can take a look? It's around 165KB, so it looks like it won't fit into a shareable URL 😅 I narrowed it down to the SET OF SignerInfo, which I've screencapped below: (The only integers seem to be the CMSVersion for the SignerInfo itself and a 150-bit integer for the SubjectKeyIdentifier). — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

woodruffw · 2022-12-20T18:59:20Z

I was able to parse your blob using my fork after changing EncapsulatedContentInfo to use AnyRef instead of OctetStringRef: carl-wallace@6988578.

Thanks for testing. It's weird that it works there, but not here -- are you allowing BER encodings for integers?

smndtrl · 2022-12-20T19:08:52Z

It's me using a u8 instead of UintRef in IssuerAndSerialNumber

woodruffw · 2022-12-20T19:31:06Z

It's me using a u8 instead of UintRef in IssuerAndSerialNumber

I can confirm that makes the parse succeed for me locally! Thanks a ton for debugging!

carl-wallace · 2022-12-20T19:36:49Z

I see it’s OBE now, but I made no changes to tolerate BER (but that’s likely going to be necessary). From: William Woodruff ***@***.***> Reply-To: RustCrypto/formats ***@***.***> Date: Tuesday, December 20, 2022 at 1:59 PM To: RustCrypto/formats ***@***.***> Cc: Carl Wallace ***@***.***>, Comment ***@***.***> Subject: Re: [RustCrypto/formats] Adds SignedData type to `pkcs7` (PR #813) I was able to parse your blob using my fork after changing EncapsulatedContentInfo to use AnyRef instead of OctetStringRef: ***@***.*** Thanks for testing. It's weird that it works there, but not here -- are you allowing BER encoding for integers? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

smndtrl · 2022-12-20T19:52:38Z

I checked @carl-wallace's code and saw a lot of similar code in a new cms crate. Before I put this PR ready for review I wanted to check with you @tarcieri if we should get this PR going with the solution of using AnyRef to deal with the CMS vs PKCS#7 or use a cms crate and look to deal with all the common things in some way?

tarcieri · 2022-12-20T22:32:01Z

@smndtrl add it to pkcs7 for now and we can circle back on @carl-wallace's implementation

woodruffw · 2022-12-21T17:41:43Z

I played around with this a bit, and I noticed that the current EncapsulatedContentInfo handling of e_content means that the e_content's top-level SEQUENCE gets consumed if present, which results in a harder-to-parse slice if the contents are themselves DER-encoded.

On the other hand, if I change it to this:

#[derive(Clone, Copy, Debug, Eq, PartialEq, Sequence)]
pub struct EncapsulatedContentInfo<'a> {
    /// indicates the type of content.
    pub e_content_type: ObjectIdentifier,

    /// encapsulated content
    pub e_content: Option<AnyRef<'a>>,
}

I get back the interior SEQUENCE as expected (minus the context-specific tag somehow, even though I haven't explicitly specified it?)

Curious about thoughts on this -- it surprises me that the current behavior (i.e. with #[asn1(context_specific = "0", optional = "true", tag_mode = "EXPLICIT")] is to consume the SEQUENCE, but it's possible I'm misunderstanding something there.

tarcieri · 2022-12-21T17:47:50Z

@woodruffw if that's true, that sounds like a bug.

Could you put together a minimal reproduction and file an issue?

woodruffw · 2022-12-21T17:49:04Z

To clarify what I mean: without the changes above, the AnyRef's slice points to the highlighted member:

whereas I expect it to point to the parent SEQUENCE of that memebr:

@woodruffw if that's true, that sounds like a bug.

Could you put together a minimal reproduction and file an issue?

Yep, can do!

woodruffw · 2022-12-21T18:10:25Z

Just answering my own question/confusion, based on investigation in #815: I think the behavior here is currently correct, and I was just holding the AnyRef API incorrectly. The correct way to use it is decode_into, not to grab the underlying value() slice and treat that as a complete DER stream.

woodruffw · 2022-12-22T00:54:00Z

@roblabla this work may also be interesting to you (I saw you're also working on Authenticode support in the repo history).

pkcs7/src/lib.rs

pkcs7/src/signer_info.rs

woodruffw

LGTM! I've been testing this locally for the past week, without problems.

I think the only thing that could go into this is sanity-checking the SignedData content based on the CmsVersion, but it looks like x509-cert doesn't do that for its version either. So maybe that's a follow-on item.

tarcieri · 2022-12-29T01:14:07Z

pkcs7/src/cms_version.rs

+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum CmsVersion {
+    /// syntax version 0
+    V0 = 0,
+    /// syntax version 1
+    V1 = 1,
+    /// syntax version 2
+    V2 = 2,
+    /// syntax version 3
+    V3 = 3,
+    /// syntax version 4
+    V4 = 4,
+    /// syntax version 5
+    V5 = 5,
+}
+
+impl FixedTag for CmsVersion {
+    const TAG: Tag = Tag::Integer;
+}
+
+impl From<CmsVersion> for u8 {
+    fn from(version: CmsVersion) -> Self {
+        version as u8
+    }
+}
+
+impl TryFrom<u8> for CmsVersion {
+    type Error = der::Error;
+    fn try_from(byte: u8) -> der::Result<CmsVersion> {
+        match byte {
+            0 => Ok(CmsVersion::V0),
+            1 => Ok(CmsVersion::V1),
+            2 => Ok(CmsVersion::V2),
+            3 => Ok(CmsVersion::V3),
+            4 => Ok(CmsVersion::V4),
+            5 => Ok(CmsVersion::V5),
+            _ => Err(Self::TAG.value_error()),
+        }
+    }
+}
+
+impl<'a> DecodeValue<'a> for CmsVersion {
+    fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> der::Result<CmsVersion> {
+        CmsVersion::try_from(u8::decode_value(reader, header)?)
+    }
+}
+
+impl EncodeValue for CmsVersion {
+    fn value_len(&self) -> der::Result<Length> {
+        u8::from(*self).value_len()
+    }
+
+    fn encode_value(&self, writer: &mut dyn Writer) -> der::Result<()> {
+        u8::from(*self).encode_value(writer)
+    }
+}


I haven't tested this edit specifically to ensure it adds all of these same impls, but you should be able to eliminate much of the boilerplate code here using custom derive

Suggested change

#[derive(Clone, Copy, Debug, Eq, PartialEq)]

pub enum CmsVersion {

/// syntax version 0

V0 = 0,

/// syntax version 1

V1 = 1,

/// syntax version 2

V2 = 2,

/// syntax version 3

V3 = 3,

/// syntax version 4

V4 = 4,

/// syntax version 5

V5 = 5,

}

impl FixedTag for CmsVersion {

const TAG: Tag = Tag::Integer;

}

impl From<CmsVersion> for u8 {

fn from(version: CmsVersion) -> Self {

version as u8

}

}

impl TryFrom<u8> for CmsVersion {

type Error = der::Error;

fn try_from(byte: u8) -> der::Result<CmsVersion> {

match byte {

0 => Ok(CmsVersion::V0),

1 => Ok(CmsVersion::V1),

2 => Ok(CmsVersion::V2),

3 => Ok(CmsVersion::V3),

4 => Ok(CmsVersion::V4),

5 => Ok(CmsVersion::V5),

_ => Err(Self::TAG.value_error()),

}

}

}

impl<'a> DecodeValue<'a> for CmsVersion {

fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> der::Result<CmsVersion> {

CmsVersion::try_from(u8::decode_value(reader, header)?)

}

}

impl EncodeValue for CmsVersion {

fn value_len(&self) -> der::Result<Length> {

u8::from(*self).value_len()

}

fn encode_value(&self, writer: &mut dyn Writer) -> der::Result<()> {

u8::from(*self).encode_value(writer)

}

}

#[derive(Clone, Copy, Debug, Enumerated, Eq, PartialEq)]

#[asn1(type = "INTEGER")]

#[repr(u8)]

pub enum CmsVersion {

/// syntax version 0

V0 = 0,

/// syntax version 1

V1 = 1,

/// syntax version 2

V2 = 2,

/// syntax version 3

V3 = 3,

/// syntax version 4

V4 = 4,

/// syntax version 5

V5 = 5,

}

Seems to work fine just as is. Pushed the change to use the macro

tarcieri

This looks good aside from using custom derive for CmsVersion

jathan · 2022-12-29T03:44:29Z

Would this patch properly parse certificate bundles like http://repo.fpki.gov/fcpca/caCertsIssuedByfcpcag2.p7c? As I was trying to parse it I ran across the same error It "found" the signed content. Perhaps a good test case? (See https://playbooks.idmanagement.gov/fpki/common/certificates/)

I'm new to Rust and was banging my head against the wall trying to figure out why this wasn't working...

woodruffw · 2022-12-29T04:53:55Z

From a quick look, that does indeed look like a PKCS#7 SignedData. So this patch would (hopefully!) parse it, although it would then be up to you to interpret the SignedData's body correctly (and to actually verify the signature on it).

tarcieri · 2022-12-29T21:00:36Z

pkcs7/src/signer_info.rs

+// TODO: figure out what ordering makes sense - if any
+impl ValueOrd for SignerInfo<'_> {
+    fn value_cmp(&self, _other: &Self) -> der::Result<Ordering> {
+        Ok(Ordering::Equal)
+    }
+}


You should be able to derive ValueOrd

That'll end up requiring ValueOrd on CmsVersion and SignerIdentifier (which works nice with just adding ValueOrd there) but

the method `der_cmp` exists for enum `CmsVersion`, but its trait bounds were not satisfied the following trait bounds were not satisfied: `CmsVersion: ValueOrd` which is required by `CmsVersion: DerOrd`

which ends up in when adding ValueOrd

invalid `asn1` attribute (valid options are `tag_mode`)

on CmsVersion

Alright, I can go ahead and merge and see if I can figure it out

Opened #825 to address this

tarcieri · 2022-12-29T22:19:27Z

@smndtrl if you could open a PR which adds test vectors for the original BER-encoded e.g. Apple MDM signature (or just paste the relevant bytes here as e.g. hex), I can see if I can find a solution for handling BER for this specific case

Followup from #813

smndtrl added 5 commits December 20, 2022 16:07

Add SignedData data type

be61382

remove unnecessary base64 dependency

0e4dd77

Fix SignerInfo docs

062734b

clippy: allow large enums

c4e0387

remove signed data from catch all

7cb1639

smndtrl mentioned this pull request Dec 20, 2022

pkcs7: support for RFC5652 5.2.1 #812

Closed

woodruffw reviewed Dec 20, 2022

View reviewed changes

pkcs7/src/certificate_choices.rs Outdated Show resolved Hide resolved

woodruffw reviewed Dec 20, 2022

View reviewed changes

pkcs7/src/encapsulated_content_info.rs Outdated Show resolved Hide resolved

smndtrl added 3 commits December 20, 2022 18:26

derive ValueOrd for CertificateChoices

2a7a5c6

Change content_type to ObjectIdentifier

83a9b2d

remove unused import

52d4356

tarcieri reviewed Dec 20, 2022

View reviewed changes

pkcs7/src/cms_version.rs Outdated Show resolved Hide resolved

smndtrl added 2 commits December 20, 2022 19:35

Rename CMSVersion to CmsVersion

ef41b30

Rest of CmsVersion

2bd8e06

UintRef instead of u8 for IssuerAndSerialNumber

70a4d14

smndtrl added 2 commits December 20, 2022 20:31

rustfmt

a339132

rustfmt on tests as well

dbea0d2

woodruffw mentioned this pull request Dec 21, 2022

DER: Over-eager consumption in AnyRef #815

Closed

roblabla reviewed Dec 23, 2022

View reviewed changes

pkcs7/src/lib.rs Outdated Show resolved Hide resolved

roblabla reviewed Dec 23, 2022

View reviewed changes

pkcs7/src/signer_info.rs Outdated Show resolved Hide resolved

smndtrl added 3 commits December 26, 2022 17:00

make signer_info pub, x509-cert as SerialNumber

61a5d8a

more docs and pub

4650341

fix docs link and test

9b35f6d

smndtrl marked this pull request as ready for review December 26, 2022 17:41

woodruffw approved these changes Dec 28, 2022

View reviewed changes

tarcieri reviewed Dec 29, 2022

View reviewed changes

tarcieri requested changes Dec 29, 2022

View reviewed changes

use Enumerated macro for CmsVersion

90f0834

tarcieri reviewed Dec 29, 2022

View reviewed changes

tarcieri approved these changes Dec 29, 2022

View reviewed changes

tarcieri merged commit 96bc022 into RustCrypto:master Dec 29, 2022

tarcieri added a commit that referenced this pull request Dec 30, 2022

pkcs7: add ValueOrd impls

b4dd5be

Followup from #813

tarcieri mentioned this pull request Dec 30, 2022

pkcs7: add ValueOrd impls #825

Merged

tarcieri added a commit that referenced this pull request Dec 30, 2022

pkcs7: add ValueOrd impls

3f54105

Followup from #813

tarcieri added a commit that referenced this pull request Dec 30, 2022

pkcs7: add ValueOrd impls (#825)

7b59dbf

Followup from #813

smndtrl deleted the pkcs7 branch December 30, 2022 18:32

smndtrl mentioned this pull request Dec 30, 2022

WIP: add test for mixed DER/BER encoding CMS #827

Closed

tarcieri mentioned this pull request Mar 19, 2023

pkcs7 v0.4.0 #936

Merged

pkking mentioned this pull request Nov 9, 2023

Guide about authenticode #1250

Closed

Adds SignedData type to pkcs7 #813

Adds SignedData type to pkcs7 #813

Conversation

smndtrl commented Dec 20, 2022 • edited Loading

woodruffw commented Dec 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarcieri Dec 20, 2022 • edited Loading

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

woodruffw commented Dec 20, 2022

tarcieri commented Dec 20, 2022

woodruffw commented Dec 20, 2022

carl-wallace commented Dec 20, 2022 via email

woodruffw commented Dec 20, 2022 • edited Loading

smndtrl commented Dec 20, 2022

woodruffw commented Dec 20, 2022

carl-wallace commented Dec 20, 2022 via email

smndtrl commented Dec 20, 2022

tarcieri commented Dec 20, 2022

woodruffw commented Dec 21, 2022 • edited Loading

tarcieri commented Dec 21, 2022

woodruffw commented Dec 21, 2022

woodruffw commented Dec 21, 2022

woodruffw commented Dec 22, 2022

woodruffw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarcieri left a comment

Choose a reason for hiding this comment

jathan commented Dec 29, 2022

woodruffw commented Dec 29, 2022 • edited Loading

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

tarcieri commented Dec 29, 2022

Adds SignedData type to `pkcs7` #813

Adds SignedData type to `pkcs7` #813

smndtrl commented Dec 20, 2022 •

edited

Loading

tarcieri Dec 20, 2022 •

edited

Loading

woodruffw commented Dec 20, 2022 •

edited

Loading

woodruffw commented Dec 21, 2022 •

edited

Loading

woodruffw commented Dec 29, 2022 •

edited

Loading