#49611 Ticket 49543 - fix certmap dn comparison
Closed 8 months ago by ftweedal. Opened a year ago by ftweedal.

file modified
+12 -8

@@ -16,6 +16,7 @@ 

  /* What was extcmap.h begins ... */

  

  #include <ldap.h>

+ #include <nss3/cert.h>

  

  #ifndef NSAPI_PUBLIC

  #define NSAPI_PUBLIC

@@ -156,7 +157,7 @@ 

   *  otherwise return LDAPU_CERT_MAP_INITFN_FAILED.  The server startup will be

   *  aborted if the return value is not LDAPU_SUCCESS.

   */

- typedef int (*CertMapInitFn_t)(void *certmap_info, const char *issuerName, const char *issuerDN, const char *libname);

+ typedef int (*CertMapInitFn_t)(void *certmap_info, const char *issuerName, const CERTName *issuerDN, const char *libname);

  

  /*

   * Refer to the description of the function ldapu_get_cert_ava_val

@@ -209,27 +210,30 @@ 

  

  NSAPI_PUBLIC int ldapu_cert_to_ldap_entry(void *cert, LDAP *ld, const char *basedn, LDAPMessage **res);

  

- NSAPI_PUBLIC int ldapu_set_cert_mapfn(const char *issuerDN,

+ NSAPI_PUBLIC int ldapu_set_cert_mapfn(const CERTName *issuerDN,

                                        CertMapFn_t mapfn);

  

  

- NSAPI_PUBLIC CertMapFn_t ldapu_get_cert_mapfn(const char *issuerDN);

+ NSAPI_PUBLIC CertMapFn_t ldapu_get_cert_mapfn(const CERTName *issuerDN);

  

- NSAPI_PUBLIC int ldapu_set_cert_searchfn(const char *issuerDN,

+ NSAPI_PUBLIC int ldapu_set_cert_searchfn(const CERTName *issuerDN,

                                           CertSearchFn_t searchfn);

  

  

- NSAPI_PUBLIC CertSearchFn_t ldapu_get_cert_searchfn(const char *issuerDN);

+ NSAPI_PUBLIC CertSearchFn_t ldapu_get_cert_searchfn(const CERTName *issuerDN);

  

- NSAPI_PUBLIC int ldapu_set_cert_verifyfn(const char *issuerDN,

+ NSAPI_PUBLIC int ldapu_set_cert_verifyfn(const CERTName *issuerDN,

                                           CertVerifyFn_t verifyFn);

  

- NSAPI_PUBLIC CertVerifyFn_t ldapu_get_cert_verifyfn(const char *issuerDN);

+ NSAPI_PUBLIC CertVerifyFn_t ldapu_get_cert_verifyfn(const CERTName *issuerDN);

  

  

  NSAPI_PUBLIC int ldapu_get_cert_subject_dn(void *cert, char **subjectDN);

  

  

+ NSAPI_PUBLIC CERTName *ldapu_get_cert_issuer_dn_as_CERTName(CERTCertificate *cert);

+ 

+ 

  NSAPI_PUBLIC int ldapu_get_cert_issuer_dn(void *cert, char **issuerDN);

  

  

@@ -242,7 +246,7 @@ 

  NSAPI_PUBLIC int ldapu_get_cert_der(void *cert, unsigned char **derCert, unsigned int *len);

  

  

- NSAPI_PUBLIC int ldapu_issuer_certinfo(const char *issuerDN,

+ NSAPI_PUBLIC int ldapu_issuer_certinfo(const CERTName *issuerDN,

                                         void **certmap_info);

  

  

file modified
+1 -1

@@ -48,7 +48,7 @@ 

  typedef struct

  {

      char *issuerName;            /* issuer (symbolic/short) name */

-     char *issuerDN;              /* cert issuer's DN */

+     CERTName *issuerDN;          /* cert issuer's DN */

      LDAPUPropValList_t *propval; /* pointer to the prop-val pairs list */

      CertMapFn_t mapfn;           /* cert to ldapdn & filter mapping func */

      CertVerifyFn_t verifyfn;     /* verify cert function */

file modified
+21 -6

@@ -54,15 +54,30 @@ 

      return *subjectDN ? LDAPU_SUCCESS : LDAPU_ERR_EXTRACT_SUBJECTDN_FAILED;

  }

  

+ /*

+  * Return the Issuer DN as a CERTName.

+  * The CERTName is owned by the CERTCertificate.

+  */

