Skip to content

Pkcs7 add support for internal certificates #7160

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

Draft
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

daverodgman
Copy link
Contributor

Description

Contributed by Joakim Sindholt [email protected] under the terms of the DCO, so I've signed it off: see https://lists.trustedfirmware.org/archives/list/[email protected]/thread/WKW5ODIOQKM6FKLFOAEZEFAO47GOTMWU/

I have verified that this doesn't break the existing tests, but haven't done any other checking. It does conflict with the latest version so will need some work to update it, as well as to add tests. This is implemented on top of #7159

Gatekeeper checklist

  • changelog TODO
  • backport not required - not in 2.28
  • tests TODO

Author: Joakim Sindholt <[email protected]>
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman daverodgman added enhancement component-x509 needs-preceding-pr Requires another PR to be merged first size-s Estimated task size: small (~2d) needs-adoption stalled PR that someone should pick up and complete priority-medium Medium priority - this can be reviewed as time permits needs-work labels Feb 23, 2023
Author: Joakim Sindholt <[email protected]>
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman daverodgman force-pushed the pkcs7-add-support-for-internal-certificates branch from ff3e4d8 to 26b150c Compare February 24, 2023 14:45
Copy link

@LionNatsu LionNatsu left a comment

Choose a reason for hiding this comment

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

I found some const to non-const changes, IMHO I think it's a better idea to correct the const-ness of mbedtls_x509_crt_verify, instead of letting it propagate the wrong things to your new code. And because it will further improve (relax) the const-ness restrictions, so that should be source-compatible to the existing code.

}

static mbedtls_x509_crt *pkcs7_find_cert(mbedtls_x509_buf *serial,
mbedtls_x509_crt *cert)

Choose a reason for hiding this comment

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

Both can be const, this is a read-only function.

static int mbedtls_pkcs7_data_or_hash_verify(mbedtls_pkcs7 *pkcs7,
const mbedtls_x509_crt *cert,
mbedtls_x509_crt *cert,

Choose a reason for hiding this comment

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

Why?

&pkcs7->signed_data.certs);
pk_cert != NULL;
pk_cert = pkcs7_find_cert(&signer->serial, pk_cert->next)) {
if (mbedtls_x509_crt_verify(pk_cert, cert,

Choose a reason for hiding this comment

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

It seems a non-const cert is ONLY required by the call to mbedtls_x509_crt_verify here. I'm not sure about this, maybe parameters in mbedtls_x509_crt_*verify family:

mbedtls/library/x509_crt.c

Lines 3110 to 3122 in 3c0e4ef

int mbedtls_x509_crt_verify(mbedtls_x509_crt *crt,
mbedtls_x509_crt *trust_ca,
mbedtls_x509_crl *ca_crl,
const char *cn, uint32_t *flags,
int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *),
void *p_vrfy)
{
return x509_crt_verify_restartable_ca_cb(crt, trust_ca, ca_crl,
NULL, NULL,
&mbedtls_x509_crt_profile_default,
cn, flags,
f_vrfy, p_vrfy, NULL);
}

should be changed to const at the same time too (which will also change x509_crt_verify_chain etc)?

@@ -217,7 +221,7 @@ int mbedtls_pkcs7_parse_der(mbedtls_pkcs7 *pkcs7, const unsigned char *buf,
* \return 0 if the signature verifies, or a negative error code on failure.
*/
int mbedtls_pkcs7_signed_data_verify(mbedtls_pkcs7 *pkcs7,
const mbedtls_x509_crt *cert,
mbedtls_x509_crt *cert,

Choose a reason for hiding this comment

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

This makes the prototype stricter than the original const one, let alone incompatibilities, now it requires a mutable variable and implies potential writes and changes, therefore compiler will behave more conservatively, not good.

@daverodgman daverodgman marked this pull request as draft July 7, 2023 13:00
@daverodgman daverodgman added the historical-reviewing Currently reviewing (for legacy PR/issues) label Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-x509 enhancement historical-reviewing Currently reviewing (for legacy PR/issues) needs-adoption stalled PR that someone should pick up and complete needs-preceding-pr Requires another PR to be merged first needs-work priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants