Pkcs7 pr4 #3

naynajain · 2020-10-25T23:15:57Z

Hi,

I have the second iteration for PKCS7 PR ready to be pushed to upstream community.
I would like to know if any feedbacks. This is still based on 2.16 version and I will update it to "development" branch before sending upstream.

Few notes:

This version supports single signer. However, even for single signer you can have multiple certificates as certificate chain. But it seems it is not easy to differentiate whether the list of certificates are certificate chain or just different certificates. So, I restricted the whole support to single root certificate, and not even certificate chain for the single signer support. Moreover, we do not need also this support, so it is probably ok to leave it like this as of now rather than spending too much time on figuring out how to support it.
As per CRL support, it seems signedData has one more purpose of passing around certificates and CRLs packed as PKCS7 signed data. I couldn't find openssl command line way to specify CRL file while using PKCS7 for signature. It might take it automatically from CA, but not very sure. However, I did find openssl crl2pkcs7 command as specified in the following link. From the following two links, I came to conclusion that CRL works more in combination with certificates to be passed around rather than actually part of PKCS7 signature usecase.
https://www.mkssoftware.com/docs/man1/openssl_crl2pkcs7.1.asp
And if you see the GnuTLS link (https://www.gnutls.org/manual/html_node/Cryptographic-Message-Syntax-_002f-PKCS7.html) , it specifically says this - In certain cases the same format is also used to transport lists of certificates and CRLs.
We do not have such use case for CRL and even from revocation perspective we make use of dbx and not the CRLs atleast as of now. So, again I have strictly made it to not consider CRL. If CRL is added, it will fail while parsing it as signer info.
I did realize that I removed the test for CRL but didn't remove it from Makefile. I will fix that before sending upstream.
I thought too much about error codes. After switching multiple times between narrowing it further to give exact parse fail or to keep it only a high level fail, I finally chose to keep it high level until I understand how people are having issues in interpreting it. I think it will improve over time.

Please let me know if any feedbacks by Tuesday EOD.. so that we can try to send second iteration sometime by end of this week.

Thanks & Regards,
- Nayna

PKCS7 signing format is used by OpenPOWER Key Management, which is using mbedtls as its crypto library. This patch adds the limited support of pkcs7 parser and verification to the mbedtls. The limitations are: * Only signed data is supported. * CRLs are not currently handled. * Single signer is supported. Signed-off-by: Daniel Axtens <[email protected]> Signed-off-by: Nayna Jain <[email protected]>

OpenSSL provides APIs to generate only the signted data format PKCS7 i.e. without content type OID. This patch adds support to parse the data correctly even if formatted only as signed data. Signed-off-by: Nayna Jain <[email protected]>

This patch updates the generate_errors.pl to handle PKCS7 code as well. Signed-off-by: Nayna Jain <[email protected]>

This patch adds the updates generated by running generate_features.pl for pkcs7. Signed-off-by: Nayna Jain <[email protected]>

The patch enables pkcs7 where File I/O may not be available. Reported-by: Eric Richter<[email protected]> Signed-off-by: Daniel Axtens <[email protected]>

daxtens

Hi Nayna!

I've looked at everything except test_suite_pkcs7.function. It looks good! 👍 I have left some minor comments but I'm happy with the overall design and function.

Looking back at https://ibm.ent.box.com/notes/724259293796 - did we conclude on the zero signers case? I think at the moment we will reject a zero signerInfo case, but I think we could support it in future without a big API change: a user could distinguish between a present signerInfo and an absent signerInfo by looking at the version field of signerInfo. It would be 1 if there's a signerInfo and 0 if there isn't. That's a bit inelegant but it should work, so I guess that's probably OK.

Similarly, with zero certificates - I think looking at the code, we will successfully parse a PKCS#7 message with 1 or 0 certificates. If a user wanted to determine if there was a certificate parsed, they could check to see if signed_data->certs.version != 0.

Finally, I'm not sure I fully understood what the mbedTLS team meant when they asked for 'negative tests' in your last PR - do you think you've covered that?

Kind regards,
Daniel

daxtens · 2020-10-29T01:54:28Z

include/mbedtls/check_config.h

+#if ( ( defined(MBEDTLS_PKCS7_C) ) && ( !defined(MBEDTLS_ASN1_PARSE_C) ) && \
+    ( !defined(MBEDTLS_OID_C) ) && ( !defined(MBEDTLD_PK_PARSE_C) ) && \
+    ( !defined(MBEDTLS_X509_CRT_PARSE_C) ) && ( !defined(MBEDTLS_BIGNUM_C) ) )
+#error  "MBEDTLS_PKCS7_C is defined, but not all prerequisites"


I think this will only error if you have none of the prerequisites. Should it be:

#if ( defined(MBEDTLS_PKCS7_C) ) && ( ( !defined(MBEDTLS_ASN1_PARSE_C) ) || \ ( !defined(MBEDTLS_OID_C) ) || ( !defined(MBEDTLD_PK_PARSE_C) ) || \ ( !defined(MBEDTLS_X509_CRT_PARSE_C) ) || ( !defined(MBEDTLS_BIGNUM_C) ) )

