From: Alberto Leiva Popper Date: Fri, 8 Feb 2019 19:05:05 +0000 (-0600) Subject: Add RPKI-specific validation for CRLs X-Git-Tag: v0.0.2~93 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c668acd21b388cc602f2739a45bd0f1828d07846;p=thirdparty%2FFORT-validator.git Add RPKI-specific validation for CRLs --- diff --git a/src/Makefile.am b/src/Makefile.am index afaebd34..4ea82830 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -7,11 +7,13 @@ bin_PROGRAMS = rpki_validator rpki_validator_SOURCES = main.c rpki_validator_SOURCES += address.h address.c +rpki_validator_SOURCES += algorithm.h algorithm.c rpki_validator_SOURCES += array_list.h rpki_validator_SOURCES += certificate_refs.h certificate_refs.c rpki_validator_SOURCES += common.h common.c rpki_validator_SOURCES += config.h config.c rpki_validator_SOURCES += debug.h debug.c +rpki_validator_SOURCES += extension.h extension.c rpki_validator_SOURCES += file.h file.c rpki_validator_SOURCES += line_file.h line_file.c rpki_validator_SOURCES += log.h log.c diff --git a/src/algorithm.c b/src/algorithm.c new file mode 100644 index 00000000..4df2d1f7 --- /dev/null +++ b/src/algorithm.c @@ -0,0 +1,19 @@ +#include "algorithm.h" + +#include + +int +rpki_signature_algorithm(void) +{ + return NID_sha256WithRSAEncryption; +} + +int +rpki_public_key_algorithm(void) +{ + /* + * TODO Everyone uses this algorithm, but at a quick glance, it doesn't + * seem to match RFC 7935's public key algorithm. Wtf? + */ + return NID_rsaEncryption; +} diff --git a/src/algorithm.h b/src/algorithm.h new file mode 100644 index 00000000..0bd149fc --- /dev/null +++ b/src/algorithm.h @@ -0,0 +1,15 @@ +#ifndef SRC_ALGORITHM_H_ +#define SRC_ALGORITHM_H_ + +/** + * This file is an implementation of RFC 7935 (previously 6485) and its update, + * 8208. + * + * It's just a bunch of functions that return the NIDs of the algorithms RPKI + * validations are supposed to employ. + */ + +int rpki_signature_algorithm(void); +int rpki_public_key_algorithm(void); + +#endif /* SRC_ALGORITHM_H_ */ diff --git a/src/common.c b/src/common.c index df54b9d2..6ffb3cf8 100644 --- a/src/common.c +++ b/src/common.c @@ -3,6 +3,7 @@ #include #include #include "log.h" +#include "thread_var.h" int NID_rpkiManifest; int NID_signedObject; @@ -10,6 +11,9 @@ int NID_rpkiNotify; int NID_certPolicyRpki; int NID_certPolicyRpkiV2; +/** + * Does not assume that @string is NULL-terminated. + */ int string_clone(void const *string, size_t size, char **clone) { @@ -33,3 +37,134 @@ ia5s2string(ASN1_IA5STRING *ia5, char **result) ? pr_err("CRL URI IA5String has unused bits.") : string_clone(ia5->data, ia5->length, result); } + +/** + * Only prints error message if the result is not 0 nor -ESRCH. + */ +int +x509_name_decode(X509_NAME *name, int nid, char **_result) +{ + char *result; + int len1, len2; + + len1 = X509_NAME_get_text_by_NID(name, nid, NULL, 0); + if (len1 < 0) + return -ESRCH; + + if (_result == NULL) + return 0; + + result = calloc(len1 + 1, sizeof(char)); + if (result == NULL) + return pr_enomem(); + + len2 = X509_NAME_get_text_by_NID(name, nid, result, len1 + 1); + if (len1 != len2) { + free(result); + return pr_err("Likely programming error: X509_NAME_get_text_by_NID() returned inconsistent lengths: %d,%d", + len1, len2); + } + + *_result = result; + return 0; +} + +struct rfc5280_names { + char *commonName; + char *serialNumber; +}; + +static int +get_names(X509_NAME *name, char *what, struct rfc5280_names *result) +{ + int error; + + error = x509_name_decode(name, NID_commonName, &result->commonName); + if (error == -ESRCH) + return pr_err("The '%s' name lacks a commonName attribute."); + if (error) + return error; + + error = x509_name_decode(name, NID_serialNumber, &result->serialNumber); + if (error == -ESRCH) { + result->serialNumber = NULL; + return 0; + } + if (error) { + free(result->commonName); + return error; + } + + return 0; +} + +/** + * Also checks NULL. + * + * Does assume that @str1 and @str2 are NULL-terminated. + */ +static bool str_equals(char const *str1, char const *str2) +{ + if (str1 == str2) + return true; + if (str1 == NULL || str2 == NULL) + return false; + return strcmp(str1, str2) == 0; +} + +int +validate_issuer_name(char const *container, X509_NAME *issuer) +{ + struct validation *state; + X509 *parent; + struct rfc5280_names parent_subject = { 0 }; + struct rfc5280_names child_issuer = { 0 }; + int error; + + /* + * Not sure whether "the CRL issuer is the CA" means that the issuer + * name should equal the parent's subject name or not, because that's + * very much not what rfc6487#section-4.4 is asking us to check. + * But let's check it anyway. + */ + + state = state_retrieve(); + if (state == NULL) + return -EINVAL; + parent = validation_peek_cert(state); + if (parent == NULL) { + return pr_err("%s appears to have no parent certificate.", + container); + } + + error = get_names(X509_get_subject_name(parent), "subject", + &parent_subject); + if (error) + return error; + error = get_names(issuer, "issuer", &child_issuer); + if (error) + goto end2; + + if (strcmp(parent_subject.commonName, child_issuer.commonName) != 0) { + error = pr_err("%s's issuer commonName ('%s') does not equal issuer certificate's commonName ('%s').", + container, parent_subject.commonName, + child_issuer.commonName); + goto end1; + } + + if (!str_equals(parent_subject.serialNumber, + child_issuer.serialNumber)) { + error = pr_err("%s's issuer serialNumber ('%s') does not equal issuer certificate's serialNumber ('%s').", + container, parent_subject.serialNumber, + child_issuer.serialNumber); + goto end1; + } + +end1: + free(child_issuer.commonName); + free(child_issuer.serialNumber); +end2: + free(parent_subject.commonName); + free(parent_subject.serialNumber); + return error; +} diff --git a/src/common.h b/src/common.h index 07024f03..117f6eef 100644 --- a/src/common.h +++ b/src/common.h @@ -27,4 +27,7 @@ extern int NID_certPolicyRpkiV2; int string_clone(void const *, size_t, char **); int ia5s2string(ASN1_IA5STRING *, char **); +int x509_name_decode(X509_NAME *, int, char **); +int validate_issuer_name(char const *, X509_NAME *); + #endif /* SRC_RTR_COMMON_H_ */ diff --git a/src/extension.c b/src/extension.c new file mode 100644 index 00000000..25535baa --- /dev/null +++ b/src/extension.c @@ -0,0 +1,251 @@ +#include "extension.h" + +#include +#include "log.h" +#include "thread_var.h" +#include "crypto/hash.h" + +const struct extension_metadata BC = { + "Basic Constraints", + NID_basic_constraints, + true, +}; +const struct extension_metadata SKI = { + "Subject Key Identifier", + NID_subject_key_identifier, + false, +}; +const struct extension_metadata AKI = { + "Authority Key Identifier", + NID_authority_key_identifier, + false, +}; +const struct extension_metadata KU = { + "Key Usage", + NID_key_usage, + true, +}; +const struct extension_metadata CDP = { + "CRL Distribution Points", + NID_crl_distribution_points, + false, +}; +const struct extension_metadata AIA = { + "Authority Information Access", + NID_info_access, + false, +}; +const struct extension_metadata SIA = { + "Subject Information Access", + NID_sinfo_access , + false, +}; +const struct extension_metadata CP = { + "Certificate Policies", + NID_certificate_policies, + true, +}; +const struct extension_metadata IR = { + "IP Resources", + NID_sbgp_ipAddrBlock, + true, +}; +const struct extension_metadata AR = { + "AS Resources", + NID_sbgp_autonomousSysNum, + true, +}; +const struct extension_metadata CN = { + "CRL Number", + NID_crl_number, + false, +}; + +static int +handle_extension(struct extension_handler *handlers, X509_EXTENSION *ext) +{ + struct extension_handler *handler; + int nid; + + nid = OBJ_obj2nid(X509_EXTENSION_get_object(ext)); + + for (handler = handlers; handler->meta != NULL; handler++) { + if (handler->meta->nid == nid) { + if (handler->found) + goto dupe; + handler->found = true; + + if (handler->meta->critical) { + if (!X509_EXTENSION_get_critical(ext)) + goto not_critical; + } else { + if (X509_EXTENSION_get_critical(ext)) + goto critical; + } + + return handler->cb(ext, handler->arg); + } + } + + if (!X509_EXTENSION_get_critical(ext)) + return 0; /* Unknown and not critical; ignore it. */ + + return pr_err("Certificate has unknown extension. (Extension NID: %d)", + nid); +dupe: + return pr_err("Certificate has more than one '%s' extension.", + handler->meta->name); +not_critical: + return pr_err("Extension '%s' is supposed to be marked critical.", + handler->meta->name); +critical: + return pr_err("Extension '%s' is not supposed to be marked critical.", + handler->meta->name); +} + +int +handle_extensions(struct extension_handler *handlers, + STACK_OF(X509_EXTENSION) const *extensions) +{ + struct extension_handler *handler; + int e; + int error; + + /* TODO check that no other extensions are present? */ + + for (e = 0; e < sk_X509_EXTENSION_num(extensions); e++) { + error = handle_extension(handlers, + sk_X509_EXTENSION_value(extensions, e)); + if (error) + return error; + } + + for (handler = handlers; handler->meta != NULL; handler++) { + if (handler->mandatory && !handler->found) + return pr_err("Certificate is missing the '%s' extension.", + handler->meta->name); + } + + return 0; +} + +int +cannot_decode(struct extension_metadata const *meta) +{ + return pr_err("Extension '%s' seems to be malformed. Cannot decode.", + meta->name); +} + +/** + * Returns 0 if the identifier (ie. SHA-1 hash) of @cert's public key is @hash. + * Otherwise returns error code. + */ +int +validate_public_key_hash(X509 *cert, ASN1_OCTET_STRING *hash) +{ + X509_PUBKEY *pubkey; + const unsigned char *spk; + int spk_len; + int ok; + int error; + + /* + * I really can't tell if this validation needs to be performed. + * Probably not. + * + * "Applications are not required to verify that key identifiers match + * when performing certification path validation." + * (rfc5280#section-4.2.1.2) + * + * From its context, my reading is that the quote refers to the + * "parent's SKI must equal the children's AKI" requirement, not the + * "child's SKI must equal the SHA-1 of its own's SPK" requirement. So + * I think that we're only supposed to check the SHA-1. Or nothing at + * all, because we only care about the keys, not their identifiers. + * + * But the two requirements actually have a lot in common: + * + * The quote is from 5280, not 6487. 6487 chooses to enforce the SKI's + * "SHA-1 as identifier" option, even for the AKI. And if I'm validating + * the AKI's SHA-1, then I'm also indirectly checking the children vs + * parent relationship. + * + * Also, what's with using a hash as identifier? That's an accident + * waiting to happen... + * + * Bottom line, I don't know. But better be safe than sorry, so here's + * the validation. + * + * Shit. I feel like I'm losing so much performance because the RFCs + * are so wishy-washy about what is our realm and what is not. + */ + + /* Get the SPK (ask libcrypto) */ + pubkey = X509_get_X509_PUBKEY(cert); + if (pubkey == NULL) + return crypto_err("X509_get_X509_PUBKEY() returned NULL"); + + ok = X509_PUBKEY_get0_param(NULL, &spk, &spk_len, NULL, pubkey); + if (!ok) + return crypto_err("X509_PUBKEY_get0_param() returned %d", ok); + + /* Hash the SPK, compare SPK hash with the SKI */ + if (hash->length < 0 || SIZE_MAX < hash->length) { + return pr_err("%s length (%d) is out of bounds. (0-%zu)", + SKI.name, hash->length, SIZE_MAX); + } + if (spk_len < 0 || SIZE_MAX < spk_len) { + return pr_err("Subject Public Key length (%d) is out of bounds. (0-%zu)", + spk_len, SIZE_MAX); + } + + error = hash_validate("sha1", hash->data, hash->length, spk, spk_len); + if (error) { + pr_err("The Subject Public Key's hash does not match the %s.", + SKI.name); + } + + return error; +} + +int +handle_aki(X509_EXTENSION *ext, void *arg) +{ + AUTHORITY_KEYID *aki; + struct validation *state; + X509 *parent; + int error; + + aki = X509V3_EXT_d2i(ext); + if (aki == NULL) + return cannot_decode(&AKI); + + if (aki->issuer != NULL) { + error = pr_err("%s extension contains an authorityCertIssuer.", + AKI.name); + goto end; + } + if (aki->serial != NULL) { + error = pr_err("%s extension contains an authorityCertSerialNumber.", + AKI.name); + goto end; + } + + state = state_retrieve(); + if (state == NULL) { + error = -EINVAL; + goto end; + } + + parent = validation_peek_cert(state); + if (parent == NULL) { + error = pr_err("Certificate has no parent."); + goto end; + } + + error = validate_public_key_hash(parent, aki->keyid); + +end: + AUTHORITY_KEYID_free(aki); + return error; +} diff --git a/src/extension.h b/src/extension.h new file mode 100644 index 00000000..e8cd9cde --- /dev/null +++ b/src/extension.h @@ -0,0 +1,44 @@ +#ifndef SRC_EXTENSION_H_ +#define SRC_EXTENSION_H_ + +#include +#include + +struct extension_metadata { + char *name; + int nid; + bool critical; +}; + +struct extension_handler { + struct extension_metadata const *meta; + bool mandatory; + int (*cb)(X509_EXTENSION *, void *); + void *arg; + + void (*free)(void *); + + /* For internal use */ + bool found; +}; + +extern const struct extension_metadata BC; +extern const struct extension_metadata SKI; +extern const struct extension_metadata AKI; +extern const struct extension_metadata KU; +extern const struct extension_metadata CDP; +extern const struct extension_metadata AIA; +extern const struct extension_metadata SIA; +extern const struct extension_metadata CP; +extern const struct extension_metadata IR; +extern const struct extension_metadata AR; +extern const struct extension_metadata CN; + +int handle_extensions(struct extension_handler *, + STACK_OF(X509_EXTENSION) const *); + +int cannot_decode(struct extension_metadata const *); +int validate_public_key_hash(X509 *, ASN1_OCTET_STRING *); +int handle_aki(X509_EXTENSION *, void *); + +#endif /* SRC_EXTENSION_H_ */ diff --git a/src/main.c b/src/main.c index dc44fd62..0ec4ba5a 100644 --- a/src/main.c +++ b/src/main.c @@ -227,6 +227,7 @@ main(int argc, char **argv) if (config_get_shuffle_uris()) tal_shuffle_uris(tal); error = foreach_uri(tal, handle_tal_uri); + /* TODO error on no uris were valid. */ error = (error >= 0) ? 0 : error; tal_destroy(tal); } diff --git a/src/object/certificate.c b/src/object/certificate.c index 4361cc57..c8741bec 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -6,8 +6,10 @@ #include #include +#include "algorithm.h" #include "common.h" #include "config.h" +#include "extension.h" #include "log.h" #include "manifest.h" #include "thread_var.h" @@ -26,75 +28,6 @@ */ typedef AUTHORITY_INFO_ACCESS SIGNATURE_INFO_ACCESS; -struct extension_metadata { - char *name; - int nid; - bool critical; -}; - -static const struct extension_metadata BC = { - "Basic Constraints", - NID_basic_constraints, - true, -}; -static const struct extension_metadata SKI = { - "Subject Key Identifier", - NID_subject_key_identifier, - false, -}; -static const struct extension_metadata AKI = { - "Authority Key Identifier", - NID_authority_key_identifier, - false, -}; -static const struct extension_metadata KU = { - "Key Usage", - NID_key_usage, - true, -}; -static const struct extension_metadata CDP = { - "CRL Distribution Points", - NID_crl_distribution_points, - false, -}; -static const struct extension_metadata AIA = { - "Authority Information Access", - NID_info_access, - false, -}; -static const struct extension_metadata SIA = { - "Subject Information Access", - NID_sinfo_access , - false, -}; -static const struct extension_metadata CP = { - "Certificate Policies", - NID_certificate_policies, - true, -}; -static const struct extension_metadata IR = { - "IP Resources", - NID_sbgp_ipAddrBlock, - true, -}; -static const struct extension_metadata AR = { - "AS Resources", - NID_sbgp_autonomousSysNum, - true, -}; - -struct extension_handler { - struct extension_metadata const *meta; - bool mandatory; - int (*cb)(X509_EXTENSION *, void *); - void *arg; - - void (*free)(void *); - - /* For internal use */ - bool found; -}; - struct ski_arguments { X509 *cert; OCTET_STRING_t *sid; @@ -135,70 +68,51 @@ validate_signature_algorithm(X509 *cert) int nid; nid = OBJ_obj2nid(X509_get0_tbs_sigalg(cert)->algorithm); - if (nid != NID_sha256WithRSAEncryption) + if (nid != rpki_signature_algorithm()) return pr_err("Certificate's Signature Algorithm is not RSASSA-PKCS1-v1_5."); return 0; } static int -validate_name(X509_NAME *name, char *what) +validate_issuer(X509 *cert, bool is_ta) { -#ifdef DEBUG - char *str; -#endif - int str_len; - - str_len = X509_NAME_get_text_by_NID(name, NID_commonName, NULL, 0); - if (str_len < 0) { - pr_err("Certificate's %s lacks the CommonName atribute.", what); - return -ESRCH; - } + X509_NAME *issuer; + int error; -#ifdef DEBUG - str = calloc(str_len + 1, sizeof(char)); - if (str == NULL) { - pr_err("Out of memory."); - return -ENOMEM; - } + issuer = X509_get_issuer_name(cert); - X509_NAME_get_text_by_NID(name, NID_commonName, str, str_len + 1); + if (!is_ta) + return validate_issuer_name("Certificate", issuer); - pr_debug("%s: %s", what, str); - free(str); -#endif + error = x509_name_decode(issuer, NID_commonName, NULL); + if (error == -ESRCH) + pr_err("The 'issuer' name lacks a commonName attribute."); - return 0; + return error; } static int -validate_subject(X509_NAME *name, char *what) +validate_subject(X509 *cert) { struct validation *state; char *subject; - int error, sub_len; - - error = validate_name(name, what); - if (error) - return error; + int error; state = state_retrieve(); if (state == NULL) return -EINVAL; - sub_len = X509_NAME_get_text_by_NID(name, NID_commonName, NULL, 0); - subject = calloc(sub_len + 1, sizeof(char)); - if (subject == NULL) { - pr_err("Out of memory."); - return -ENOMEM; - } - - X509_NAME_get_text_by_NID(name, NID_commonName, subject, sub_len + 1); + error = x509_name_decode(X509_get_subject_name(cert), NID_commonName, + &subject); + if (error == -ESRCH) + pr_err("Certificate's subject lacks the CommonName atribute."); + if (error) + return error; error = validation_store_subject(state, subject); - if (error) - free(subject); + free(subject); return error; } @@ -303,11 +217,7 @@ validate_public_key(X509 *cert, bool is_root) return crypto_err("X509_PUBKEY_get0_param() returned %d", ok); alg_nid = OBJ_obj2nid(alg); - /* - * TODO Everyone uses this algorithm, but at a quick glance, it doesn't - * seem to match RFC 7935's public key algorithm. Wtf? - */ - if (alg_nid != NID_rsaEncryption) { + if (alg_nid != rpki_public_key_algorithm()) { return pr_err("Certificate's public key format is %d, not RSA PKCS#1 v1.5 with SHA-256.", alg_nid); } @@ -366,7 +276,7 @@ certificate_validate_rfc6487(X509 *cert, bool is_root) return error; /* rfc6487#section-4.4 */ - error = validate_name(X509_get_issuer_name(cert), "issuer"); + error = validate_issuer(cert, is_root); if (error) return error; @@ -376,7 +286,7 @@ certificate_validate_rfc6487(X509 *cert, bool is_root) * "An issuer SHOULD use a different subject name if the subject's * key pair has changed" (it's a SHOULD, so [for now] avoid validation) */ - error = validate_subject(X509_get_subject_name(cert), "subject"); + error = validate_subject(cert); if (error) return error; @@ -613,13 +523,6 @@ certificate_get_resources(X509 *cert, struct resources *resources) return 0; } -static int -cannot_decode(struct extension_metadata const *meta) -{ - return pr_err("Extension '%s' seems to be malformed. Cannot decode.", - meta->name); -} - static bool is_rsync(ASN1_IA5STRING *uri) { @@ -688,78 +591,6 @@ handle_bc(X509_EXTENSION *ext, void *arg) return error; } -/** - * Returns 0 if the identifier (ie. SHA-1 hash) of @cert's public key is @hash. - * Otherwise returns error code. - */ -static int -validate_public_key_hash(X509 *cert, ASN1_OCTET_STRING *hash) -{ - X509_PUBKEY *pubkey; - const unsigned char *spk; - int spk_len; - int ok; - int error; - - /* - * I really can't tell if this validation needs to be performed. - * Probably not. - * - * "Applications are not required to verify that key identifiers match - * when performing certification path validation." - * (rfc5280#section-4.2.1.2) - * - * From its context, my reading is that the quote refers to the - * "parent's SKI must equal the children's AKI" requirement, not the - * "child's SKI must equal the SHA-1 of its own's SPK" requirement. So - * I think that we're only supposed to check the SHA-1. Or nothing at - * all, because we only care about the keys, not their identifiers. - * - * But the two requirements actually have a lot in common: - * - * The quote is from 5280, not 6487. 6487 chooses to enforce the SKI's - * "SHA-1 as identifier" option, even for the AKI. And if I'm validating - * the AKI's SHA-1, then I'm also indirectly checking the children vs - * parent relationship. - * - * Also, what's with using a hash as identifier? That's an accident - * waiting to happen... - * - * Bottom line, I don't know. But better be safe than sorry, so here's - * the validation. - * - * Shit. I feel like I'm losing so much performance because the RFCs - * are so wishy-washy about what is our realm and what is not. - */ - - /* Get the SPK (ask libcrypto) */ - pubkey = X509_get_X509_PUBKEY(cert); - if (pubkey == NULL) - return crypto_err("X509_get_X509_PUBKEY() returned NULL"); - - ok = X509_PUBKEY_get0_param(NULL, &spk, &spk_len, NULL, pubkey); - if (!ok) - return crypto_err("X509_PUBKEY_get0_param() returned %d", ok); - - /* Hash the SPK, compare SPK hash with the SKI */ - if (hash->length < 0 || SIZE_MAX < hash->length) { - return pr_err("%s length (%d) is out of bounds. (0-%zu)", - SKI.name, hash->length, SIZE_MAX); - } - if (spk_len < 0 || SIZE_MAX < spk_len) { - return pr_err("Subject Public Key length (%d) is out of bounds. (0-%zu)", - spk_len, SIZE_MAX); - } - - error = hash_validate("sha1", hash->data, hash->length, spk, spk_len); - if (error) { - pr_err("The Subject Public Key's hash does not match the %s.", - SKI.name); - } - - return error; -} - static int handle_ski_ca(X509_EXTENSION *ext, void *arg) { @@ -855,48 +686,6 @@ handle_aki_ta(X509_EXTENSION *aki, void *arg) return -ESRCH; } -static int -handle_aki(X509_EXTENSION *ext, void *arg) -{ - AUTHORITY_KEYID *aki; - struct validation *state; - X509 *parent; - int error; - - aki = X509V3_EXT_d2i(ext); - if (aki == NULL) - return cannot_decode(&AKI); - - if (aki->issuer != NULL) { - error = pr_err("%s extension contains an authorityCertIssuer.", - AKI.name); - goto end; - } - if (aki->serial != NULL) { - error = pr_err("%s extension contains an authorityCertSerialNumber.", - AKI.name); - goto end; - } - - state = state_retrieve(); - if (state == NULL) { - error = -EINVAL; - goto end; - } - - parent = validation_peek_cert(state); - if (parent == NULL) { - error = pr_err("Certificate has no parent."); - goto end; - } - - error = validate_public_key_hash(parent, aki->keyid); - -end: - AUTHORITY_KEYID_free(aki); - return error; -} - static int handle_ku(X509_EXTENSION *ext, unsigned char byte1) { @@ -1238,70 +1027,6 @@ handle_ar(X509_EXTENSION *ext, void *arg) return 0; /* Handled in certificate_get_resources(). */ } -static int -handle_extension(struct extension_handler *handlers, X509_EXTENSION *ext) -{ - struct extension_handler *handler; - int nid; - - nid = OBJ_obj2nid(X509_EXTENSION_get_object(ext)); - - for (handler = handlers; handler->meta != NULL; handler++) { - if (handler->meta->nid == nid) { - if (handler->found) - goto dupe; - handler->found = true; - - if (handler->meta->critical) { - if (!X509_EXTENSION_get_critical(ext)) - goto not_critical; - } else { - if (X509_EXTENSION_get_critical(ext)) - goto critical; - } - - return handler->cb(ext, handler->arg); - } - } - - if (!X509_EXTENSION_get_critical(ext)) - return 0; /* Unknown and not critical; ignore it. */ - - return pr_err("Certificate has unknown extension. (Extension NID: %d)", - nid); -dupe: - return pr_err("Certificate has more than one '%s' extension.", - handler->meta->name); -not_critical: - return pr_err("Extension '%s' is supposed to be marked critical.", - handler->meta->name); -critical: - return pr_err("Extension '%s' is not supposed to be marked critical.", - handler->meta->name); -} - -static int -handle_cert_extensions(struct extension_handler *handlers, X509 *cert) -{ - struct extension_handler *handler; - int e; - int error; - - for (e = 0; e < X509_get_ext_count(cert); e++) { - error = handle_extension(handlers, X509_get_ext(cert, e)); - if (error) - return error; - } - - for (handler = handlers; handler->meta != NULL; handler++) { - if (handler->mandatory && !handler->found) - return pr_err("Certificate is missing the '%s' extension.", - handler->meta->name); - } - - return 0; -} - int certificate_validate_extensions_ta(X509 *cert, struct rpki_uri *mft) { @@ -1318,7 +1043,7 @@ certificate_validate_extensions_ta(X509 *cert, struct rpki_uri *mft) { NULL }, }; - return handle_cert_extensions(handlers, cert); + return handle_extensions(handlers, X509_get0_extensions(cert)); } int @@ -1340,7 +1065,7 @@ certificate_validate_extensions_ca(X509 *cert, struct rpki_uri *mft, { NULL }, }; - return handle_cert_extensions(handlers, cert); + return handle_extensions(handlers, X509_get0_extensions(cert)); } int @@ -1365,7 +1090,7 @@ certificate_validate_extensions_ee(X509 *cert, OCTET_STRING_t *sid, ski_args.cert = cert; ski_args.sid = sid; - return handle_cert_extensions(handlers, cert); + return handle_extensions(handlers, X509_get0_extensions(cert)); } /* Boilerplate code for CA certificate validation and recursive traversal. */ diff --git a/src/object/crl.c b/src/object/crl.c index 6d995ca5..913386bc 100644 --- a/src/object/crl.c +++ b/src/object/crl.c @@ -1,32 +1,66 @@ #include "crl.h" +#include +#include "algorithm.h" +#include "common.h" +#include "extension.h" #include "log.h" +#include "thread_var.h" -static void -print_serials(X509_CRL *crl) +static int +__crl_load(struct rpki_uri const *uri, X509_CRL **result) { -#ifdef DEBUG - STACK_OF(X509_REVOKED) *revokeds; + X509_CRL *crl; + BIO *bio; + int error; + + bio = BIO_new(BIO_s_file()); + if (bio == NULL) + return crypto_err("BIO_new(BIO_s_file()) returned NULL"); + if (BIO_read_filename(bio, uri->local) <= 0) { + error = crypto_err("Error reading CRL '%s'", uri->local); + goto end; + } + + crl = d2i_X509_CRL_bio(bio, NULL); + if (crl == NULL) { + error = crypto_err("Error parsing CRL '%s'", uri->local); + goto end; + } + + *result = crl; + error = 0; + +end: + BIO_free(bio); + return error; +} + +static int +validate_revoked(X509_CRL *crl) +{ + STACK_OF(X509_REVOKED) *revoked_stack; X509_REVOKED *revoked; ASN1_INTEGER const *serial_int; +#ifdef DEBUG BIGNUM *serial_bn; +#endif int i; - revokeds = X509_CRL_get_REVOKED(crl); + revoked_stack = X509_CRL_get_REVOKED(crl); + if (revoked_stack == NULL) + return pr_err("Likely programming error: CRL revoked stack is NULL."); - for (i = 0; i < sk_X509_REVOKED_num(revokeds); i++) { - revoked = sk_X509_REVOKED_value(revokeds, i); - if (revoked == NULL) { - pr_err("??"); - continue; - } + for (i = 0; i < sk_X509_REVOKED_num(revoked_stack); i++) { + revoked = sk_X509_REVOKED_value(revoked_stack, i); serial_int = X509_REVOKED_get0_serialNumber(revoked); if (serial_int == NULL) { - pr_err("??"); - continue; + return pr_err("CRL's revoked entry #%d lacks a serial number.", + i + 1); } +#ifdef DEBUG serial_bn = ASN1_INTEGER_to_BN(serial_int, NULL); if (serial_bn == NULL) { crypto_err("Could not parse revoked serial number"); @@ -38,49 +72,80 @@ print_serials(X509_CRL *crl) BN_print_fp(stdout, serial_bn); fprintf(stdout, "\n"); BN_free(serial_bn); - } #endif + + if (X509_REVOKED_get0_revocationDate(revoked) == NULL) { + return pr_err("CRL's revoked entry #%d lacks a revocation date.", + i + 1); + } + if (X509_REVOKED_get0_extensions(revoked) != NULL) { + return pr_err("CRL's revoked entry #%d has extensions.", + i + 1); + } + } + + return 0; } static int -__crl_load(struct rpki_uri const *uri, X509_CRL **result) +handle_crlnum(X509_EXTENSION *ext, void *arg) { - X509_CRL *crl = NULL; - BIO *bio; + /* + * We're allowing only one CRL per RPP, so there's nothing to do here I + * think. + */ + return 0; +} + +static int +validate_extensions(X509_CRL *crl) +{ + struct extension_handler handlers[] = { + /* ext reqd handler arg */ + { &AKI, true, handle_aki, }, + { &CN, true, handle_crlnum, }, + { NULL }, + }; + + return handle_extensions(handlers, X509_CRL_get0_extensions(crl)); +} + +static int +crl_validate(X509_CRL *crl) +{ + long version; int error; - bio = BIO_new(BIO_s_file()); - if (bio == NULL) - return crypto_err("BIO_new(BIO_s_file()) returned NULL"); - if (BIO_read_filename(bio, uri->local) <= 0) { - error = crypto_err("Error reading CRL '%s'", uri->local); - goto end; + version = X509_CRL_get_version(crl); + if (version != 1) { + return pr_err("CRL version (0x%lx) is not 2 (0x%x).", + version, 1); } - crl = d2i_X509_CRL_bio(bio, NULL); - if (crl == NULL) { - error = crypto_err("Error parsing CRL '%s'", uri->local); - goto end; - } + if (X509_CRL_get_signature_nid(crl) != rpki_signature_algorithm()) + return pr_err("CRL's signature algorithm is not RSASSA-PKCS1-v1_5."); - print_serials(crl); + error = validate_issuer_name("CRL", X509_CRL_get_issuer(crl)); + if (error) + return error; - *result = crl; - error = 0; + error = validate_revoked(crl); + if (error) + return error; -end: - BIO_free(bio); - return error; + return validate_extensions(crl); } int crl_load(struct rpki_uri const *uri, X509_CRL **result) { int error; - pr_debug_add("CRL %s {", uri->global); + error = __crl_load(uri, result); - pr_debug_rm("}"); + if (!error) + error = crl_validate(*result); + pr_debug_rm("}"); return error; }