From: Alberto Leiva Popper Date: Tue, 8 Feb 2022 17:16:28 +0000 (-0600) Subject: Certificate: Remove subject name uniqueness validation X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=0a6a80b558e12304ba0e68c021848e292bfe3ce6;p=thirdparty%2FFORT-validator.git Certificate: Remove subject name uniqueness validation RFC 6487: > An issuer SHOULD use a different subject name if the subject's key > pair has changed (i.e., when the CA issues a certificate as part of > re-keying the subject.) Fort's implementation was problematic. The code was comparing the certificate's subject name and public key to siblings that had potentially not been validated yet. It seems to me this would make it possible for attackers to crash FORT (by posting invalid objects) or to invalidate legitimate objects (by publishing siblings that contained conflicting subject names and public keys, without having to worry about the rest of the fields). This would be somewhat difficult o fix. I asked on the mailing list and Discord ("RPKI Community"), and it seems the concensus is "don't validate it." Subject Names don't really matter that much, because RPKI's primary concern is resource ownership, not identity. Furthermore, I'm not convinced that chopping off branches off the tree because of a clumsy key rollover is a good idea. https://mailarchive.ietf.org/arch/msg/sidrops/mXWbCwh6RO8pAtt7N30Q9m6jUws/ --- diff --git a/src/cert_stack.c b/src/cert_stack.c index 1ec72de7..f5b5ece4 100644 --- a/src/cert_stack.c +++ b/src/cert_stack.c @@ -35,13 +35,6 @@ struct serial_number { STATIC_ARRAY_LIST(serial_numbers, struct serial_number) -struct subject_name { - struct rfc5280_name *name; - char *file; /* File where this subject name was found. */ -}; - -STATIC_ARRAY_LIST(subjects, struct subject_name) - /** * Cached certificate data. */ @@ -54,7 +47,6 @@ struct metadata_node { * don't have many children, and I'm running out of time. */ struct serial_numbers serials; - struct subjects subjects; /** Used by certstack. Points to the next stacked certificate. */ SLIST_ENTRY(metadata_node) next; @@ -146,20 +138,12 @@ serial_cleanup(struct serial_number *serial) free(serial->file); } -static void -subject_cleanup(struct subject_name *subject) -{ - x509_name_put(subject->name); - free(subject->file); -} - static void meta_destroy(struct metadata_node *meta) { uri_refput(meta->uri); resources_destroy(meta->resources); serial_numbers_cleanup(&meta->serials, serial_cleanup); - subjects_cleanup(&meta->subjects, subject_cleanup); free(meta); } @@ -329,11 +313,10 @@ x509stack_push(struct rpki_uri *uri, X509 *x509, enum rpki_policy policy, meta->uri = uri; uri_refget(uri); serial_numbers_init(&meta->serials); - subjects_init(&meta->subjects); error = init_resources(x509, policy, type, &meta->resources); if (error) - goto cleanup_subjects; + goto cleanup_serials; stack = validation_certstack(state_retrieve()); @@ -359,8 +342,7 @@ destroy_separator: free(defer_separator); destroy_resources: resources_destroy(meta->resources); -cleanup_subjects: - subjects_cleanup(&meta->subjects, subject_cleanup); +cleanup_serials: serial_numbers_cleanup(&meta->serials, serial_cleanup); uri_refput(meta->uri); free(meta); @@ -500,167 +482,6 @@ x509stack_store_serial(struct cert_stack *stack, BIGNUM *number) return error; } -/** - * Intended to validate subject uniqueness. - * "Stores" the subject in the current relevant certificate metadata, and - * 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, - 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; - - /* - * "Each distinct subordinate CA and - * EE certified by the issuer MUST be identified using a subject name - * that is unique per issuer. In this context, 'distinct' is defined as - * an entity and a given public key." - * - * What the hell? Removing redundancy: - * - * Each distinct subordinate CA and EE certified by the issuer MUST - * have a unique subject name. In this context, 'distinct' is - * defined as an entity and a given public key. - * - * Trimming fat: - * - * Each distinct child certified by the issuer MUST have a unique - * subject name. In this context, 'distinct' is defined as an - * entity and a given public key. - * - * Translation: - * - * Sibling certificates must have different subject names. - * Among a parent's children, siblings are identified by their - * [entity, public key] tuple. - * - * From RFC 6484, one can surmise that "entity" means "INR holder." - * (INR = Internet Number Resource = IP address or ASN.) - * - * The requirement is kind of circular because an entity would normally - * be identified by its parent certificate and subject name... right? - * How else would an RPKI consumer identify the entity? - * - * Educated flight of fancy: - * - * Sibling certificates must have different subject names. - * Among a parent's children, siblings are identified by their - * [parent, subject name, public key] tuple. - * - * Given that we've established that we're talking about siblings, - * the parent is always the same, so it becomes - * - * Sibling certificates must have different subject names. - * Among a parent's children, siblings are identified by their - * [subject name, public key] tuple. - * - * Circular: - * - * Given two siblings, if their [subject name, public key] is - * different, then their subject names must also be different. - * - * In other words: - * - * - If A = [a, m] and B = [a, m], then no constraints must be imposed. - * - If A = [a, m] and B = [a, n], then a != a. This makes no sense. - * - If A = [a, m] and B = [b, m], then a != b. Self-evident. - * - If A = [a, m] and B = [b, n], then a != b. Self-evident. - * - * So... assuming my train of thought is correct, the requirement can - * therefore be reduced to - * - * If two sibling certificates have different public keys, then - * they must have different subject names as well. - * - * In other words, this whole fucking mess is just a reword of the - * (infinitely more competently articulated) immediately following - * sentence: - * - * "An issuer SHOULD use a different - * subject name if the subject's key pair has changed (i.e., when the CA - * issues a certificate as part of re-keying the subject.)" - * - * Except... the two requirements are actually contradicting each other: - * The former is defined as a MUST and the latter is a SHOULD. - * - * FFFFFFFFFUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUCK!!! - * - * There's also another problem: What should we do when we find two - * siblings with different public keys and equal names? Are we even - * expected to do anything? I mean, does this requirement exist to - * simplify the work of RPs, or to be enforced by RPs? - * - * Sigh. I'm gonna have to ping some people. In the meantime, I'm going - * to comment the caller, because this code is conflicting with the - * rpki_uri refactors. - * - * Leftover from the previous version of this comment: - * - * - Certificates do not share name nor public key. This is the most - * normal situation; accept. - * - Certificates share name, but not public key. Illegal. - * - Certificates share public key, but not name. This is basically - * impossible, but fine nonetheless. Accept. - * (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. - * Accept. (see rfc6484#section-4.7.1 for an example) - */ - - meta = SLIST_FIRST(&stack->metas); - if (meta == NULL) - 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_val_warn("Subject name '%s%s%s' is not unique. (Also found in '%s'.)", - x509_name_commonName(subject), - (serial != NULL) ? "/" : "", - (serial != NULL) ? serial : "", - cursor->file); - return 0; - } - } - - duplicate.name = subject; - x509_name_get(subject); - - error = get_current_file_name(&duplicate.file); - if (error) - goto revert_name; - - error = subjects_add(&meta->subjects, &duplicate); - if (error) - goto revert_file; - - return 0; - -revert_file: - free(duplicate.file); -revert_name: - x509_name_put(subject); - return error; -} - STACK_OF(X509) * certstack_get_x509s(struct cert_stack *stack) { diff --git a/src/object/certificate.c b/src/object/certificate.c index 07c70a2b..db77f40e 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -176,132 +176,10 @@ spki_cmp(X509_PUBKEY *tal_spki, X509_PUBKEY *cert_spki, 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; - - /* what about RRDP? */ - /* - error = uri_create_rsync(&uri, file); - if (error) - return error; - */ - - /* Check if it's '.cer', otherwise treat as a signed object */ - if (uri_is_certificate(uri)) { - 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 = val_crypto_err("Signed object's '%s' 'certificate' element does not decode into a Certificate", - uri_val_get_printable(uri)); - goto free_uri; - } - } - - curr_pk = X509_get_X509_PUBKEY(curr_cert); - if (curr_pk == NULL) { - error = val_crypto_err("X509_get_X509_PUBKEY() returned NULL"); - goto free_cert; - } - - rcvd_pk = X509_get_X509_PUBKEY(rcvd_cert); - if (rcvd_pk == NULL) { - error = val_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; -} - -/* - * "An issuer SHOULD use a different subject name if the subject's - * key pair has changed" + * https://mailarchive.ietf.org/arch/msg/sidrops/mXWbCwh6RO8pAtt7N30Q9m6jUws/ + * Concensus (in mailing list as well as Discord) seems to be "do not check + * subject name uniqueness." */ static int validate_subject(X509 *cert) @@ -314,12 +192,6 @@ validate_subject(X509 *cert) return error; pr_val_debug("Subject: %s", x509_name_commonName(name)); - /* TODO (aaaa) */ - if (false) { - error = x509stack_store_subject(validation_certstack(state_retrieve()), - name, check_dup_public_key, cert); - } - x509_name_put(name); return error; }