Fixed handling of channel bindings on the client side

The client side was failing to select a suitable SASL mechanism when
the application specified channel bindings, but didn't make them mandatory
to use. In such a configuration, if a non channel binding capable mechanism
was selected through "client_mech_list" SASL option, sasl_client_start
would fail. For example if the server supports both SCRAM-SHA-1[-PLUS] and
PLAIN and "client_mech_list" was set to "PLAIN", authentication would never
work. This patch fixes the problem.

The patch also cleans up the best SASL mechanism selection code to
prefer better channel bindings over SASL security layer.

Test-information:

Compiled and tested on Windows with msadm expire_mail and imapd.

Signed-off-by: Dave Cridland <dave.cridland@isode.com>

diff --git a/include/saslplug.h b/include/saslplug.h
index bd50367..4878a96 100755 (executable)
--- a/include/saslplug.h
+++ b/include/saslplug.h
@@ -225,8 +225,8 @@ typedef enum  {
 
 typedef enum {
     SASL_CB_DISP_NONE = 0,          /* client did not support CB */
-    SASL_CB_DISP_USED,              /* client supports and used CB */
-    SASL_CB_DISP_WANT               /* client supports CB, thinks server does not */
+    SASL_CB_DISP_WANT,              /* client supports CB, thinks server does not */
+    SASL_CB_DISP_USED               /* client supports and used CB */
 } sasl_cbinding_disp_t;
 
 /* TRUE if channel binding is non-NULL */

diff --git a/lib/client.c b/lib/client.c
index e4ef163..1c5374a 100644 (file)
--- a/lib/client.c
+++ b/lib/client.c
@@ -486,10 +486,11 @@ _sasl_cbinding_disp(sasl_client_params_t *cparams,
 
     if (SASL_CB_PRESENT(cparams)) {
         if (mech_nego) {
-            if (!server_can_cb && SASL_CB_CRITICAL(cparams))
+            if (!server_can_cb && SASL_CB_CRITICAL(cparams)) {
                return SASL_NOMECH;
-            else
+            } else {
                 *cbindingdisp = SASL_CB_DISP_WANT;
+            }
         } else if (SASL_CB_CRITICAL(cparams)) {
             *cbindingdisp = SASL_CB_DISP_USED;
         }
@@ -522,11 +523,13 @@ int sasl_client_start(sasl_conn_t *conn,
 {
     sasl_client_conn_t *c_conn= (sasl_client_conn_t *) conn;
     char *ordered_mechs = NULL, *name;
-    cmechanism_t *m=NULL,*bestm=NULL;
+    cmechanism_t *m = NULL, *bestm = NULL;
     size_t i, list_len;
     sasl_ssf_t bestssf = 0, minssf = 0;
     int result, server_can_cb = 0;
     sasl_cbinding_disp_t cbindingdisp;
+    sasl_cbinding_disp_t cur_cbindingdisp;
+    sasl_cbinding_disp_t best_cbindingdisp = SASL_CB_DISP_NONE;
 
     if(_sasl_client_active==0) return SASL_NOTINIT;
 
@@ -566,18 +569,23 @@ int sasl_client_start(sasl_conn_t *conn,
      * are doing mechanism negotiation and whether server supports
      * channel bindings.
      */
-    result = _sasl_cbinding_disp(c_conn->cparams, (list_len > 1),
-                                 server_can_cb, &cbindingdisp);
+    result = _sasl_cbinding_disp(c_conn->cparams,
+                                 (list_len > 1),
+                                 server_can_cb,
+                                 &cbindingdisp);
     if (result != 0)
        goto done;
 
     for (i = 0, name = ordered_mechs; i < list_len; i++) {
+
        /* foreach in client list */
        for (m = cmechlist->mech_list; m != NULL; m = m->next) {
            int myflags, plus;
 
-           if (!_sasl_is_equal_mech(name, m->m.plug->mech_name, &plus))
+           if (!_sasl_is_equal_mech(name, m->m.plug->mech_name,
+                                     strlen(m->m.plug->mech_name), &plus)) {
                continue;
+            }
 
            /* Do we have the prompts for it? */
            if (!have_prompts(conn, m->m.plug))
@@ -601,7 +609,7 @@ int sasl_client_start(sasl_conn_t *conn,
            }
 
            /* Can we meet it's features? */
-           if (cbindingdisp != SASL_CB_DISP_NONE &&
+           if (cbindingdisp == SASL_CB_DISP_USED &&
                !(m->m.plug->features & SASL_FEAT_CHANNEL_BINDING)) {
                break;
            }
@@ -617,20 +625,6 @@ int sasl_client_start(sasl_conn_t *conn,
                break;
            }
 
-#ifdef PREFER_MECH
-           if (strcasecmp(m->m.plug->mech_name, PREFER_MECH) &&
-               bestm && m->m.plug->max_ssf <= bestssf) {
-               /* this mechanism isn't our favorite, and it's no better
-                  than what we already have! */
-               break;
-           }
-#else
-           if (bestm && m->m.plug->max_ssf <= bestssf) {
-               /* this mechanism is no better than what we already have! */
-               break;
-           }
-#endif
-
            /* compare security flags, only take new mechanism if it has
             * all the security flags of the previous one.
             *
@@ -656,12 +650,34 @@ int sasl_client_start(sasl_conn_t *conn,
            }
 
            if (SASL_CB_PRESENT(c_conn->cparams) && plus) {
-               cbindingdisp = SASL_CB_DISP_USED;
+               cur_cbindingdisp = SASL_CB_DISP_USED;
+           } else {
+               cur_cbindingdisp = cbindingdisp;
+            }
+
+            if (bestm && (best_cbindingdisp > cur_cbindingdisp)) {
+                break;
+            }
+
+#ifdef PREFER_MECH
+           if (strcasecmp(m->m.plug->mech_name, PREFER_MECH) &&
+               bestm && m->m.plug->max_ssf <= bestssf) {
+               /* this mechanism isn't our favorite, and it's no better
+                  than what we already have! */
+               break;
            }
+#else
+           if (bestm && m->m.plug->max_ssf <= bestssf) {
+               /* this mechanism is no better than what we already have! */
+               break;
+           }
+#endif
 
            if (mech) {
                *mech = m->m.plug->mech_name;
            }
+
+            best_cbindingdisp = cur_cbindingdisp;
            bestssf = m->m.plug->max_ssf;
            bestm = m;
            break;
@@ -689,7 +705,7 @@ int sasl_client_start(sasl_conn_t *conn,
 
     c_conn->cparams->external_ssf = conn->external.ssf;
     c_conn->cparams->props = conn->props;
-    c_conn->cparams->cbindingdisp = cbindingdisp;
+    c_conn->cparams->cbindingdisp = best_cbindingdisp;
     c_conn->mech = bestm;
 
     /* init that plugin */

diff --git a/lib/common.c b/lib/common.c
index 2481697..b79161a 100644 (file)
--- a/lib/common.c
+++ b/lib/common.c
@@ -2328,17 +2328,17 @@ int sasl_listmech(sasl_conn_t *conn,
 
 int _sasl_is_equal_mech(const char *req_mech,
                         const char *plug_mech,
+                        size_t req_mech_len,
                         int *plus)
 {
-    size_t len = strlen(req_mech);
     size_t n;
 
-    if (len > 5 &&
-        strcasecmp(&req_mech[len - 5], "-PLUS") == 0) {
-        n = len - 5;
+    if (req_mech_len > 5 &&
+        strcasecmp(&req_mech[req_mech_len - 5], "-PLUS") == 0) {
+        n = req_mech_len - 5;
         *plus = 1;
     } else {
-        n = len;
+        n = req_mech_len;
         *plus = 0;
     }
 

diff --git a/lib/saslint.h b/lib/saslint.h
index 1af2415..11d0e10 100644 (file)
--- a/lib/saslint.h
+++ b/lib/saslint.h
@@ -303,6 +303,7 @@ extern sasl_mutex_utils_t _sasl_mutex_utils;
 
 extern int _sasl_is_equal_mech(const char *req_mech,
                                const char *plug_mech,
+                               size_t req_mech_len,
                                int *plus);
 
 /*

diff --git a/lib/server.c b/lib/server.c
index 4b78c01..c0ee30f 100644 (file)
--- a/lib/server.c
+++ b/lib/server.c
@@ -1025,6 +1025,7 @@ static int mech_permitted(sasl_conn_t *conn,
     if (_sasl_getcallback(conn, SASL_CB_GETOPT, &getopt, &context)
             == SASL_OK) {
        const char *mlist = NULL;
+       int plus = 0;
 
        getopt(context, NULL, "mech_list", &mlist, NULL);
 
@@ -1034,9 +1035,8 @@ static int mech_permitted(sasl_conn_t *conn,
 
            while (*mlist) {
                for (cp = mlist; *cp && !isspace((int) *cp); cp++);
-               if (((size_t) (cp - mlist) == strlen(plug->mech_name)) &&
-                   !strncasecmp(mlist, plug->mech_name,
-                                strlen(plug->mech_name))) {
+               if (_sasl_is_equal_mech(mlist, plug->mech_name, (size_t) (cp - mlist), &plus)) {
+                   /* found a match */
                    break;
                }
                mlist = cp;
@@ -1214,6 +1214,7 @@ int sasl_server_start(sasl_conn_t *conn,
     int result;
     context_list_t *cur, **prev;
     mechanism_t *m;
+    size_t mech_len;
     int plus = 0;
 
     if (_sasl_server_active==0) return SASL_NOTINIT;
@@ -1221,6 +1222,7 @@ int sasl_server_start(sasl_conn_t *conn,
     /* make sure mech is valid mechanism
        if not return appropriate error */
     m=mechlist->mech_list;
+    mech_len = strlen(mech);
 
     /* check parameters */
     if(!conn) return SASL_BADPARAM;
@@ -1232,7 +1234,7 @@ int sasl_server_start(sasl_conn_t *conn,
     if(serveroutlen) *serveroutlen = 0;
 
     while (m != NULL) {
-       if (_sasl_is_equal_mech(mech, m->m.plug->mech_name, &plus))
+       if (_sasl_is_equal_mech(mech, m->m.plug->mech_name, mech_len, &plus))
            break;
 
        m = m->next;