search

cyrusimap / cyrus-imapd

Cyrus IMAP is an email, contacts and calendar server
http://cyrusimap.org
Other
541 stars 148 forks source link

SIGSEGV in mupdate-client.c/mupdate_disconnect() with SASL DIGEST-MD5 #3343

Open MASHtm opened 3 years ago

MASHtm commented 3 years ago

I'm running cyrus-imapd 2.5.15, but the code is still the same in HEAD. So I guess this bugreport is valid for all versions since 2.5.

I use DIGEST-MD5 on my mupdate server for authentication and after updating from RHEL6 (cyrus-sasl-2.1.23) to 8 (2.1.27) I have sporadic SIGSEGV while XFERing mailboxes between backends (what causes mupdate traffic). It looks like a use-after-free inside cyrus-sasl searching the logging callback function.

The core backtrace shows that cyrus-sasl digest-md5 plugin crashes while syslogging "DIGEST-MD5 client mech dispose" which was added after 2.1.23. The BT also shows that the pointer to the callback function for logging is invalid and causes the SEGV.

(gdb) bt
#0  0x0000000000000051 in ?? ()
#1  0x00007f5264829e4d in _sasl_log (conn=<optimized out>, level=5, fmt=0x7f525d598c48 "DIGEST-MD5 client mech dispose")
    at common.c:1987
#2  0x00007f525d592a66 in digestmd5_client_mech_dispose (utils=0x56532d350400, conn_context=0x56532d34fa00) at digestmd5.c:4695
#3  digestmd5_client_mech_dispose (conn_context=0x56532d34fa00, utils=0x56532d350400) at digestmd5.c:4688
#4  0x00007f52648262d8 in client_dispose (pconn=0x56532d36fae0) at client.c:337
#5  0x00007f5264828fe4 in sasl_dispose (pconn=pconn@entry=0x56532d361ff8) at common.c:849
#6  0x00007f5264fda7e2 in backend_disconnect (s=0x56532d361700) at imap/backend.c:1135
#7  0x00007f5264fdb4c7 in backend_disconnect (s=<optimized out>) at imap/backend.c:1083
#8  0x00007f526500dac2 in mupdate_disconnect (hp=hp@entry=0x7ffd71099958) at imap/mupdate-client.c:150
#9  0x00007f5264ffd9b1 in mboxlist_setacl (namespace=namespace@entry=0x56532bc48740 <imapd_namespace>, 
    name=name@entry=0x7ffd71099d20 "user.xxxxxxxxx", identifier=<optimized out>, identifier@entry=0x56532d2d3b30 "xxxxxxxxx", 
    rights=rights@entry=0x0, isadmin=<optimized out>, userid=0x56532d3529b0 "xxxxxxxxxx", auth_state=0x56532d357d50)
    at imap/mboxlist.c:1954
#10 0x000056532ba2a221 in cmd_setacl (tag=0x56532d296d70 "ACL1", name=0x56532d2e8350 "user.xxxxxxxxx", 
    identifier=0x56532d2d3b30 "xxxxxxxxx", rights=rights@entry=0x0) at imap/imapd.c:7164
#11 0x000056532ba383e2 in cmdloop () at imap/imapd.c:1450
#12 0x000056532ba3d059 in service_main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at imap/imapd.c:984
#13 0x000056532ba1f0c5 in main (argc=<optimized out>, argv=<optimized out>, envp=0x7ffd7109e328) at master/service.c:606

I did a lot of debugging and IMO this is caused by this part of mupdate-client.c/mupdate_connect():

    if (!cbs) {
        local_cbs = 1;
        cbs = mysasl_callbacks(config_getstring(IMAPOPT_MUPDATE_USERNAME),
                               config_getstring(IMAPOPT_MUPDATE_AUTHNAME),
                               config_getstring(IMAPOPT_MUPDATE_REALM),
                               config_getstring(IMAPOPT_MUPDATE_PASSWORD));
    }

    h->conn = backend_connect(NULL, server, &mupdate_protocol,
                              "", cbs, &status, -1);

    /* xxx unclear that this is correct, but it prevents a memory leak */
    if (local_cbs) free_callbacks(cbs);

