From: pcarana Date: Mon, 4 Nov 2019 21:09:09 +0000 (-0600) Subject: Validate PK when subject is duplicated, log warning when PK is diff X-Git-Tag: v1.2.0~60 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=271bcfa18b03790cd0cdbd3446269357e721c2e7;p=thirdparty%2FFORT-validator.git Validate PK when subject is duplicated, log warning when PK is diff --- diff --git a/src/cert_stack.c b/src/cert_stack.c index 61943e44..a532d7f2 100644 --- a/src/cert_stack.c +++ b/src/cert_stack.c @@ -460,15 +460,19 @@ x509stack_store_serial(struct cert_stack *stack, BIGNUM *number) /** * Intended to validate subject uniqueness. * "Stores" the subject in the current relevant certificate metadata, and - * complains if there's a collision. That's all. + * complains if there's a collision. The @cb should check the primary key of + * the subject, it will be called when a subject isn't unique (certificate + * shares the subject but not the public key). That's all. */ int -x509stack_store_subject(struct cert_stack *stack, struct rfc5280_name *subject) +x509stack_store_subject(struct cert_stack *stack, struct rfc5280_name *subject, + subject_pk_check_cb cb, void *arg) { struct metadata_node *meta; struct subject_name *cursor; array_index i; struct subject_name duplicate; + bool duplicated; int error; /* @@ -481,9 +485,10 @@ x509stack_store_subject(struct cert_stack *stack, struct rfc5280_name *subject) * * Does the last sentence have any significance to us? I don't even * understand why the requirement exists. 5280 and 6487 don't even - * define "entity." I guess it's the same meaning from "End-Entity", - * and in this case it's supposed to function as a synonym for "subject - * name." + * define "entity." We'll take the closest definition from the context, + * specifically from RFC 6484 or RFC 6481 (both RFCs don't define + * "entity" explicitly, but use the word in a way that it can be + * inferred what it means). * * "An issuer SHOULD use a different * subject name if the subject's key pair has changed (i.e., when the CA @@ -504,14 +509,9 @@ x509stack_store_subject(struct cert_stack *stack, struct rfc5280_name *subject) * (But maybe issue a warning. It sounds like the two children can * impersonate each other.) * - Certificates share name and public key. This likely means that we - * are looking at two different versions of the same certificate, but - * we can't tell for sure right now, and so we should accept it. + * are looking at two different versions of the same certificate. + * Accept. (see rfc6484#section-4.7.1 for an example) * - * This is all conjecture. We should probably mail the IETF. - * - * TODO (next iteration) The code below complains when certificates - * share names, and ignores public keys. I've decided to defer the - * fixing. */ meta = SLIST_FIRST(&stack->metas); @@ -519,8 +519,16 @@ x509stack_store_subject(struct cert_stack *stack, struct rfc5280_name *subject) return 0; /* The TA lacks siblings, so subject is unique. */ /* See the large comment in certstack_x509_store_serial(). */ + duplicated = false; ARRAYLIST_FOREACH(&meta->subjects, cursor, i) { if (x509_name_equals(cursor->name, subject)) { + error = cb(&duplicated, cursor->file, arg); + if (error) + return error; + + if (!duplicated) + continue; + char const *serial = x509_name_serialNumber(subject); pr_warn("Subject name '%s%s%s' is not unique. (Also found in '%s'.)", x509_name_commonName(subject), diff --git a/src/cert_stack.h b/src/cert_stack.h index 4ff617c3..6c64783c 100644 --- a/src/cert_stack.h +++ b/src/cert_stack.h @@ -48,7 +48,9 @@ X509 *x509stack_peek(struct cert_stack *); struct rpki_uri *x509stack_peek_uri(struct cert_stack *); struct resources *x509stack_peek_resources(struct cert_stack *); int x509stack_store_serial(struct cert_stack *, BIGNUM *); -int x509stack_store_subject(struct cert_stack *, struct rfc5280_name *); +typedef int (*subject_pk_check_cb)(bool *, char const *, void *); +int x509stack_store_subject(struct cert_stack *, struct rfc5280_name *, + subject_pk_check_cb, void *); STACK_OF(X509) *certstack_get_x509s(struct cert_stack *); int certstack_get_x509_num(struct cert_stack *); diff --git a/src/object/certificate.c b/src/object/certificate.c index c941b18e..c1dff1e3 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -18,6 +18,7 @@ #include "object/bgpsec.h" #include "object/name.h" #include "object/manifest.h" +#include "object/signed_object.h" #include "rsync/rsync.h" /* Just to prevent some line breaking. */ @@ -115,30 +116,13 @@ validate_issuer(X509 *cert, bool is_ta) return 0; } +/* + * Compare public keys, call @diff_alg_cb when the algorithm is different, call + * @diff_pk_cb when the public key is different; return 0 if both are equal. + */ static int -validate_subject(X509 *cert) -{ - struct validation *state; - struct rfc5280_name *name; - int error; - - state = state_retrieve(); - if (state == NULL) - return -EINVAL; - - error = x509_name_decode(X509_get_subject_name(cert), "subject", &name); - if (error) - return error; - pr_debug("Subject: %s", x509_name_commonName(name)); - - error = x509stack_store_subject(validation_certstack(state), name); - - x509_name_put(name); - return error; -} - -static int -spki_cmp(X509_PUBKEY *tal_spki, X509_PUBKEY *cert_spki) +spki_cmp(X509_PUBKEY *tal_spki, X509_PUBKEY *cert_spki, + int (*diff_alg_cb)(void), int (*diff_pk_cb)(void)) { ASN1_OBJECT *tal_alg; ASN1_OBJECT *cert_alg; @@ -158,17 +142,167 @@ spki_cmp(X509_PUBKEY *tal_spki, X509_PUBKEY *cert_spki) return crypto_err("X509_PUBKEY_get0_param() 2 returned %d", ok); if (OBJ_cmp(tal_alg, cert_alg) != 0) - goto different_alg; + return diff_alg_cb(); if (tal_spk_len != cert_spk_len) - goto different_pk; + return diff_pk_cb(); if (memcmp(tal_spk, cert_spk, cert_spk_len) != 0) - goto different_pk; + return diff_pk_cb(); + + return 0; +} +/** Call whenever the public key is different and the subject is the same**/ +static int +cert_different_pk_err(void) +{ + return 1; +} + +/* + * Beware: this function skips a lot (maybe all) RPKI validations for a signed + * object, and clearly expects that the signed object has at least one + * certificate. + * + * Allocates @result, don't forget to release it. + * + * It has been placed here to minimize the risk of misuse. + */ +static int +cert_from_signed_object(struct rpki_uri *uri, uint8_t **result, int *size) +{ + struct signed_object sobj; + ANY_t *cert; + unsigned char *tmp; + int error; + + error = signed_object_decode(&sobj, uri); + if (error) + return error; + + cert = sobj.sdata.decoded->certificates->list.array[0]; + + tmp = malloc(cert->size); + if (tmp == NULL) { + signed_object_cleanup(&sobj); + return pr_enomem(); + } + + memcpy(tmp, cert->buf, cert->size); + + *result = tmp; + (*size) = cert->size; + + signed_object_cleanup(&sobj); return 0; +} + +static int +check_dup_public_key(bool *duplicated, char const *file, void *arg) +{ + X509 *curr_cert = arg; /* Current cert */ + X509 *rcvd_cert; + X509_PUBKEY *curr_pk, *rcvd_pk; + struct rpki_uri *uri; + uint8_t *tmp; + int tmp_size; + int error; + + uri = NULL; + rcvd_cert = NULL; + tmp_size = 0; + + error = uri_create_str(&uri, file, strlen(file)); + if (error) + return error; + + /* Check if it's '.cer', otherwise treat as a signed object */ + if (uri_has_extension(uri, ".cer")) { + error = certificate_load(uri, &rcvd_cert); + if (error) + goto free_uri; + } else { + error = cert_from_signed_object(uri, &tmp, &tmp_size); + if (error) + goto free_uri; + + rcvd_cert = d2i_X509(NULL, (const unsigned char **) &tmp, + tmp_size); + free(tmp); /* Release at once */ + if (rcvd_cert == NULL) { + error = crypto_err("Signed object's '%s' 'certificate' element does not decode into a Certificate", + uri_get_printable(uri)); + goto free_uri; + } + } + + curr_pk = X509_get_X509_PUBKEY(curr_cert); + if (curr_pk == NULL) { + error = crypto_err("X509_get_X509_PUBKEY() returned NULL"); + goto free_cert; + } + + rcvd_pk = X509_get_X509_PUBKEY(rcvd_cert); + if (rcvd_pk == NULL) { + error = crypto_err("X509_get_X509_PUBKEY() returned NULL"); + goto free_cert; + } + + /* + * The function response will be: + * < 0 if there's an error + * = 0 if the PKs are equal + * > 0 if the PKs are different + */ + error = spki_cmp(curr_pk, rcvd_pk, cert_different_pk_err, + cert_different_pk_err); + + if (error < 0) + goto free_cert; + + /* No error, a positive value means the name is duplicated */ + if (error) + (*duplicated) = true; + error = 0; + +free_cert: + X509_free(rcvd_cert); +free_uri: + uri_refput(uri); + return error; +} + +static int +validate_subject(X509 *cert) +{ + struct validation *state; + struct rfc5280_name *name; + int error; + + state = state_retrieve(); + if (state == NULL) + return -EINVAL; + + error = x509_name_decode(X509_get_subject_name(cert), "subject", &name); + if (error) + return error; + pr_debug("Subject: %s", x509_name_commonName(name)); + + error = x509stack_store_subject(validation_certstack(state), name, + check_dup_public_key, cert); + + x509_name_put(name); + return error; +} -different_alg: +static int +root_different_alg_err(void) +{ return pr_err("TAL's public key algorithm is different than the root certificate's public key algorithm."); -different_pk: +} + +static int +root_different_pk_err(void) +{ return pr_err("TAL's public key is different than the root certificate's public key."); } @@ -218,7 +352,8 @@ validate_spki(X509_PUBKEY *cert_spki) goto fail1; } - if (spki_cmp(tal_spki, cert_spki) != 0) + if (spki_cmp(tal_spki, cert_spki, root_different_alg_err, + root_different_pk_err) != 0) goto fail2; X509_PUBKEY_free(tal_spki);