From c793d281ed73b08a13871fdd82a3101daff0dd0b Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Nov 21 2018 16:50:37 +0000 Subject: [PATCH 1/4] Use ldap_str2dn to convert a subject into a DN Previously certmonger was parsing the subject itself using commas which didn't account for escaping. Instead rely on LDAP DN parsing. https://pagure.io/certmonger/issue/90 Signed-off-by: Rob Crittenden --- diff --git a/configure.ac b/configure.ac index 663c5b0..ad4b0ad 100644 --- a/configure.ac +++ b/configure.ac @@ -772,6 +772,8 @@ if ! ${configure_dist_target_only:-false} ; then AC_CHECK_HEADER(ldap.h,,AC_MSG_ERROR(ldap.h not found)) AC_CHECK_FUNC(ldap_initialize,,AC_CHECK_LIB(ldap,ldap_initialize)) AC_CHECK_FUNC(ldap_sasl_interactive_bind_s,,AC_CHECK_LIB(ldap,ldap_sasl_interactive_bind_s)) + AC_CHECK_FUNC(ldap_str2dn,,AC_CHECK_LIB(ldap,ldap_str2dn)) + AC_CHECK_FUNC(ldap_dnfree,,AC_CHECK_LIB(ldap,ldap_dnfree)) LDAP_CFLAGS="$CFLAGS" LDAP_LIBS="$LIBS" CFLAGS="$CFLAGSsave" diff --git a/src/Makefile.am b/src/Makefile.am index c871347..fe3b235 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -150,7 +150,7 @@ pkglibexecdir = $(libexecdir)/$(PACKAGE) getcert_CFLAGS = $(AM_CFLAGS) $(NSS_CFLAGS) $(UUID_CFLAGS) getcert_SOURCES = getcert.c tm.c tm.h getcert_LDADD = libcm.a $(GETCERT_LIBS) $(KRB5_LIBS) $(NSS_LIBS) $(UUID_LIBS) \ - $(POPT_LIBS) $(LTLIBICONV) + $(POPT_LIBS) $(LTLIBICONV) $(LDAP_LIBS) if WITH_IPA bin_PROGRAMS += ipa-getcert ipa_getcert_CFLAGS = $(getcert_CFLAGS) @@ -176,16 +176,16 @@ certmonger_getcert_CFLAGS = $(getcert_CFLAGS) certmonger_SOURCES = main.c env-system.c tm.c tm.h certmonger_LDADD = libcm.a \ $(OPENSSL_LIBS) $(CERTMONGER_LIBS) $(KRB5_LIBS) $(IDN_LIBS) \ - $(GMP_LIBS) $(UUID_LIBS) $(POPT_LIBS) $(LTLIBICONV) + $(GMP_LIBS) $(UUID_LIBS) $(POPT_LIBS) $(LTLIBICONV) $(LDAP_LIBS) certmonger_session_SOURCES = main.c env-session.c tm.c tm.h certmonger_session_LDADD = libcm.a \ $(OPENSSL_LIBS) $(CERTMONGER_LIBS) $(KRB5_LIBS) $(IDN_LIBS) \ - $(GMP_LIBS) $(UUID_LIBS) $(POPT_LIBS) $(LTLIBICONV) + $(GMP_LIBS) $(UUID_LIBS) $(POPT_LIBS) $(LTLIBICONV) $(LDAP_LIBS) noinst_PROGRAMS = tdbusm-check serial-check nl-check submit-x toklist tdbusm_check_SOURCES = tdbusm-check.c tm.c tm.h -tdbusm_check_LDADD = libcm.a $(CERTMONGER_LIBS) $(POPT_LIBS) -serial_check_LDADD = libcm.a $(CERTMONGER_LIBS) $(LTLIBICONV) -nl_check_LDADD = libcm.a $(CERTMONGER_LIBS) +tdbusm_check_LDADD = libcm.a $(CERTMONGER_LIBS) $(POPT_LIBS) $(LDAP_LIBS) +serial_check_LDADD = libcm.a $(CERTMONGER_LIBS) $(LTLIBICONV) $(LDAP_LIBS) +nl_check_LDADD = libcm.a $(CERTMONGER_LIBS) $(LDAP_LIBS) submit_x_CFLAGS = $(AM_CFLAGS) $(NSS_CFLAGS) -DCM_SUBMIT_X_MAIN submit_x_SOURCES = submit-x.c submit-x.h submit-u.c submit-u.h log.c log.h \ tm.c tm.h diff --git a/src/csrgen-o.c b/src/csrgen-o.c index 55b0a59..b3f3775 100644 --- a/src/csrgen-o.c +++ b/src/csrgen-o.c @@ -34,6 +34,8 @@ #include #include +#include + #include #include "certext.h" @@ -92,7 +94,10 @@ cm_csrgen_o_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, NETSCAPE_SPKAC spkac; EVP_PKEY *pkey; BIGNUM *serialbn; - char buf[LINE_MAX], *p, *q, *s, *nickname, *pin, *password, *filename; + char buf[LINE_MAX], *s, *nickname, *pin, *password, *filename; + LDAPDN dn = NULL; + LDAPRDN rdn = NULL; + LDAPAVA *attr = NULL; unsigned char *extensions, *upassword, *bmp, *name, *up, *uq, md[CM_DIGEST_MAX]; char *spkidec, *mcb64, *nows; const char *default_cn = CM_DEFAULT_CERT_SUBJECT_CN, *spkihex = NULL; @@ -193,33 +198,28 @@ cm_csrgen_o_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, if ((subject == NULL) && (entry->cm_template_subject != NULL) && (strlen(entry->cm_template_subject) != 0)) { - /* This isn't really correct, but it will - * probably do for now. */ - p = entry->cm_template_subject; - q = p + strcspn(p, ","); + int ret; subject = X509_NAME_new(); if (subject != NULL) { - while (*p != '\0') { - if ((s = memchr(p, '=', q - p)) != NULL) { - *s = '\0'; - for (i = 0; p[i] != '\0'; i++) { - p[i] = toupper(p[i]); - } - X509_NAME_add_entry_by_txt(subject, - p, astring_type(p, s + 1, q - s - 1), - (unsigned char *) (s + 1), q - s - 1, - -1, 0); - *s = '='; - } else { + ret = ldap_str2dn(entry->cm_template_subject, &dn, LDAP_DN_FORMAT_LDAPV3); + if (ret == LDAP_SUCCESS) { + for (i = 0; dn[i] != NULL; i++) { + rdn = dn[i]; + + attr = rdn[0]; X509_NAME_add_entry_by_txt(subject, - "CN", astring_type("CN", p, q - p), - (unsigned char *) p, q - p, + attr->la_attr.bv_val, astring_type(attr->la_attr.bv_val, attr->la_value.bv_val, attr->la_value.bv_len), + (unsigned char *) attr->la_value.bv_val, attr->la_value.bv_len, -1, 0); } - p = q + strspn(q, ","); - q = p + strcspn(p, ","); + if (dn != NULL) + ldap_dnfree(dn); + } else { + X509_NAME_add_entry_by_txt(subject, + "CN", astring_type("CN", entry->cm_template_subject, -1), + (unsigned char *) entry->cm_template_subject, -1, -1, 0); + } } - } } if (subject == NULL) { subject = X509_NAME_new(); diff --git a/tests/tools/Makefile.am b/tests/tools/Makefile.am index b2c42ce..39fa954 100644 --- a/tests/tools/Makefile.am +++ b/tests/tools/Makefile.am @@ -3,7 +3,8 @@ AM_CFLAGS = $(TALLOC_CFLAGS) $(TEVENT_CFLAGS) $(DBUS_CFLAGS) $(KRB5_CFLAGS) \ $(POPT_CFLAGS) -I$(top_builddir)/src -I$(top_srcdir)/src LDADD = libtools.a $(top_builddir)/src/libcm.a $(top_srcdir)/src/env-system.c \ libtools.a $(OPENSSL_LIBS) $(CERTMONGER_LIBS) $(KRB5_LIBS) $(IDN_LIBS) \ - $(GMP_LIBS) $(UUID_LIBS) $(RESOLV_LIBS) $(POPT_LIBS) $(LTLIBICONV) + $(GMP_LIBS) $(UUID_LIBS) $(RESOLV_LIBS) $(POPT_LIBS) $(LTLIBICONV) \ + $(LDAP_LIBS) noinst_SCRIPTS = cachain.sh From 47a60740b6aab81a221351094652d0ed3c09c568 Mon Sep 17 00:00:00 2001 From: Fraser Tweedale Date: Nov 21 2018 16:53:57 +0000 Subject: [PATCH 2/4] csrgen-o: handle multi-value RDNs and log dropped AVAs The existing procedure only took the first AVA from each RDN. Update the X509_NAME construction procedure to preserve multi-valued RDNs. X509_NAME_add_entry_by_txt() requires attribute short names ("CN", "O", etc) to be in upper case, otherwise it fails to add the attribute. Explicitly convert the user input to an Object ID (ASN1_OBJECT) and if it fails, upper case the string and retry. Log when an AVA cannot be added to the X509_NAME (typically because the attribute type is not recognised). --- diff --git a/src/csrgen-o.c b/src/csrgen-o.c index b3f3775..86b2580 100644 --- a/src/csrgen-o.c +++ b/src/csrgen-o.c @@ -32,6 +32,7 @@ #include #include +#include #include #include @@ -51,6 +52,7 @@ #include "subproc.h" #include "util-m.h" #include "util-o.h" +#include "util.h" struct cm_csrgen_state { struct cm_csrgen_state_pvt pvt; @@ -205,12 +207,53 @@ cm_csrgen_o_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, if (ret == LDAP_SUCCESS) { for (i = 0; dn[i] != NULL; i++) { rdn = dn[i]; + int set = 0; // add next AVA in new RDN + for (int j = 0; rdn[j] != NULL; j++) { + attr = rdn[j]; - attr = rdn[0]; - X509_NAME_add_entry_by_txt(subject, - attr->la_attr.bv_val, astring_type(attr->la_attr.bv_val, attr->la_value.bv_val, attr->la_value.bv_len), - (unsigned char *) attr->la_value.bv_val, attr->la_value.bv_len, - -1, 0); + // process attribute type + ASN1_OBJECT *obj = OBJ_txt2obj( + attr->la_attr.bv_val, + 0 /* allow dotted OIDs */); + if (obj == NULL) { + // OpenSSL requires upper-cased short names + // i.e. "CN", "O", etc. + // Convert to upper and try again. + char *attr_upper = str_to_upper(attr->la_attr.bv_val); + if (attr_upper != NULL) { + obj = OBJ_txt2obj(attr_upper, 0); + free(attr_upper); + } + } + + if (obj == NULL) { + cm_log( + 0, + "Unrecognised attribute type: (%s). Continuing.\n", + attr->la_attr.bv_val); + } else { + ret = X509_NAME_add_entry_by_OBJ( + subject, + obj, + astring_type( + attr->la_attr.bv_val, + attr->la_value.bv_val, + attr->la_value.bv_len), + (unsigned char *) attr->la_value.bv_val, + attr->la_value.bv_len, + -1, // append to RDN + set); + if (ret == 1) { + set = -1; // add next AVA to previous RDN + } else { + cm_log( + 0, + "Failed to add AVA to CSR: (%s=%s). Continuing.\n", + attr->la_attr.bv_val, + attr->la_value.bv_val); + } + } + } } if (dn != NULL) ldap_dnfree(dn); diff --git a/src/util.c b/src/util.c index 373bb53..28e2d0d 100644 --- a/src/util.c +++ b/src/util.c @@ -176,3 +176,17 @@ get_config_entry(char * in_data, const char *section, const char *key) free(tmp); return NULL; } + +void str_to_upper_inplace(char *s) { + if (NULL == s) return; + for (; *s != '\0'; s++) { + *s = toupper(*s); + } +} + +char *str_to_upper(const char *s) { + char *ret = strdup(s); + if (ret != NULL) + str_to_upper_inplace(ret); + return ret; +} diff --git a/src/util.h b/src/util.h index c1bcd0a..68fcde8 100644 --- a/src/util.h +++ b/src/util.h @@ -21,4 +21,17 @@ char *read_config_file(const char *filename); char *get_config_entry(char *data, const char *section, const char *key); +/* + * Convert string to upper case in place. + * String must be null-terminated. Locale-unaware. + */ +void str_to_upper_inplace(char *s); + +/* + * Return upper-cased copy of string. + * String must be null-terminated. Locale-unaware. + * Return NULL on error (insufficient memory). + */ +char *str_to_upper(const char *s); + #endif From bf91c2841f81469224400d0435a5b9b24046f988 Mon Sep 17 00:00:00 2001 From: Fraser Tweedale Date: Nov 21 2018 16:54:30 +0000 Subject: [PATCH 3/4] csrgen-o: extract X509_NAME creation subroutines The codes that turns a string into an X509_NAME are big, complex and way too deeply indented. Extract these to two separate subroutines. `ldap_dn_to_X509_NAME` tries to turn a string DN into an X509_NAME. `cn_to_X509_NAME` take a whole string and uses it as the CN in a single-AVA X509_NAME. --- diff --git a/src/csrgen-o.c b/src/csrgen-o.c index 86b2580..402284f 100644 --- a/src/csrgen-o.c +++ b/src/csrgen-o.c @@ -80,6 +80,89 @@ astring_type(const char *attr, const char *p, ssize_t n) return V_ASN1_PRINTABLESTRING; } +static X509_NAME * +ldap_dn_to_X509_NAME(char *s) { + LDAPDN dn = NULL; + LDAPRDN rdn = NULL; + LDAPAVA *attr = NULL; + int ret = ldap_str2dn(s, &dn, LDAP_DN_FORMAT_LDAPV3); + if (ret != LDAP_SUCCESS) + return NULL; + + X509_NAME *x509name = X509_NAME_new(); + if (x509name == NULL) + return NULL; + + for (int i = 0; dn[i] != NULL; i++) { + rdn = dn[i]; + int set = 0; // add next AVA in new RDN + for (int j = 0; rdn[j] != NULL; j++) { + attr = rdn[j]; + + // process attribute type + ASN1_OBJECT *obj = OBJ_txt2obj( + attr->la_attr.bv_val, + 0 /* allow dotted OIDs */); + if (obj == NULL) { + // OpenSSL requires upper-cased short names + // i.e. "CN", "O", etc. + // Convert to upper and try again. + char *attr_upper = str_to_upper(attr->la_attr.bv_val); + if (attr_upper != NULL) { + obj = OBJ_txt2obj(attr_upper, 0); + free(attr_upper); + } + } + + if (obj == NULL) { + cm_log( + 0, + "Unrecognised attribute type: (%s). Continuing.\n", + attr->la_attr.bv_val); + } else { + ret = X509_NAME_add_entry_by_OBJ( + x509name, + obj, + astring_type( + attr->la_attr.bv_val, + attr->la_value.bv_val, + attr->la_value.bv_len), + (unsigned char *) attr->la_value.bv_val, + attr->la_value.bv_len, + -1, // append to RDN + set); + if (ret == 1) { + set = -1; // add next AVA to previous RDN + } else { + cm_log( + 0, + "Failed to add AVA to CSR: (%s=%s). Continuing.\n", + attr->la_attr.bv_val, + attr->la_value.bv_val); + } + } + } + } + ldap_dnfree(dn); + return x509name; +} + +/* Create a single-AVA X509_NAME, with given string as CN */ +static X509_NAME * +cn_to_X509_NAME(const char *s) { + X509_NAME *n = X509_NAME_new(); + if (n != NULL) { + X509_NAME_add_entry_by_txt( + n, + "CN", + astring_type("CN", s, -1), + (unsigned char *) s, + -1 /* compute value length internally */, + -1, 0); + } + return n; +} + static int cm_csrgen_o_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, void *userdata) @@ -97,9 +180,6 @@ cm_csrgen_o_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, EVP_PKEY *pkey; BIGNUM *serialbn; char buf[LINE_MAX], *s, *nickname, *pin, *password, *filename; - LDAPDN dn = NULL; - LDAPRDN rdn = NULL; - LDAPAVA *attr = NULL; unsigned char *extensions, *upassword, *bmp, *name, *up, *uq, md[CM_DIGEST_MAX]; char *spkidec, *mcb64, *nows; const char *default_cn = CM_DEFAULT_CERT_SUBJECT_CN, *spkihex = NULL; @@ -200,78 +280,14 @@ cm_csrgen_o_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, if ((subject == NULL) && (entry->cm_template_subject != NULL) && (strlen(entry->cm_template_subject) != 0)) { - int ret; - subject = X509_NAME_new(); - if (subject != NULL) { - ret = ldap_str2dn(entry->cm_template_subject, &dn, LDAP_DN_FORMAT_LDAPV3); - if (ret == LDAP_SUCCESS) { - for (i = 0; dn[i] != NULL; i++) { - rdn = dn[i]; - int set = 0; // add next AVA in new RDN - for (int j = 0; rdn[j] != NULL; j++) { - attr = rdn[j]; + subject = ldap_dn_to_X509_NAME(entry->cm_template_subject); - // process attribute type - ASN1_OBJECT *obj = OBJ_txt2obj( - attr->la_attr.bv_val, - 0 /* allow dotted OIDs */); - if (obj == NULL) { - // OpenSSL requires upper-cased short names - // i.e. "CN", "O", etc. - // Convert to upper and try again. - char *attr_upper = str_to_upper(attr->la_attr.bv_val); - if (attr_upper != NULL) { - obj = OBJ_txt2obj(attr_upper, 0); - free(attr_upper); - } - } - - if (obj == NULL) { - cm_log( - 0, - "Unrecognised attribute type: (%s). Continuing.\n", - attr->la_attr.bv_val); - } else { - ret = X509_NAME_add_entry_by_OBJ( - subject, - obj, - astring_type( - attr->la_attr.bv_val, - attr->la_value.bv_val, - attr->la_value.bv_len), - (unsigned char *) attr->la_value.bv_val, - attr->la_value.bv_len, - -1, // append to RDN - set); - if (ret == 1) { - set = -1; // add next AVA to previous RDN - } else { - cm_log( - 0, - "Failed to add AVA to CSR: (%s=%s). Continuing.\n", - attr->la_attr.bv_val, - attr->la_value.bv_val); - } - } - } - } - if (dn != NULL) - ldap_dnfree(dn); - } else { - X509_NAME_add_entry_by_txt(subject, - "CN", astring_type("CN", entry->cm_template_subject, -1), - (unsigned char *) entry->cm_template_subject, -1, -1, 0); - } - } + if (subject == NULL) { + subject = cn_to_X509_NAME(entry->cm_template_subject); + } } if (subject == NULL) { - subject = X509_NAME_new(); - if (subject != NULL) { - X509_NAME_add_entry_by_txt(subject, - "CN", astring_type("CN", default_cn, -1), - (const unsigned char *) default_cn, - -1, -1, 0); - } + subject = cn_to_X509_NAME(default_cn); } if (subject != NULL) { util_X509_REQ_set_subject_name(req, subject); From 632fc96cf21381814f843e0c3160011477bef6e7 Mon Sep 17 00:00:00 2001 From: Fraser Tweedale Date: Feb 11 2019 02:33:15 +0000 Subject: [PATCH 4/4] test handling of DNs with escaped chars We were not testing handling of DNs with escaped characters. Update the csrgen test to check this. Part of: https://pagure.io/certmonger/issue/90 --- diff --git a/tests/003-csrgen/expected.out b/tests/003-csrgen/expected.out index 8e6cac6..f17783a 100644 --- a/tests/003-csrgen/expected.out +++ b/tests/003-csrgen/expected.out @@ -26,7 +26,7 @@ The last CSR (the one with everything) was: 13:d=3 hl=2 l= 20 cons: SET 15:d=4 hl=2 l= 18 cons: SEQUENCE 17:d=5 hl=2 l= 3 prim: OBJECT :commonName - 22:d=5 hl=2 l= 11 prim: PRINTABLESTRING :Babs Jensen + 22:d=5 hl=2 l= 11 prim: PRINTABLESTRING :Cloud, Inc. 35:d=2 hl=4 l= 290 cons: SEQUENCE 39:d=3 hl=2 l= 13 cons: SEQUENCE 41:d=4 hl=2 l= 9 prim: OBJECT :rsaEncryption diff --git a/tests/003-csrgen/run.sh b/tests/003-csrgen/run.sh index 7c169ed..e693381 100755 --- a/tests/003-csrgen/run.sh +++ b/tests/003-csrgen/run.sh @@ -299,7 +299,7 @@ done ns_certtype= size=2048 -subject="CN=Babs Jensen" +subject="CN=Cloud\, Inc." hostname=localhost,localhost.localdomain email=root@localhost,root@localhost.localdomain principal=root@EXAMPLE.COM,root@FOO.EXAMPLE.COM