Add CMS Implimentation and support of Windows Autheticode Code Signing #83

RRRadicalEdward · 2021-05-24T15:43:06Z

References:

http://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/authenticode_pe.docx - Windows Authenticode Portable Executable Signature Format
https://datatracker.ietf.org/doc/html/rfc5652 - CMS RFC

…SpcSpOpusInfo

- Remove hardcoded sha256 hash alog in into_win_certificate func - Intermediate certificate is a signer certificate - SignedData::certificates should contain leaf and any intermediate certificates, but doesn't contain the root certificate - Add support of default Certificate::version field processing while deserializing and serializing - Replace SpcLink::File AplicationTag2 with ContextTag2 - Add new_rsa_encypthion_with_sha func to AlgorithmIdentifier

- Add padding processing to the WinCertificate::certificat vector - Add ApplicationTag0 to SpcImageData::SpcLink as otherwise Authenticode singature is invalid - Documentation and general renaming according to the CMS RFC

CBenoit

PKCS7 implementation looks good, but I have a few change requests related to API and others details.

CBenoit · 2021-05-25T14:48:51Z

picky-asn1/src/restricted_string.rs

@@ -285,4 +305,14 @@ mod tests {
    fn valid_utf8_string() {
        Utf8String::from_str("1224na÷日本語はむずかちー−×—«BUeisuteurnt").expect("invalid string");
    }
+
+    #[test]
+    fn valid_unicode_string() {


Suggested change

fn valid_unicode_string() {

fn valid_bmp_string() {

CBenoit · 2021-05-25T14:51:09Z

picky-asn1/src/restricted_string.rs

+    }
+
+    #[test]
+    fn invalid_unicode_string() {


Suggested change

fn invalid_unicode_string() {

fn invalid_bmp_string() {

CBenoit · 2021-05-25T14:57:34Z

picky-asn1-x509/Cargo.toml

@@ -21,6 +21,7 @@ serde = { version = "1.0", features = ["derive"] }
 oid = { version = "0.2", features = ["serde_support"] }
 base64 = "0.13"
 num-bigint-dig = { version = "0.7", optional = true }
+widestring = "0.4.3"


Disable unused features:

Suggested change

widestring = "0.4.3"

widestring = { version = "0.4", default-features = false }

(also we don't specify patch version number in this repo)

CBenoit · 2021-05-25T15:02:40Z

picky-asn1-x509/Cargo.toml

@@ -21,6 +21,7 @@ serde = { version = "1.0", features = ["derive"] }
 oid = { version = "0.2", features = ["serde_support"] }
 base64 = "0.13"
 num-bigint-dig = { version = "0.7", optional = true }
+widestring = "0.4.3"


Put this behind a pkcs7 feature for now

CBenoit · 2021-05-25T15:13:02Z

picky-asn1-x509/src/oids.rs

+    PKCS7 => pkcs7 => "1.2.840.113549.1.7.1",
+    SIGNED_DATA => signed_data => "1.2.840.113549.1.7.2",
+    CONTENT_TYPE => content_type => "1.2.840.113549.1.9.3",
+    MESSAGE_DIGEST => message_digest => "1.2.840.113549.1.9.4",


Move this under the // RSADSI comment in increasing order

CBenoit · 2021-05-25T22:01:54Z

picky-asn1-x509/src/pkcs7/content_info.rs

+/// SpcAttributeTypeAndOptionalValue ::= SEQUENCE {
+///     type                    ObjectID,
+///     value                   [0] EXPLICIT ANY OPTIONAL
+/// }


This should be the docstring of SpcAttributeAndOptionalValue

CBenoit · 2021-05-25T22:02:42Z

picky-asn1-x509/src/pkcs7/content_info.rs

+/// SpcLink ::= CHOICE {
+///     url                     [0] IMPLICIT IA5STRING,
+///     moniker                 [1] IMPLICIT SpcSerializedObject,
+///     file                    [2] EXPLICIT SpcString
+/// } --#public--


Put in SpcLink's docstring

CBenoit · 2021-05-25T22:03:17Z

picky-asn1-x509/src/pkcs7/content_info.rs

+/// SpcString ::= CHOICE {
+///     unicode                 [0] IMPLICIT BMPSTRING,
+///     ascii                   [1] IMPLICIT IA5STRING
+/// }


In SpcString's docstring

CBenoit · 2021-05-25T22:03:32Z

picky-asn1-x509/src/pkcs7/content_info.rs

+/// SpcPeImageFlags ::= BIT STRING {
+///     includeResources            (0),
+///     includeDebugInfo            (1),
+///     includeImportAddressTable   (2)
+/// }


In SpcPeImageFlags's docstring.

CBenoit · 2021-05-25T22:05:18Z

picky-asn1-x509/src/pkcs7/crls.rs

+/// RevocationInfoChoice ::= CHOICE {
+///    crl CertificateList,
+///    other [1] IMPLICIT OtherRevocationInfoFormat }
+///
+/// OtherRevocationInfoFormat ::= SEQUENCE {
+///    otherRevInfoFormat OBJECT IDENTIFIER,
+///    otherRevInfo ANY DEFINED BY otherRevInfoFormat }
+/// ```


Same as above

CBenoit

Thank you! Minor changes are required. As for the CI, I sent a PR to update the rust toolchain, no action is required on your part.

CBenoit · 2021-05-26T17:38:21Z

picky-asn1/src/restricted_string.rs

+        let buffer = data
+            .chunks_exact(2)
+            .into_iter()
+            .map(|elem| u16::from_be_bytes([elem[1], elem[0]]))
+            .collect::<Vec<u16>>();
+
+        String::from_utf16(buffer.as_ref()).is_ok()


Oops, I didn't realize it got stabilized recently. I'll update wayk-builders so it builds, no problem.

CBenoit · 2021-05-26T17:48:04Z

picky-asn1-x509/src/pkcs7/crls.rs

+///  TBSCertList  ::=  SEQUENCE  {
+///         version                 Version OPTIONAL,
+///                                      -- if present, MUST be v2
+///         signature               AlgorithmIdentifier,
+///         issuer                  Name,
+///         thisUpdate              Time,
+///         nextUpdate              Time OPTIONAL,
+///         revokedCertificates     SEQUENCE OF SEQUENCE  {
+///              userCertificate         CertificateSerialNumber,
+///              revocationDate          Time,
+///              crlEntryExtensions      Extensions OPTIONAL
+///                                       -- if present, version MUST be v2
+///                                   }  OPTIONAL,
+///         crlExtensions           [0]  EXPLICIT Extensions OPTIONAL
+///                                       -- if present, version MUST be v2
+///                                 }
+/// ```


Sorry, maybe I missed in the first review, but this one should be in the docstring of TBSCertList

CBenoit · 2021-05-26T17:48:28Z

picky-asn1-x509/src/pkcs7/signed_data.rs

+///       DigestAlgorithmIdentifiers ::= SET OF DigestAlgorithmIdentifier
+///
+///       SignerInfos ::= SET OF SignerInfo


Same as above

CBenoit · 2021-05-26T17:49:27Z

picky-asn1-x509/src/pkcs7/signer_info.rs

+/// SignerIdentifier ::= CHOICE {
+///          issuerAndSerialNumber IssuerAndSerialNumber,
+///          subjectKeyIdentifier [0] SubjectKeyIdentifier }
+///
+/// SignedAttributes ::= SET SIZE (1..MAX) OF Attribute
+///
+/// UnsignedAttributes ::= SET SIZE (1..MAX) OF Attribute
+///
+/// SignatureValue ::= OCTET STRING


Same as above (if we define a type)

CBenoit · 2021-05-26T17:50:23Z

picky/src/hash.rs

@@ -29,6 +47,39 @@ impl From<HashAlgorithm> for rsa::Hash {
        }
    }
 }
+impl TryFrom<HashAlgorithm> for SHAVariant {


Suggested change

impl TryFrom<HashAlgorithm> for SHAVariant {

impl TryFrom<HashAlgorithm> for SHAVariant {

CBenoit · 2021-05-26T17:51:31Z

picky/src/x509/mod.rs

@@ -126,6 +126,11 @@
 //!# Ok(())
 //!# }
 //! ```
+#[cfg(feature = "pkcs7")]


Suggested change

#[cfg(feature = "pkcs7")]

#[cfg(feature = "pkcs7")]

CBenoit · 2021-05-26T17:51:49Z

picky/src/pem.rs

+    #[cfg(not(windows))]
+    // This test should not run on Windows. writeln! add `/r` ending character to Pem in String format on Windows targets.


Suggested change

#[cfg(not(windows))]

// This test should not run on Windows. writeln! add `/r` ending character to Pem in String format on Windows targets.

// This test should not run on Windows. writeln! add `/r` ending character to Pem in String format on Windows targets.

#[cfg(not(windows))]

CBenoit · 2021-05-26T17:52:39Z

picky/src/x509/pkcs7.rs

+pub const AUTHENTICODE_ATTRIBUTES_COUNT: usize = 3;
+


We probably don't need that at all? Is that something you use in consumer code?

Suggested change

pub const AUTHENTICODE_ATTRIBUTES_COUNT: usize = 3;

Yes, We don't need it. I forget to remove it.

CBenoit · 2021-05-26T17:53:11Z

picky/src/x509/pkcs7.rs

+    #[error("Certificates must contain at least Leaf and Intermediate certificates, but got no certificates")]
+    NoCertificates,
+}
+const PKCS7_PEM_LABEL: &str = "PKCS7";


Suggested change

const PKCS7_PEM_LABEL: &str = "PKCS7";

const PKCS7_PEM_LABEL: &str = "PKCS7";

RRRadicalEdward · 2021-05-27T14:26:22Z

@CBenoit I resolved the latest issues. Is there still something that blocks the merging that I missed? 🙂

CBenoit

Thank you, this looks good to me! 😄

(I just moved a docstring around, since it's not much I went ahead and pushed directly)

RRRadicalEdward · 2021-05-27T15:10:58Z

Thank you, this looks good to me! 😄

(I just moved a docstring around, since it's not much I went ahead and pushed directly)

Thank you for the review. picky is awesome.

RRRadicalEdward · 2021-05-27T15:36:27Z

@awakecoding Could you approve this PR?

awakecoding

LGTM

RRRadicalEdward · 2021-05-27T16:23:43Z

@CBenoit Could you merge it? Seems like I cannot merge because of falling CI checks.

awakecoding · 2021-05-27T16:58:21Z

@RRRadicalEdward Benoit has been working on fixing the CI with the Rust update, it should be updated soon

CBenoit · 2021-05-27T17:24:52Z

CI is green!
@RRRadicalEdward I'll publish a new version of picky & ci 🙂

RRRadicalEdward requested review from awakecoding and CBenoit May 24, 2021 15:43

RRRadicalEdward added 15 commits May 24, 2021 18:47

Implimented WinCertificate

14d5024

Implimented content info and related structures

35d9307

Implement PKCS7 related attributes: ContentType, PKCS9MessageDigest, …

fd809b8

…SpcSpOpusInfo

Implimented SingerInfo

549ea1c

Impliment SignedData

da709a0

Add decode test for SignedData. Refactoring

f42e2f4

Impliment pkcs7 struct with into_wincertificate func

4627af2

fix compiling: there was an issue with lifetime

3d68644

Unit tests. Fix implimentation and refactoring.

3e3e8fb

Clean up

24a3b41

Run cargo fmt

3ac84fc

downgrade byteorder version

e90175d

Fixes

eaaa91b

Fixes and documentation

8ccabc8

- Add padding processing to the WinCertificate::certificat vector - Add ApplicationTag0 to SpcImageData::SpcLink as otherwise Authenticode singature is invalid - Documentation and general renaming according to the CMS RFC

RRRadicalEdward force-pushed the authenticode branch from bb9bc91 to 8ccabc8 Compare May 24, 2021 15:55

Run cargo fmt

89bead6

CBenoit requested changes May 25, 2021

View reviewed changes

Refactoring based on the reviewer's suggestions

c573757

CBenoit requested changes May 26, 2021

View reviewed changes

Clean up

0089e55

Last clean up

8730ab0

CBenoit approved these changes May 27, 2021

View reviewed changes

awakecoding approved these changes May 27, 2021

View reviewed changes

CBenoit merged commit 3834f93 into master May 27, 2021

CBenoit deleted the authenticode branch May 27, 2021 17:25

CBenoit mentioned this pull request May 27, 2021

Question on ECParameters #84

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CMS Implimentation and support of Windows Autheticode Code Signing #83

Add CMS Implimentation and support of Windows Autheticode Code Signing #83

RRRadicalEdward commented May 24, 2021

CBenoit left a comment •

edited

Loading

CBenoit May 25, 2021

CBenoit May 25, 2021

CBenoit May 25, 2021

CBenoit May 25, 2021

CBenoit May 25, 2021

CBenoit May 25, 2021

CBenoit May 25, 2021

CBenoit May 25, 2021

CBenoit May 25, 2021

CBenoit May 25, 2021

CBenoit left a comment

CBenoit May 26, 2021

CBenoit May 26, 2021

CBenoit May 26, 2021

CBenoit May 26, 2021

CBenoit May 26, 2021

CBenoit May 26, 2021

CBenoit May 26, 2021

CBenoit May 26, 2021

RRRadicalEdward May 26, 2021

CBenoit May 26, 2021

RRRadicalEdward commented May 27, 2021 •

edited

Loading

CBenoit left a comment •

edited

Loading

RRRadicalEdward commented May 27, 2021 •

edited

Loading

RRRadicalEdward commented May 27, 2021

awakecoding left a comment

RRRadicalEdward commented May 27, 2021

awakecoding commented May 27, 2021

CBenoit commented May 27, 2021

	widestring = "0.4.3"
	widestring = { version = "0.4", default-features = false }

	impl TryFrom<HashAlgorithm> for SHAVariant {

	impl TryFrom<HashAlgorithm> for SHAVariant {

		#[cfg(not(windows))]
		// This test should not run on Windows. writeln! add `/r` ending character to Pem in String format on Windows targets.

	const PKCS7_PEM_LABEL: &str = "PKCS7";

	const PKCS7_PEM_LABEL: &str = "PKCS7";

Add CMS Implimentation and support of Windows Autheticode Code Signing #83

Add CMS Implimentation and support of Windows Autheticode Code Signing #83

Conversation

RRRadicalEdward commented May 24, 2021

CBenoit left a comment • 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

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

CBenoit 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RRRadicalEdward commented May 27, 2021 • edited Loading

CBenoit left a comment • edited Loading

Choose a reason for hiding this comment

RRRadicalEdward commented May 27, 2021 • edited Loading

RRRadicalEdward commented May 27, 2021

awakecoding left a comment

Choose a reason for hiding this comment

RRRadicalEdward commented May 27, 2021

awakecoding commented May 27, 2021

CBenoit commented May 27, 2021

CBenoit left a comment •

edited

Loading

RRRadicalEdward commented May 27, 2021 •

edited

Loading

CBenoit left a comment •

edited

Loading

RRRadicalEdward commented May 27, 2021 •

edited

Loading