From: Alberto Leiva Popper Date: Wed, 13 Mar 2019 00:07:53 +0000 (-0600) Subject: Validate signed object signature X-Git-Tag: v0.0.2~72 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fee789050d4418ea187b8ceb7a769d7f993cb553;p=thirdparty%2FFORT-validator.git Validate signed object signature Looks like the chain validation is complete. It's only missing some profile checks. --- diff --git a/src/asn1/decode.c b/src/asn1/decode.c index 4e4f2647..e7adb5eb 100644 --- a/src/asn1/decode.c +++ b/src/asn1/decode.c @@ -30,6 +30,7 @@ asn1_decode(const void *buffer, size_t buffer_size, *result = NULL; + /* TODO (next iteration) first argument is more or less important. */ rval = ber_decode(0, descriptor, result, buffer, buffer_size); if (rval.code != RC_OK) { /* Must free partial object according to API contracts. */ diff --git a/src/asn1/signed_data.c b/src/asn1/signed_data.c index 745f5db6..a5e917ae 100644 --- a/src/asn1/signed_data.c +++ b/src/asn1/signed_data.c @@ -75,8 +75,8 @@ get_sid(struct SignerInfo *sinfo, OCTET_STRING_t **result) } static int -handle_sdata_certificate(ANY_t *any, struct signed_object_args *args, - OCTET_STRING_t *sid) +handle_sdata_certificate(ANY_t *cert_encoded, struct signed_object_args *args, + OCTET_STRING_t *sid, ANY_t *signedData, SignatureValue_t *signature) { struct validation *state; const unsigned char *tmp; @@ -99,9 +99,9 @@ handle_sdata_certificate(ANY_t *any, struct signed_object_args *args, * We definitely don't want @any->buf to be modified, so use a dummy * pointer. */ - tmp = (const unsigned char *) any->buf; + tmp = (const unsigned char *) cert_encoded->buf; - cert = d2i_X509(NULL, &tmp, any->size); + cert = d2i_X509(NULL, &tmp, cert_encoded->size); if (cert == NULL) { error = crypto_err("Signed object's 'certificate' element does not decode into a Certificate"); goto end1; @@ -117,6 +117,9 @@ handle_sdata_certificate(ANY_t *any, struct signed_object_args *args, &policy); if (error) goto end2; + error = certificate_validate_signature(cert, signedData, signature); + if (error) + goto end2; resources_set_policy(args->res, policy); error = certificate_get_resources(cert, args->res); @@ -272,7 +275,8 @@ illegal_attrType: } static int -validate(struct SignedData *sdata, struct signed_object_args *args) +validate(struct SignedData *sdata, ANY_t *sdata_encoded, + struct signed_object_args *args) { struct SignerInfo *sinfo; OCTET_STRING_t *sid = NULL; @@ -322,7 +326,8 @@ validate(struct SignedData *sdata, struct signed_object_args *args) /* * We will validate the certificate later, because we need the sid - * first. + * first. We should also probably validate the signed attributes first + * as well. */ /* rfc6488#section-2.1.5 */ @@ -354,21 +359,6 @@ validate(struct SignedData *sdata, struct signed_object_args *args) if (error) return error; - /* rfc6488#section-2.1.4 */ - /* rfc6488#section-3.1.c 1/2 */ - if (sdata->certificates == NULL) - return pr_err("The SignedData does not contain certificates."); - - if (sdata->certificates->list.count != 1) { - return pr_err("The SignedData contains %d certificates, one expected.", - sdata->certificates->list.count); - } - - error = handle_sdata_certificate(sdata->certificates->list.array[0], - args, sid); - if (error) - return error; - /* rfc6488#section-2.1.6.3 */ /* rfc6488#section-3.1.j 2/2 */ error = is_digest_algorithm(&sinfo->digestAlgorithm, "SignerInfo"); @@ -410,9 +400,22 @@ validate(struct SignedData *sdata, struct signed_object_args *args) if (sinfo->unsignedAttrs != NULL && sinfo->unsignedAttrs->list.count > 0) return pr_err("SignerInfo has at least one unsignedAttr."); + /* rfc6488#section-2.1.4 */ + /* rfc6488#section-3.1.c 1/2 */ /* rfc6488#section-3.2 */ /* rfc6488#section-3.3 */ - /* TODO (field) */ + if (sdata->certificates == NULL) + return pr_err("The SignedData does not contain certificates."); + + if (sdata->certificates->list.count != 1) { + return pr_err("The SignedData contains %d certificates, one expected.", + sdata->certificates->list.count); + } + + error = handle_sdata_certificate(sdata->certificates->list.array[0], + args, sid, sdata_encoded, &sinfo->signature); + if (error) + return error; return 0; } @@ -430,7 +433,7 @@ signed_data_decode(ANY_t *coded, struct signed_object_args *args, if (error) return error; - error = validate(sdata, args); + error = validate(sdata, coded, args); if (error) { signed_data_free(sdata); return error; diff --git a/src/extension.c b/src/extension.c index ab7d53c7..52edb127 100644 --- a/src/extension.c +++ b/src/extension.c @@ -175,6 +175,11 @@ handle_extension(struct extension_handler *handlers, X509_EXTENSION *ext) if (!X509_EXTENSION_get_critical(ext)) return 0; /* Unknown and not critical; ignore it. */ + /* + * TODO (next iteration?) print the NID as string. + * Also "unknown" is misleading. I think it's only "unknown" if the NID + * is -1 or something like that. + */ return pr_err("Certificate has unknown extension. (Extension NID: %d)", nid); dupe: diff --git a/src/log.c b/src/log.c index fe729740..eba8efe5 100644 --- a/src/log.c +++ b/src/log.c @@ -237,7 +237,7 @@ crypto_err(const char *format, ...) * If this function was called, then we need to assume that * there WAS an error; go generic. */ - fprintf(STDERR, "(There are no error messages in the stack.)"); + fprintf(STDERR, "(There are no error messages in libcrypto's stack.)"); error = -EINVAL; } diff --git a/src/object/certificate.c b/src/object/certificate.c index 6602c1dc..ad98ae4e 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -304,6 +304,282 @@ certificate_validate_rfc6487(X509 *cert, bool is_root) return 0; } +struct progress { + size_t offset; + size_t remaining; +}; + +/** + * Skip the "T" part of a TLV. + */ +static int +skip_t(ANY_t *content, struct progress *p, unsigned int tag) +{ + /* + * BTW: I made these errors critical because the signedData is supposed + * to be validated by this point. + */ + + if (content->buf[p->offset] != tag) { + return pr_crit("Expected tag 0x%x, got 0x%x", tag, + content->buf[p->offset]); + } + + if (p->remaining == 0) + return pr_crit("Buffer seems to be truncated"); + p->offset++; + p->remaining--; + return 0; +} + +/** + * Skip the "TL" part of a TLV. + */ +static int +skip_tl(ANY_t *content, struct progress *p, unsigned int tag) +{ + ssize_t len_len; /* Length of the length field */ + ber_tlv_len_t value_len; /* Length of the value */ + int error; + + error = skip_t(content, p, tag); + if (error) + return error; + + len_len = ber_fetch_length(true, &content->buf[p->offset], p->remaining, + &value_len); + if (len_len == -1) + return pr_crit("Could not decipher length (Cause is unknown)"); + if (len_len == 0) + return pr_crit("Buffer seems to be truncated"); + + p->offset += len_len; + p->remaining -= len_len; + return 0; +} + +static int +skip_tlv(ANY_t *content, struct progress *p, unsigned int tag) +{ + int is_constructed; + int skip; + int error; + + is_constructed = BER_TLV_CONSTRUCTED(&content->buf[p->offset]); + + error = skip_t(content, p, tag); + if (error) + return error; + + skip = ber_skip_length(NULL, is_constructed, &content->buf[p->offset], + p->remaining); + if (skip == -1) + return pr_crit("Could not skip length (Cause is unknown)"); + if (skip == 0) + return pr_crit("Buffer seems to be truncated"); + + p->offset += skip; + p->remaining -= skip; + return 0; +} + +/** + * A structure that points to the LV part of a signedAttrs TLV. + */ +struct encoded_signedAttrs { + const uint8_t *buffer; + ber_tlv_len_t size; +}; + +static int +find_signedAttrs(ANY_t *signedData, struct encoded_signedAttrs *result) +{ +#define INTEGER_TAG 0x02 +#define SEQUENCE_TAG 0x30 +#define SET_TAG 0x31 + + struct progress p; + int error; + ssize_t len_len; + + /* Reference: rfc5652-12.1.asn1 */ + + p.offset = 0; + p.remaining = signedData->size; + + /* SignedData: SEQUENCE */ + error = skip_tl(signedData, &p, SEQUENCE_TAG); + if (error) + return error; + + /* SignedData.version: CMSVersion -> INTEGER */ + error = skip_tlv(signedData, &p, INTEGER_TAG); + if (error) + return error; + /* SignedData.digestAlgorithms: DigestAlgorithmIdentifiers -> SET */ + error = skip_tlv(signedData, &p, SET_TAG); + if (error) + return error; + /* SignedData.encapContentInfo: EncapsulatedContentInfo -> SEQUENCE */ + error = skip_tlv(signedData, &p, SEQUENCE_TAG); + if (error) + return error; + /* SignedData.certificates: CertificateSet -> SET */ + error = skip_tlv(signedData, &p, 0xA0); + if (error) + return error; + /* SignedData.signerInfos: SignerInfos -> SET OF SEQUENCE */ + error = skip_tl(signedData, &p, SET_TAG); + if (error) + return error; + error = skip_tl(signedData, &p, SEQUENCE_TAG); + if (error) + return error; + + /* SignedData.signerInfos.version: CMSVersion -> INTEGER */ + error = skip_tlv(signedData, &p, INTEGER_TAG); + if (error) + return error; + /* + * SignedData.signerInfos.sid: SignerIdentifier -> CHOICE -> always + * subjectKeyIdentifier, which is a [0]. + */ + error = skip_tlv(signedData, &p, 0x80); + if (error) + return error; + /* SignedData.signerInfos.digestAlgorithm: DigestAlgorithmIdentifier + * -> AlgorithmIdentifier -> SEQUENCE */ + error = skip_tlv(signedData, &p, SEQUENCE_TAG); + if (error) + return error; + + /* SignedData.signerInfos.signedAttrs: SignedAttributes -> SET */ + /* We will need to replace the tag 0xA0 with 0x31, so skip it as well */ + error = skip_t(signedData, &p, 0xA0); + if (error) + return error; + + result->buffer = &signedData->buf[p.offset]; + len_len = ber_fetch_length(true, result->buffer, + p.remaining, &result->size); + if (len_len == -1) + return pr_crit("Could not decipher length (Cause is unknown)"); + if (len_len == 0) + return pr_crit("Buffer seems to be truncated"); + result->size += len_len; + + return 0; +} + +/* + * TODO (next iteration) there exists a thing called "PKCS7_NOVERIFY", which + * skips unnecessary validations when using the PKCS7 API. Maybe the methods + * we're using have something similar. + */ +int +certificate_validate_signature(X509 *cert, ANY_t *signedData, + SignatureValue_t *signature) +{ + static const uint8_t EXPLICIT_SET_OF_TAG = 0x31; + + X509_PUBKEY *public_key; + EVP_MD_CTX *ctx; + struct encoded_signedAttrs signedAttrs; + int error; + + public_key = X509_get_X509_PUBKEY(cert); + if (public_key == NULL) + return crypto_err("Certificate seems to lack a public key"); + + /* Create the Message Digest Context */ + ctx = EVP_MD_CTX_create(); + if (ctx == NULL) + return crypto_err("EVP_MD_CTX_create() error"); + + if (1 != EVP_DigestVerifyInit(ctx, NULL, EVP_sha256(), NULL, + X509_PUBKEY_get0(public_key))) { + error = crypto_err("EVP_DigestVerifyInit() error"); + goto end; + } + + /* + * When the [signedAttrs] field is present + * (...), + * the result is the message + * digest of the complete DER encoding of the SignedAttrs value + * contained in the signedAttrs field. + * (...) + * A separate encoding + * of the signedAttrs field is performed for message digest calculation. + * The IMPLICIT [0] tag in the signedAttrs is not used for the DER + * encoding, rather an EXPLICIT SET OF tag is used. That is, the DER + * encoding of the EXPLICIT SET OF tag, rather than of the IMPLICIT [0] + * tag, MUST be included in the message digest calculation along with + * the length and content octets of the SignedAttributes value. + * (https://tools.ietf.org/html/rfc5652#section-5.4) + * + * FYI: IMPLICIT [0] is 0xA0, and EXPLICIT SET OF is 0x31. + * + * I can officially declare that these requirements are a gargantuan + * pain in the ass. Through the validation, we need access to the + * signedAttrs thingo in both encoded and decoded versions. + * (We need the decoded version for the sake of profile validation + * during validate_signed_attrs(), and the encoded version to check + * the signature of the SO right here.) + * Getting the encoded version is the problem. We have two options: + * + * 1. Re-encode the decoded version. + * 2. Extract the encoded version from the original BER SignedData. + * + * The first one sounded less efficient but more straightforward, but + * I couldn't pull it off because there's some memory bug with asn1c's + * encoding function that core dumps the fuck out of everything. It's + * caused by undefined behavior that triggers who knows where. + * + * There's another problem with that approach: If we DER-encode the + * signedAttrs, we have no guarantee that the signature will match + * because of the very real possibility that whoever signed used BER + * instead. + * + * One drawback for the second option is that obviously there's no API + * for it. asn1c encodes and decodes; there's no method for extracting + * a particular encoded object out of an encoded container. We need to + * do the parsing ourselves. But it's not that bad because of of + * ber_fetch_length() and ber_skip_length(). + * + * Second option it is. + */ + + error = find_signedAttrs(signedData, &signedAttrs); + if (error) + goto end; + + error = EVP_DigestVerifyUpdate(ctx, &EXPLICIT_SET_OF_TAG, + sizeof(EXPLICIT_SET_OF_TAG)); + if (1 != error) { + error = crypto_err("EVP_DigestVerifyInit() error"); + goto end; + } + + error = EVP_DigestVerifyUpdate(ctx, signedAttrs.buffer, + signedAttrs.size); + if (1 != error) { + error = crypto_err("EVP_DigestVerifyInit() error"); + goto end; + } + + if (1 != EVP_DigestVerifyFinal(ctx, signature->buf, signature->size)) { + error = crypto_err("Signed Object's signature is invalid"); + goto end; + } + + error = 0; + +end: + EVP_MD_CTX_free(ctx); + return error; +} + int certificate_load(struct rpki_uri const *uri, X509 **result) { diff --git a/src/object/certificate.h b/src/object/certificate.h index 22d0a681..643425d7 100644 --- a/src/object/certificate.h +++ b/src/object/certificate.h @@ -2,6 +2,8 @@ #define SRC_OBJECT_CERTIFICATE_H_ #include +#include +#include #include #include "certificate_refs.h" #include "resource.h" @@ -21,6 +23,8 @@ int certificate_validate_chain(X509 *, STACK_OF(X509_CRL) *); */ int certificate_validate_rfc6487(X509 *, bool); +int certificate_validate_signature(X509 *, ANY_t *coded, SignatureValue_t *); + /** * Returns the IP and AS resources declared in the respective extensions. * diff --git a/src/sorted_array.c b/src/sorted_array.c index 35369ca9..ac2fd126 100644 --- a/src/sorted_array.c +++ b/src/sorted_array.c @@ -72,6 +72,11 @@ get_nth_element(struct sorted_array *sarray, unsigned int index) return ((char *)sarray->array) + index * sarray->size; } +/** + * Returns success only if @new can be added to @array. + * (Meaning, returns success if @new is larger than all of the elements in + * @array.) + */ static int compare(struct sorted_array *sarray, void *new) {