From: Alberto Leiva Popper Date: Tue, 5 Mar 2019 01:12:02 +0000 (-0600) Subject: Review on issuer/subject names X-Git-Tag: v0.0.2~77 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=17fb689238e31a31474629a77f61c4c08a3d0fa5;p=thirdparty%2FFORT-validator.git Review on issuer/subject names 1. Was ignoring name.serialNumber on some validations 2. Was not erroring on unknown name attributes 3. If the name is not unique, also print the file where the collision was found 4. Downgrade uniqueness violation to warning. Otherwise some offending certificates are traversed, and others aren't Number 3 also applied to serial numbers. Patched that as well. Also, print the full global URI of each file name on error. I don't like being tied to these awkward long names though; might upload a program argument to tweak this manually tomorrow. --- diff --git a/src/object/certificate.c b/src/object/certificate.c index 50f77ae8..629cc6c4 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -81,6 +81,7 @@ static int validate_issuer(X509 *cert, bool is_ta) { X509_NAME *issuer; + struct rfc5280_name name; int error; issuer = X509_get_issuer_name(cert); @@ -88,9 +89,9 @@ validate_issuer(X509 *cert, bool is_ta) if (!is_ta) return validate_issuer_name("Certificate", issuer); - error = x509_name_decode(issuer, NID_commonName, NULL); - if (error == -ESRCH) - pr_err("The 'issuer' name lacks a commonName attribute."); + error = x509_name_decode(issuer, "issuer", &name); + if (!error) + x509_name_cleanup(&name); return error; } @@ -99,23 +100,21 @@ static int validate_subject(X509 *cert) { struct validation *state; - char *subject; + struct rfc5280_name name; int error; state = state_retrieve(); if (state == NULL) return -EINVAL; - error = x509_name_decode(X509_get_subject_name(cert), NID_commonName, - &subject); - if (error == -ESRCH) - pr_err("Certificate's subject lacks the CommonName atribute."); + error = x509_name_decode(X509_get_subject_name(cert), "subject", &name); if (error) return error; - error = validation_store_subject(state, subject); + error = validation_store_subject(state, &name); + if (error) + x509_name_cleanup(&name); - free(subject); return error; } diff --git a/src/object/name.c b/src/object/name.c index 2dae6893..9c07fe5c 100644 --- a/src/object/name.c +++ b/src/object/name.c @@ -7,66 +7,77 @@ #include "log.h" #include "thread_var.h" -/** - * Only prints error message if the result is not 0 nor -ESRCH. - */ -int -x509_name_decode(X509_NAME *name, int nid, char **_result) +static int +name2string(X509_NAME_ENTRY *name, char **_result) { + const ASN1_STRING *data; char *result; - int len1, len2; - len1 = X509_NAME_get_text_by_NID(name, nid, NULL, 0); - if (len1 < 0) - return -ESRCH; + data = X509_NAME_ENTRY_get_data(name); + if (data == NULL) + return crypto_err("X509_NAME_ENTRY_get_data() returned NULL"); - if (_result == NULL) - return 0; - - result = calloc(len1 + 1, sizeof(char)); + result = malloc(data->length + 1); 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); - } + memcpy(result, data->data, data->length); + result[data->length] = '\0'; *_result = result; return 0; } -struct rfc5280_names { - char *commonName; - char *serialNumber; -}; - -static int -get_names(X509_NAME *name, char const *what, struct rfc5280_names *result) +int +x509_name_decode(X509_NAME *name, char const *what, + struct rfc5280_name *result) { + int i; + X509_NAME_ENTRY *entry; + int nid; int error; - error = x509_name_decode(name, NID_commonName, &result->commonName); - if (error == -ESRCH) { - return pr_err("The '%s' name lacks a commonName attribute.", - what); + result->commonName = NULL; + result->serialNumber = NULL; + + for (i = 0; i < X509_NAME_entry_count(name); i++) { + entry = X509_NAME_get_entry(name, i); + nid = OBJ_obj2nid(X509_NAME_ENTRY_get_object(entry)); + switch (nid) { + case NID_commonName: + error = name2string(entry, &result->commonName); + break; + case NID_serialNumber: + error = name2string(entry, &result->serialNumber); + break; + default: + error = pr_err("The '%s' name has an unknown attribute. (NID: %d)", + what, nid); + break; + } + + if (error) + goto fail; } - 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; + if (result->commonName == NULL) { + error = pr_err("The '%s' name lacks a commonName attribute.", + what); + goto fail; } return 0; + +fail: + x509_name_cleanup(result); + return error; +} + +void +x509_name_cleanup(struct rfc5280_name *name) +{ + free(name->commonName); + free(name->serialNumber); } /** @@ -83,13 +94,20 @@ static bool str_equals(char const *str1, char const *str2) return strcmp(str1, str2) == 0; } +bool +x509_name_equals(struct rfc5280_name *a, struct rfc5280_name *b) +{ + return (strcmp(a->commonName, b->commonName) == 0) + && str_equals(a->serialNumber, b->serialNumber); +} + 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 }; + struct rfc5280_name parent_subject; + struct rfc5280_name child_issuer; int error; /* @@ -108,34 +126,30 @@ validate_issuer_name(char const *container, X509_NAME *issuer) container); } - error = get_names(X509_get_subject_name(parent), "subject", + error = x509_name_decode(X509_get_subject_name(parent), "subject", &parent_subject); if (error) return error; - error = get_names(issuer, "issuer", &child_issuer); + error = x509_name_decode(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; + goto end; + + if (!x509_name_equals(&parent_subject, &child_issuer)) { + error = pr_err("%s's issuer name ('%s%s%s') does not equal issuer certificate's name ('%s%s%s').", + container, + parent_subject.commonName, + (parent_subject.serialNumber != NULL) ? "/" : "", + (parent_subject.serialNumber != NULL) + ? parent_subject.serialNumber + : "", + child_issuer.commonName, + (child_issuer.serialNumber != NULL) ? "/" : "", + (child_issuer.serialNumber != NULL) + ? child_issuer.serialNumber + : ""); } -end1: - free(child_issuer.commonName); - free(child_issuer.serialNumber); -end2: - free(parent_subject.commonName); - free(parent_subject.serialNumber); + x509_name_cleanup(&child_issuer); +end: x509_name_cleanup(&parent_subject); return error; } diff --git a/src/object/name.h b/src/object/name.h index fb0fcf83..c686979b 100644 --- a/src/object/name.h +++ b/src/object/name.h @@ -1,9 +1,24 @@ #ifndef SRC_OBJECT_NAME_H_ #define SRC_OBJECT_NAME_H_ +#include #include -int x509_name_decode(X509_NAME *, int, char **); +/** + * It's an RFC5280 name, but from RFC 6487's perspective. + * Meaning, only commonName and serialNumbers are allowed, and the latter is + * optional. + */ +struct rfc5280_name { + char *commonName; + char *serialNumber; +}; + +int x509_name_decode(X509_NAME *, char const *, struct rfc5280_name *); +void x509_name_cleanup(struct rfc5280_name *); + +bool x509_name_equals(struct rfc5280_name *, struct rfc5280_name *); + int validate_issuer_name(char const *, X509_NAME *); #endif /* SRC_OBJECT_NAME_H_ */ diff --git a/src/object/vcard.c b/src/object/vcard.c index e20a4461..eb32fe9c 100644 --- a/src/object/vcard.c +++ b/src/object/vcard.c @@ -5,10 +5,16 @@ #include "log.h" +/* + * TODO (next iteration) Implement RFC 6350. + * (The current code implements RFC 6493 *only*.) + */ + /** * Reminder: UTF-8 strings are **not** C strings. * They can contain null characters, which do not terminate them. - * DO NOT use standard string operations on them. (Except for the `n` ones). + * DO NOT use standard string operations on them. (Unless you know what you're + * doing). */ struct utf8_string { uint8_t *val; diff --git a/src/state.c b/src/state.c index 179df39a..01cdaff5 100644 --- a/src/state.c +++ b/src/state.c @@ -5,11 +5,22 @@ #include #include "array_list.h" #include "log.h" +#include "str.h" #include "thread_var.h" #include "object/certificate.h" -ARRAY_LIST(serial_numbers, BIGNUM *) -ARRAY_LIST(subjects, char *) +struct serial_number { + BIGNUM *number; + char *file; /* File where this serial number was found. */ +}; + +struct subject_name { + struct rfc5280_name name; + char *file; /* File where this subject name was found. */ +}; + +ARRAY_LIST(serial_numbers, struct serial_number) +ARRAY_LIST(subjects, struct subject_name) /** * Cached certificate data. @@ -154,15 +165,17 @@ abort1: } static void -serial_cleanup(BIGNUM **serial) +serial_cleanup(struct serial_number *serial) { - BN_free(*serial); + BN_free(serial->number); + free(serial->file); } static void -subject_cleanup(char **subject) +subject_cleanup(struct subject_name *subject) { - free(*subject); + x509_name_cleanup(&subject->name); + free(subject->file); } void @@ -334,65 +347,171 @@ validation_peek_resource(struct validation *state) return (cert != NULL) ? cert->resources : NULL; } +static int +get_current_file_name(char **_result) +{ + char const *tmp; + char *result; + + tmp = fnstack_peek(); + if (tmp == NULL) + return pr_crit("The file name stack is empty."); + + result = strdup(tmp); + if (result == NULL) + return pr_enomem(); + + *_result = result; + return 0; +} + +/** + * This function will steal ownership of @number on success. + */ int validation_store_serial_number(struct validation *state, BIGNUM *number) { struct certificate *cert; - BIGNUM **cursor; - BIGNUM *duplicate; + struct serial_number *cursor; + struct serial_number duplicate; + char *string; int error; + /* Remember to free @number if you return 0 but don't store it. */ + cert = SLIST_FIRST(&state->certs); - if (cert == NULL) + if (cert == NULL) { + BN_free(number); return 0; /* The TA lacks siblings, so serial is unique. */ + } - ARRAYLIST_FOREACH(&cert->serials, cursor) - if (BN_cmp(*cursor, number) == 0) - return pr_err("Serial number is not unique."); + /* + * Note: This is is reported as a warning, even though duplicate serial + * numbers are clearly a violation of the RFC and common sense. + * + * But it cannot be simply upgraded into an error because we are + * realizing the problem too late; our traversal is depth-first, so we + * already approved the other bogus certificate and its children. + * (I don't think changing to a breath-first traversal would be a good + * idea; the RAM usage would skyrocket because, since we need the entire + * certificate path to the root to validate any certificate, we would + * end up having the entire tree loaded in memory by the time we're done + * traversing.) + * + * So instead of arbitrarily approving one certificate but not the + * other, we will accept both but report a warning. + * + * Also: It's pretty odd; a significant amount of certificates seem to + * be breaking this rule. Maybe we're the only ones catching it? + */ + ARRAYLIST_FOREACH(&cert->serials, cursor) { + if (BN_cmp(cursor->number, number) == 0) { + BN2string(number, &string); + pr_warn("Serial number '%s' is not unique. (Also found in '%s'.)", + string, cursor->file); + BN_free(number); + free(string); + return 0; + } + } - duplicate = BN_dup(number); - if (duplicate == NULL) - return crypto_err("Could not duplicate a BIGNUM"); + duplicate.number = number; + error = get_current_file_name(&duplicate.file); + if (error) + return error; error = serial_numbers_add(&cert->serials, &duplicate); if (error) - BN_free(duplicate); + free(duplicate.file); return error; } +/** + * This function will steal ownership of @subject on success. + */ int -validation_store_subject(struct validation *state, char *subject) +validation_store_subject(struct validation *state, struct rfc5280_name *subject) { struct certificate *cert; - char **cursor; - char *duplicate; + struct subject_name *cursor; + struct subject_name duplicate; int error; + /* + * There's something that's not clicking with me: + * + * "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." + * + * 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." + * + * "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.)" + * + * It's really weird that it seems to be rewording the same requirement + * except the first version is defined as MUST and the second one is + * defined as SHOULD. + * + * Ugh. Okay. Let's use some common sense. There are four possible + * situations: + * + * - Certificates do not share name nor public key. We should accept + * this. + * - Certificates share name, but not public key. We should reject this. + * - 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, but + * we can't tell for sure right now, and so we should accept it. + * + * 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. + */ + + /* Remember to free @subject if you return 0 but don't store it. */ + cert = SLIST_FIRST(&state->certs); - if (cert == NULL) + if (cert == NULL) { + x509_name_cleanup(subject); return 0; /* The TA lacks siblings, so subject is unique. */ + } + /* See the large comment in validation_store_serial_number(). */ ARRAYLIST_FOREACH(&cert->subjects, cursor) { - if (strcmp(*cursor, subject) == 0) { - /* - * I had to downgrade this because lots of people are - * breaking this rule. - * TODO (next iteration) make a framework so the user - * can choose the severity of each error. - */ - return pr_warn("Subject name '%s' is not unique.", - subject); + if (x509_name_equals(&cursor->name, subject)) { + pr_warn("Subject name '%s%s%s' is not unique. (Also found in '%s'.)", + subject->commonName, + (subject->serialNumber != NULL) ? "/" : "", + (subject->serialNumber != NULL) + ? subject->serialNumber + : "", + cursor->file); + x509_name_cleanup(subject); + return 0; } } - duplicate = strdup(subject); - if (duplicate == NULL) - return pr_err("Could not duplicate a String"); + duplicate.name = *subject; + error = get_current_file_name(&duplicate.file); + if (error) + return error; error = subjects_add(&cert->subjects, &duplicate); if (error) - free(duplicate); + free(duplicate.file); return error; } diff --git a/src/state.h b/src/state.h index 9ff7809b..5b1f45c1 100644 --- a/src/state.h +++ b/src/state.h @@ -3,6 +3,7 @@ #include #include "resource.h" +#include "object/name.h" #include "object/tal.h" struct validation; @@ -33,7 +34,7 @@ struct rpki_uri const *validation_peek_cert_uri(struct validation *); struct resources *validation_peek_resource(struct validation *); int validation_store_serial_number(struct validation *, BIGNUM *); -int validation_store_subject(struct validation *, char *); +int validation_store_subject(struct validation *, struct rfc5280_name *); char *validation_get_ip_buffer1(struct validation *); char *validation_get_ip_buffer2(struct validation *); diff --git a/src/str.c b/src/str.c index d8e52d53..1ae6170c 100644 --- a/src/str.c +++ b/src/str.c @@ -1,5 +1,6 @@ #include "str.h" +#include #include #include "log.h" @@ -30,6 +31,40 @@ ia5s2string(ASN1_IA5STRING *ia5, char **result) : string_clone(ia5->data, ia5->length, result); } +int +BN2string(BIGNUM *bn, char **_result) +{ + BIO *bio; + uint64_t written; + char *result; + + /* Callers can call free() whether this function fails or not. */ + *_result = NULL; + + bio = BIO_new(BIO_s_mem()); + if (bio == NULL) + return -ENOMEM; + + if (BN_print(bio, bn) == 0) { + BIO_free(bio); + return crypto_err("Unable to print the BIGNUM into a BIO"); + } + + written = BIO_number_written(bio); + result = malloc(written + 1); + if (result == NULL) { + BIO_free(bio); + return pr_enomem(); + } + + BIO_read(bio, result, written); + result[written] = '\0'; + + BIO_free(bio); + *_result = result; + return 0; +} + void string_tokenizer_init(struct string_tokenizer *tokenizer, char const *str, size_t str_len, unsigned char separator) diff --git a/src/str.h b/src/str.h index 5e93a1d2..acfd301e 100644 --- a/src/str.h +++ b/src/str.h @@ -8,6 +8,7 @@ int string_clone(void const *, size_t, char **); int ia5s2string(ASN1_IA5STRING *, char **); +int BN2string(BIGNUM *, char **); /* This file is named "str.h" because "string.h" collides with . */ diff --git a/src/thread_var.c b/src/thread_var.c index 955e3253..7b0ab8b5 100644 --- a/src/thread_var.c +++ b/src/thread_var.c @@ -116,8 +116,8 @@ get_file_stack(void) static char const * get_filename(char const *file_path) { - char *slash = strrchr(file_path, '/'); - return (slash != NULL) ? (slash + 1) : file_path; + /* char *slash = strrchr(file_path, '/'); */ + return /* (slash != NULL) ? (slash + 1) : */ file_path; } /**