?

Thanks for catching it. Fixed.

daxtens · 2020-10-29T01:57:29Z

include/mbedtls/pkcs7.h

+    MBEDTLS_PKCS7_DATA,
+    MBEDTLS_PKCS7_SIGNED_DATA,
+    MBEDTLS_PKCS7_ENVELOPED_DATA,
+    MBEDTLS_PKCS7_SIGNED_AND_ENVELOPED_DAYA,


DATA, not DAYA :)

Super catch. Thanks :-)

daxtens · 2020-10-29T01:58:57Z

include/mbedtls/pkcs7.h

+    int version;
+    mbedtls_x509_buf serial;
+    mbedtls_x509_name issuer;
+    mbedtls_x509_buf issuer_raw;


Minor: Do we need to store issuer_raw if we also have issuer?

Hmm probably no.. I actually added it very initially.. I will recheck it and remove if not needed.

I think I kept it for the reason it is in x509_crt.h
mbedtls_x509_buf issuer_raw; /**< The raw issuer data (DER). Used for quick comparison. */

I think since signer and issuer are supposed to be unique, and might be used for checks, I thought to keep it for reason of quick comparision.

With that I am not removing it.

Thanks & Regards,
- Nayna

daxtens · 2020-10-29T01:59:56Z

include/mbedtls/pkcs7.h

+{
+    int version;
+    mbedtls_pkcs7_buf digest_alg_identifiers;
+    struct mbedtls_pkcs7_data content;


You just established a typedef for this: should you use the typedef instead of the full struct name?

daxtens · 2020-10-29T02:00:09Z

include/mbedtls/pkcs7.h

+    struct mbedtls_pkcs7_data content;
+    mbedtls_x509_crt certs;
+    mbedtls_x509_crl crl;
+    struct mbedtls_pkcs7_signer_info signers;


daxtens · 2020-10-29T05:23:25Z

tests/data_files/Makefile

@@ -1048,6 +1048,99 @@ cert_md5.crt: cert_md5.csr
 	$(MBEDTLS_CERT_WRITE) request_file=$< serial=6 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20190210144406 not_after=20290210144406 md=MD5 version=3 output_file=$@
 all_final += cert_md5.crt

+#PKCS7 test data


Looking at https://github.com/naynajain/mbedtls/pull/3/files#diff-33f05cba6c4be0b2bf38b859d772b1a0527c9d80ad7347aada68eee172bb584aR1023, these comments should probably be #<space>Text

daxtens · 2020-10-29T05:25:45Z

tests/data_files/Makefile

+pkcs7-rsa-sha256-1.crt:
+	$(OPENSSL) req -x509 -subj="/C=NL/O=PKCS7/CN=PKCS7 Cert 1" -sha256 -nodes -days 365  -newkey rsa:2048 -keyout pkcs7-rsa-sha256-1.key -out pkcs7-rsa-sha256-1.crt
+	cat pkcs7-rsa-sha256-1.crt > pkcs7-rsa-sha256-1.pem
+	cat pkcs7-rsa-sha256-1.key >> pkcs7-rsa-sha256-1.pem


you can write cat pkcs7-rsa-sha256-1.crt pkcs7-rsa-sha256-1.key > pkcs7-rsa-sha256-1.pem as cat will automatically concatenate its arguments :)

daxtens · 2020-10-29T05:30:21Z

tests/data_files/Makefile

+#PKCS7 test data
+pkcs7_test_cert_1 = pkcs7-rsa-sha256-1.crt
+pkcs7_test_cert_2 = pkcs7-rsa-sha256-2.crt
+pkcs7_test_file = pkcs7_data.txt


I can't find any other reference to these variables - am I missing something?

Hmm.. I did see other commands where they were using the variables, and thought to do so but then looks like I didn't. I will update the commands to use the variable.

daxtens · 2020-10-29T05:31:10Z

tests/data_files/Makefile

+pkcs7_data.txt:
+	echo "Hello" > $@
+	echo 2 >> pkcs7_data_1.txt
+all_final += pkcs7_data.txt


all_final only contains pkcs7_data.txt, but you've also created pkcs7_data_1.txt. Is this intentional?

yes because that gets created as part of pkcs7_data.txt target. all_final is mainly to add targets.

daxtens · 2020-10-29T05:33:40Z

tests/data_files/Makefile

+pkcs7_data_crl.signed:
+		cat pkcs7-rsa-sha256-1.pem > pkcs7-rsa-crt-crl.pem
+		cat crl_sha256.pem >> pkcs7-rsa-crt-crl.pem
+		$(OPENSSL) smime -sign -binary -in pkcs7_data.txt -out $@ -md sha512 -signer pkcs7-rsa-crt-crl.pem -inkey pkcs7-rsa-sha256-1.key -noattr -outform DER -out $@


If you depend on crl_sha256.pem you should probably add that to the top line (pkcs7_data_crl.signed: crl_sha256.pem). There are a few dependencies like this in your changes.

naynajain · 2020-10-30T22:03:57Z

Hi Nayna!