The "xxx unclear" says it all. IMO the pointer still sits in h->conn->sasl_conn->callbacks as found in the core:

(gdb) p (**hp)->conn->saslconn->callbacks
$7 = (const sasl_callback_t *) 0x56532d3528a0

And then down the BT...

#0  0x0000000000000051 in ?? ()
#1  0x00007f5264829e4d in _sasl_log (conn=<optimized out>, level=5, fmt=0x7f525d598c48 "DIGEST-MD5 client mech dispose")
    at common.c:1987
#2  0x00007f525d592a66 in digestmd5_client_mech_dispose (utils=0x56532d350400, conn_context=0x56532d34fa00) at digestmd5.c:4695
#3  digestmd5_client_mech_dispose (conn_context=0x56532d34fa00, utils=0x56532d350400) at digestmd5.c:4688
#4  0x00007f52648262d8 in client_dispose (pconn=0x56532d36fae0) at client.c:337
#5  0x00007f5264828fe4 in sasl_dispose (pconn=pconn@entry=0x56532d361ff8) at common.c:849
#6  0x00007f5264fda7e2 in backend_disconnect (s=0x56532d361700) at imap/backend.c:1135
#7  0x00007f5264fdb4c7 in backend_disconnect (s=<optimized out>) at imap/backend.c:1083
#8  0x00007f526500dac2 in mupdate_disconnect (hp=hp@entry=0x7ffd71099958) at imap/mupdate-client.c:150

in _sasl_log() the function _sasl_getcallback() gets called which searches the callbacks which are already free()'d and finds "something" before the correct function from the global_callbacks struct is found.

Currently I see two options to fix this: 1) NULL the pointer after free... the "xxx unclear part should at least be:

--- mupdate-client.c    2019-12-16 05:34:28.000000000 +0100
+++ mupdate-client.c.clearptr   2021-02-06 15:19:37.709500304 +0100
@@ -127,7 +127,10 @@
                  "", cbs, &status, -1);

     /* xxx unclear that this is correct, but it prevents a memory leak */
-    if (local_cbs) free_callbacks(cbs);
+    if (local_cbs) {
+        free_callbacks(cbs);
+        if ((h->conn) && (h->conn->saslconn)) h->conn->saslconn->callbacks = NULL;
+    }

     if (!h->conn) {
         syslog(LOG_ERR, "mupdate_connect failed: %s", status ? status : "no auth status");

2) set h->conn->sasl_cb = cbs instead of calling free_callbacks This is used by backend.c/backend_authenticate() as well and freed by backend.c/backend_disconnect()

--- mupdate-client.c    2019-12-16 05:34:28.000000000 +0100
+++ mupdate-client.c.setsaslcb  2021-02-06 15:16:29.745955659 +0100
@@ -126,14 +126,14 @@
     h->conn = backend_connect(NULL, server, &mupdate_protocol,
                  "", cbs, &status, -1);

-    /* xxx unclear that this is correct, but it prevents a memory leak */
-    if (local_cbs) free_callbacks(cbs);
-
     if (!h->conn) {
+        if (local_cbs) free_callbacks(cbs);
         syslog(LOG_ERR, "mupdate_connect failed: %s", status ? status : "no auth status");
    return MUPDATE_NOCONN;
     }

+    if (local_cbs) h->conn->sasl_cb = cbs;
+
     h->saslcompleted = 1;

     /* SUCCESS */
MASHtm commented 3 years ago

looks like I hit the crash part of #3320

elliefm commented 3 years ago

@MASHtm Yeah, looks like it to me too. Does the patch in #3321 fix it for you? I plan to backport it all the way back to 2.5 once it's accepted on master, but in the meantime it'll probably cherry-pick cleanly if you use -X ignore-all-space so that ignores the tabs/spaces conflicts between 2.5 and 3.0+

MASHtm commented 3 years ago

currently I'm running option 2 of my initial report and it works nicely. It's mostly the same but reuses the sasl_cb pointer of backend.c.