+ NSAPI_PUBLIC CERTName *

+ ldapu_get_cert_issuer_dn_as_CERTName(CERTCertificate *cert_in)

+ {

+     return &cert_in->issuer;

+ }

+ 

+ /*

+  * Return the Issuer DN as a string.

+  * The string should be freed by the caller.

+  */

  NSAPI_PUBLIC int

  ldapu_get_cert_issuer_dn(void *cert_in, char **issuerDN)

  {

-     CERTCertificate *cert = (CERTCertificate *)cert_in;

-     char *cert_issuer = CERT_NameToAscii(&cert->issuer);

- 

-     *issuerDN = strdup(cert_issuer);

-     PR_Free(cert_issuer);

- 

+     *issuerDN = NULL;

+     CERTName *dn = ldapu_get_cert_issuer_dn_as_CERTName((CERTCertificate *)cert_in);

+     if (dn != NULL) {

+         char *cert_issuer = CERT_NameToAscii(dn);

+         *issuerDN = strdup(cert_issuer);

+         PR_Free(cert_issuer);

+     }

      return *issuerDN ? LDAPU_SUCCESS : LDAPU_ERR_EXTRACT_ISSUERDN_FAILED;

  }

  

file modified
+26 -136

@@ -52,7 +52,6 @@ 

  static const char *LIB_DIRECTIVE = "certmap";

  static const int LIB_DIRECTIVE_LEN = 7; /* strlen("LIB_DIRECTIVE") */

  

- static char *ldapu_dn_normalize(char *dn);

  static void *ldapu_propval_free(void *propval_in, void *arg);

  

  typedef struct

@@ -337,8 +336,13 @@ 

      certinfo->issuerName = db_info->dbname;

      db_info->dbname = 0;

  

-     certinfo->issuerDN = ldapu_dn_normalize(db_info->url);

-     db_info->url = 0;

+     /* Parse the Issuer DN. */

+     certinfo->issuerDN = CERT_AsciiToName(db_info->url);

+     if (NULL == certinfo->issuerDN                            /* invalid DN */

+             && ldapu_strcasecmp(db_info->url, "default") != 0 /* not "default" */) {

+         rv = LDAPU_ERR_MALFORMED_SUBJECT_DN;

+         goto error;

+     }

  

      /* hijack actual prop-vals from dbinfo -- to avoid strdup calls */

      if (db_info->firstprop) {

@@ -890,24 +894,26 @@ 

  }

  

  NSAPI_PUBLIC int

- ldapu_issuer_certinfo(const char *issuerDN, void **certmap_info)