I've looked at everything except test_suite_pkcs7.function. It looks good! +1 I have left some minor comments but I'm happy with the overall design and function.

Looking back at https://ibm.ent.box.com/notes/724259293796 - did we conclude on the zero signers case? I think at the moment we will reject a zero signerInfo case, but I think we could support it in future without a big API change: a user could distinguish between a present signerInfo and an absent signerInfo by looking at the version field of signerInfo. It would be 1 if there's a signerInfo and 0 if there isn't. That's a bit inelegant but it should work, so I guess that's probably OK.

Thanks Daniel.. We didn't discuss it again, but as I started understanding more.. I felt that
zero signer is a requirement when it used to transport list of certificates and CRLs. Openssl also provides this feature - https://www.mkssoftware.com/docs/man1/openssl_crl2pkcs7.1.asp
I felt atleast as per our requirements I think we are not doing this usecase.. I agree the API should be such that it can be extended easily in future.. then in that case I am hoping that struct will not have to be modified for supporting this atleast.. so as you said.. yes without changing the API

Similarly, with zero certificates - I think looking at the code, we will successfully parse a PKCS#7 message with 1 or 0 certificates. If a user wanted to determine if there was a certificate parsed, they could check to see if signed_data->certs.version != 0.

Finally, I'm not sure I fully understood what the mbedTLS team meant when they asked for 'negative tests' in your last PR - do you think you've covered that?

My understanding from negative tests is like - what if certificate section of ASN is corrupted.. and it fails to parse. so we should have some of those cases. I think I did cover like badcert, badsigner that is by modifing ASN files and ensuring it fails to parse and returns with failure error code. I hope I understood it right.. lets see .. writing test cases had been bit tricky.

Kind regards,
Daniel

Thanks for your review.

Thanks & Regards,
- Nayna

naynajain · 2020-10-30T22:07:07Z

Will include the other feedbacks.

Thanks & Regards,
- Nayna

naynajain · 2020-10-31T00:07:35Z

I just realized.. why don't we use the count for certificates/crls/signers like below.
typedef struct mbedtls_pkcs7_signed_data
{
int version;
mbedtls_pkcs7_buf digest_alg_identifiers;
struct mbedtls_pkcs7_data content;
int no_of_certs;
mbedtls_x509_crt certs;
int no_of_crls;
mbedtls_x509_crl crl;
int no_of_signers;
mbedtls_pkcs7_signer_info signers;
}
mbedtls_pkcs7_signed_data;

Their internal (static) parsing function can return the count or negative error code. As per count, 0 or positive number is valid value.

I think that would really simplify everything. I have added them to pkcs7 signed data struct. Hmm.. the tests are not broken so I am hoping any of the consumers will not be broken by this change.

I am sending PR as of now with this change.. if you think this idea has issues, we will do it asap once I am back..

daxtens · 2020-11-11T23:08:14Z

Just confirming, I think that tracking the number of certs/crls/signers with another variable makes sense. :)

naynajain and others added 5 commits October 25, 2020 01:41

pkcs7: add support for signed data

98ca055

OpenSSL provides APIs to generate only the signted data format PKCS7 i.e. without content type OID. This patch adds support to parse the data correctly even if formatted only as signed data. Signed-off-by: Nayna Jain <[email protected]>

mbedtls: add pkcs7 in generate_errors.pl

1623767

This patch updates the generate_errors.pl to handle PKCS7 code as well. Signed-off-by: Nayna Jain <[email protected]>

mbedtls: generate_features.pl updates features file for pkcs7

e1687e7

This patch adds the updates generated by running generate_features.pl for pkcs7. Signed-off-by: Nayna Jain <[email protected]>

pkcs7: only include file I/O if permitted by configuration

ac97eb0

The patch enables pkcs7 where File I/O may not be available. Reported-by: Eric Richter<[email protected]> Signed-off-by: Daniel Axtens <[email protected]>

daxtens reviewed Oct 29, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pkcs7 pr4 #3

Pkcs7 pr4 #3

naynajain commented Oct 25, 2020

daxtens left a comment •

edited

Loading

daxtens Oct 29, 2020

naynajain Oct 30, 2020

daxtens Oct 29, 2020

naynajain Oct 30, 2020

daxtens Oct 29, 2020

naynajain Oct 30, 2020

naynajain Oct 30, 2020

daxtens Oct 29, 2020

daxtens Oct 29, 2020

daxtens Oct 29, 2020

daxtens Oct 29, 2020

daxtens Oct 29, 2020

naynajain Oct 30, 2020

daxtens Oct 29, 2020

naynajain Oct 30, 2020

daxtens Oct 29, 2020

naynajain commented Oct 30, 2020

naynajain commented Oct 30, 2020

naynajain commented Oct 31, 2020

daxtens commented Nov 11, 2020

Pkcs7 pr4 #3

Are you sure you want to change the base?

Pkcs7 pr4 #3

Conversation

naynajain commented Oct 25, 2020

daxtens 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

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

naynajain commented Oct 30, 2020

naynajain commented Oct 30, 2020

naynajain commented Oct 31, 2020

daxtens commented Nov 11, 2020

daxtens left a comment •

edited

Loading