+ ldapu_issuer_certinfo(const CERTName *issuerDN, void **certmap_info)

  {

      *certmap_info = 0;

  

-     if (!issuerDN || !*issuerDN || !ldapu_strcasecmp(issuerDN, "default")) {

-         *certmap_info = default_certmap_info;

-     } else if (certmap_listinfo) {

-         char *n_issuerDN = ldapu_dn_normalize(ldapu_strdup(issuerDN));

+     if (certmap_listinfo) {

          LDAPUListNode_t *cur = certmap_listinfo->head;

          while (cur) {

-             if (!ldapu_strcasecmp(n_issuerDN, ((LDAPUCertMapInfo_t *)cur->info)->issuerDN)) {

+             LDAPUCertMapInfo_t *info = (LDAPUCertMapInfo_t *)cur->info;

+ 

+             if (NULL == info->issuerDN) {

+                 /* no DN to compare to (probably the default certmap info) */

+                 continue;

+             }

+ 

+             if (CERT_CompareName(issuerDN, info->issuerDN) == SECEqual) {

                  *certmap_info = cur->info;

                  break;

              }

              cur = cur->next;

          }

-         if (n_issuerDN)

-             ldapu_free(n_issuerDN);

      }

      return *certmap_info ? LDAPU_SUCCESS : LDAPU_FAILED;

  }

@@ -1128,7 +1134,7 @@ 

  }

  

  NSAPI_PUBLIC int

- ldapu_set_cert_mapfn(const char *issuerDN,

+ ldapu_set_cert_mapfn(const CERTName *issuerDN,

                       CertMapFn_t mapfn)

  {

      LDAPUCertMapInfo_t *certmap_info;

@@ -1161,7 +1167,7 @@ 

  }

  

  NSAPI_PUBLIC CertMapFn_t

- ldapu_get_cert_mapfn(const char *issuerDN)

+ ldapu_get_cert_mapfn(const CERTName *issuerDN)

  {

      LDAPUCertMapInfo_t *certmap_info = 0;

  

@@ -1173,7 +1179,7 @@ 

  }

  

  NSAPI_PUBLIC int

- ldapu_set_cert_searchfn(const char *issuerDN,

+ ldapu_set_cert_searchfn(const CERTName *issuerDN,

                          CertSearchFn_t searchfn)

  {

      LDAPUCertMapInfo_t *certmap_info;

@@ -1206,7 +1212,7 @@ 

  }

  

  NSAPI_PUBLIC CertSearchFn_t

- ldapu_get_cert_searchfn(const char *issuerDN)

+ ldapu_get_cert_searchfn(const CERTName *issuerDN)

  {

      LDAPUCertMapInfo_t *certmap_info = 0;

  

@@ -1218,7 +1224,7 @@ 

  }

  

  NSAPI_PUBLIC int

- ldapu_set_cert_verifyfn(const char *issuerDN,

+ ldapu_set_cert_verifyfn(const CERTName *issuerDN,

                          CertVerifyFn_t verifyfn)

  {

      LDAPUCertMapInfo_t *certmap_info;

@@ -1251,7 +1257,7 @@ 

  }

  

  NSAPI_PUBLIC CertVerifyFn_t

- ldapu_get_cert_verifyfn(const char *issuerDN)

+ ldapu_get_cert_verifyfn(const CERTName *issuerDN)

  {

      LDAPUCertMapInfo_t *certmap_info = 0;

  

@@ -1288,7 +1294,6 @@ 

  NSAPI_PUBLIC int

  ldapu_cert_to_ldap_entry(void *cert, LDAP *ld, const char *basedn, LDAPMessage **res)

  {

-     char *issuerDN = 0;

      char *ldapDN = 0;

      char *filter = 0;

      LDAPUCertMapInfo_t *certmap_info;

@@ -1308,14 +1313,14 @@ 

          certmap_attrs[3] = 0;

      }

  

-     rv = ldapu_get_cert_issuer_dn(cert, &issuerDN);

+     CERTName *issuerDN = ldapu_get_cert_issuer_dn_as_CERTName(cert);

+     /*        ^ don't need to free this; it will be freed with ^ the cert */

  

-     if (rv != LDAPU_SUCCESS)

+     if (NULL == issuerDN)

          return LDAPU_ERR_NO_ISSUERDN_IN_CERT;

  

      /* don't free the certmap_info -- its a pointer to an internal structure */

      rv = ldapu_issuer_certinfo(issuerDN, (void **)&certmap_info);

-     free(issuerDN);

  

      if (!certmap_info)

          certmap_info = default_certmap_info;

@@ -1604,118 +1609,3 @@ 

  {

      return realloc(ptr, size);

  }

- 

- #define DNSEPARATOR(c) (c == ',' || c == ';')

- #define SEPARATOR(c) (c == ',' || c == ';' || c == '+')

- #define SPACE(c) (c == ' ' || c == '\n')

- #define NEEDSESCAPE(c) (c == '\\' || c == '"')

- #define B4TYPE 0

- #define INTYPE 1

- #define B4EQUAL 2

- #define B4VALUE 3

- #define INVALUE 4

- #define INQUOTEDVALUE 5

- #define B4SEPARATOR 6

- 

- static char *

- ldapu_dn_normalize(char *dn)

- {

-     char *d, *s;

-     int state, gotesc;

- 

-     gotesc = 0;

-     state = B4TYPE;

-     for (d = s = dn; *s; s++) {

-         switch (state) {

-         case B4TYPE:

-             if (!SPACE(*s)) {

-                 state = INTYPE;

-                 *d++ = *s;

-             }

-             break;

-         case INTYPE:

-             if (*s == '=') {

-                 state = B4VALUE;

-                 *d++ = *s;

-             } else if (SPACE(*s)) {

-                 state = B4EQUAL;

-             } else {

-                 *d++ = *s;

-             }

-             break;

-         case B4EQUAL:

-             if (*s == '=') {

-                 state = B4VALUE;

-                 *d++ = *s;

-             } else if (!SPACE(*s)) {

-                 /* not a valid dn - but what can we do here? */

-                 *d++ = *s;

-             }

-             break;

-         case B4VALUE:

-             if (*s == '"') {

-                 state = INQUOTEDVALUE;

-                 *d++ = *s;

-             } else if (!SPACE(*s)) {

-                 state = INVALUE;

-                 *d++ = *s;

-             }

-             break;

-         case INVALUE:

-             if (!gotesc && SEPARATOR(*s)) {

-                 while (SPACE(*(d - 1)))

-                     d--;

-                 state = B4TYPE;

-                 if (*s == '+') {

-                     *d++ = *s;

-                 } else {

-                     *d++ = ',';

-                 }

-             } else if (gotesc && !NEEDSESCAPE(*s) &&

-                        !SEPARATOR(*s)) {

-                 *--d = *s;

-                 d++;

-             } else {

-                 *d++ = *s;

-             }

-             break;

-         case INQUOTEDVALUE:

-             if (!gotesc && *s == '"') {

-                 state = B4SEPARATOR;

-                 *d++ = *s;

-             } else if (gotesc && !NEEDSESCAPE(*s)) {

-                 *--d = *s;

-                 d++;

-             } else {

-                 *d++ = *s;

-             }

-             break;

-         case B4SEPARATOR:

-             if (SEPARATOR(*s)) {

-                 state = B4TYPE;

-                 if (*s == '+') {

-                     *d++ = *s;

-                 } else {

-                     *d++ = ',';

-                 }

-             }

-             break;

-         default:

-             break;

-         }

-         if (*s == '\\') {

-             gotesc = 1;

-         } else {

-             gotesc = 0;

-         }

-     }

-     *d = '\0';

- 

-     /* Trim trailing spaces */

-     d--;

-     while (d >= dn && *d == ' ') {

-         *d-- = '\0';

-     }

- 

-     return (dn);

- }

file modified
+2 -1

@@ -15,12 +15,13 @@ 

  #include <stdio.h>

  #include <string.h>

  #include <ctype.h>

+ #include <nss3/cert.h>

  #include "certmap.h" /* Public Certmap API */

  #include "plugin.h"  /* must define extern "C" functions */

  

  

  NSAPI_PUBLIC int

- plugin_init_fn(void *certmap_info, const char *issuerName, const char *issuerDN, const char *libname)

+ plugin_init_fn(void *certmap_info, const char *issuerName, const CERTName *issuerDN, const char *libname)

  {

      static int initialized = 0;

      int rv;

Bug Description: Differences in DN string representations between
the value included in certmap.conf, and the stringified value of the
Issuer DN produced by NSS, as well as buggy DN normalisation code in
389 itself, cause 389 to wrongly reject the correct certmap
configuration to use. Authentication fails. This behaviour was
observed when there is an escaped comma in an attribute value.

Fix Description: Instead of comparing stringified DNs, parse the DN
represented in certmap.conf into an NSS CertNAME. Use the NSS DN
comparison routine when comparing certificate Issuer DNs against the
certmap configurations. Remove the buggy DN normalisation routine.

https://pagure.io/389-ds-base/issue/49543

Author: Fraser Tweedale ftweedal@redhat.com

Review by: ???

rebased onto 00a2a1655c4485a8a9da0a63b21f78be957d634d

a year ago

I think it should be in master branch, not in 389-ds-base-1.3.7

@spichugi No it's 1.3.7. IT's for the 1.3.x series, but we can apply to master too. Really, https://pagure.io/389-ds-base/pull-request/49579 would obsolete it (and probably needs it's own fix too).

Looks good, can you rebase this patch again? Then I will merge it...

rebased onto 37873dbca7723791d935d41354f54c3636ba1bbb

10 months ago

@mreynolds thanks for reviewing! I've rebased this PR, and the patch will apply without changes on 1.3.8 and master branches too.

@ftweedal At some point we'll need to get together and rework this for the new certmap plugin that's still being reviewed (waiting on some other things so I can finish pushing that over the line ... )

@firstyear sure thing mate. It should be pretty straightforward, just got to make sure DNs are compared as DNs, not strings.

@mreynolds anything else you need from me to get this merged?

rebased onto 8c20519

9 months ago

@ftweedal this was already merged upstream right? So this can be closed/cancelled?

Yep, it's merged (on three branches):

commit 818807d551e211298e205313b7c29f8318403081 (389-ds-base-1.3.8)
commit 1a93d63fa1fa95599c0dda1a1f3b2a72ab90d634 (389-ds-base-1.3.9)
commit 70bdd335d151e58e227fc2263ece9aedc0803152  (master)

I'll close this.

Pull-Request has been closed by ftweedal

8 months ago