From daa59b79602cfeff81223a7461e18f513178c9d4 Mon Sep 17 00:00:00 2001

From: Sumit Bose <sbose@redhat.com>

Date: Mon, 18 Sep 2017 15:00:53 +0200

Subject: [PATCH 196/196] sssd_client: add mutex protected call to the PAC

 responder

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

SSSD's plugin for MIT Kerberos to send the PAC to the PAC responder

currently uses sss_pac_make_request() which does not protect the

communication with the PAC responder with a mutex as e.g. the NSS and

PAM clients.

If an application using threads loads this plugin via libkrb5 in

different threads and is heavily processing Kerberos tickets with PACs

chances are that two threads try to communicate with SSSD at once. In

this case one of the threads will miss a reply and will wait for it

until the default client timeout of 300s is passed.

This patch adds a call which uses a mutex to protect the communication

which will avoid the 300s delay mentioned above.

Resolves:

https://pagure.io/SSSD/sssd/issue/3518

Reviewed-by: Lukáš Slebodník <lslebodn@redhat.com>

Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>

(cherry picked from commit 1f331476e7d33bb03cc35a2a9064ee1cc5bed6cf)

---

 Makefile.am                               |  16 ++++

 src/sss_client/common.c                   |  30 +++++++

 src/sss_client/sss_cli.h                  |   7 ++

 src/sss_client/sss_pac_responder_client.c | 137 ++++++++++++++++++++++++++++++

 src/sss_client/sssd_pac.c                 |   4 +-

 src/tests/intg/Makefile.am                |   1 +

 src/tests/intg/test_pac_responder.py      | 120 ++++++++++++++++++++++++++

 7 files changed, 313 insertions(+), 2 deletions(-)

 create mode 100644 src/sss_client/sss_pac_responder_client.c

 create mode 100644 src/tests/intg/test_pac_responder.py

diff --git a/Makefile.am b/Makefile.am

index 907c3256a154ebe2aae5a1667744e1dfbe8abaae..cdd517d50679b876814303fb7d6c63d49bcd8d38 100644

--- a/Makefile.am

+++ b/Makefile.am

@@ -3501,6 +3501,9 @@ endif

 if BUILD_WITH_LIBCURL

 noinst_PROGRAMS += tcurl-test-tool

 endif

+if BUILD_PAC_RESPONDER

+    noinst_PROGRAMS += sssd_pac_test_client

+endif

 if BUILD_AUTOFS

 autofs_test_client_SOURCES = \

@@ -4210,6 +4213,19 @@ sssd_pac_plugin_la_LDFLAGS = \

     -avoid-version \

     -module

+sssd_pac_test_client_SOURCES = \

+    src/sss_client/sss_pac_responder_client.c \

+    src/sss_client/common.c \

+    src/util/strtonum.c \

+    $(NULL)

+sssd_pac_test_client_CFLAGS = \

+    $(AM_CFLAGS) \

+    $(NULL)

+sssd_pac_test_client_LDADD = \

+    $(CLIENT_LIBS) \

+    -lpthread \

+    $(NULL)

+

 # python[23] bindings

 pysss_la_SOURCES = \

     $(SSSD_TOOLS_OBJ) \

diff --git a/src/sss_client/common.c b/src/sss_client/common.c

index b7a5ed760ca379acdfd8f1d2bf95cee1aa271fd8..b527c046e2e3369934b4f9ea7efc1b52eb8c57ea 100644

--- a/src/sss_client/common.c

+++ b/src/sss_client/common.c

@@ -821,6 +821,22 @@ int sss_pac_make_request(enum sss_cli_command cmd,

     }

 }

+int sss_pac_make_request_with_lock(enum sss_cli_command cmd,

+                                   struct sss_cli_req_data *rd,

+                                   uint8_t **repbuf, size_t *replen,

+                                   int *errnop)

+{

+    int ret;

+

+    sss_pac_lock();

+

+    ret = sss_pac_make_request(cmd, rd, repbuf, replen, errnop);

+

+    sss_pac_unlock();

+

+    return ret;

+}

+

 errno_t check_server_cred(int sockfd)

 {

 #ifdef HAVE_UCRED

@@ -1079,6 +1095,8 @@ static struct sss_mutex sss_pam_mtx = { .mtx  = PTHREAD_MUTEX_INITIALIZER };

 static struct sss_mutex sss_nss_mc_mtx = { .mtx  = PTHREAD_MUTEX_INITIALIZER };

+static struct sss_mutex sss_pac_mtx = { .mtx  = PTHREAD_MUTEX_INITIALIZER };

+

 static void sss_mt_lock(struct sss_mutex *m)

 {

     pthread_mutex_lock(&m->mtx);

@@ -1121,6 +1139,16 @@ void sss_nss_mc_unlock(void)

     sss_mt_unlock(&sss_nss_mc_mtx);

 }

+/* PAC mutex wrappers */

+void sss_pac_lock(void)

+{

+    sss_mt_lock(&sss_pac_mtx);

+}

+void sss_pac_unlock(void)

+{

+    sss_mt_unlock(&sss_pac_mtx);

+}

+

 #else

 /* sorry no mutexes available */

@@ -1130,6 +1158,8 @@ void sss_pam_lock(void) { return; }

 void sss_pam_unlock(void) { return; }

 void sss_nss_mc_lock(void) { return; }

 void sss_nss_mc_unlock(void) { return; }

+void sss_pac_lock(void) { return; }

+void sss_pac_unlock(void) { return; }

 #endif

diff --git a/src/sss_client/sss_cli.h b/src/sss_client/sss_cli.h

index d4198407f2f86c6594aee6a2a43775e429692df0..337fe9803d2df3167cd2da77107dbd077f35a51b 100644

--- a/src/sss_client/sss_cli.h

+++ b/src/sss_client/sss_cli.h

@@ -585,6 +585,11 @@ int sss_pac_make_request(enum sss_cli_command cmd,

                          uint8_t **repbuf, size_t *replen,

                          int *errnop);

+int sss_pac_make_request_with_lock(enum sss_cli_command cmd,

+                                   struct sss_cli_req_data *rd,

+                                   uint8_t **repbuf, size_t *replen,

+                                   int *errnop);

+

 int sss_sudo_make_request(enum sss_cli_command cmd,

                           struct sss_cli_req_data *rd,

                           uint8_t **repbuf, size_t *replen,

@@ -634,6 +639,8 @@ void sss_pam_lock(void);

 void sss_pam_unlock(void);

 void sss_nss_mc_lock(void);

 void sss_nss_mc_unlock(void);

+void sss_pac_lock(void);

+void sss_pac_unlock(void);

 errno_t sss_readrep_copy_string(const char *in,

                                 size_t *offset,

diff --git a/src/sss_client/sss_pac_responder_client.c b/src/sss_client/sss_pac_responder_client.c

new file mode 100644

index 0000000000000000000000000000000000000000..9eb0cbea6175ee273b23d9a975529d85c02fc603

--- /dev/null

+++ b/src/sss_client/sss_pac_responder_client.c

@@ -0,0 +1,137 @@

+

+#include <stdio.h>

+#include <stdbool.h>

+#include <pthread.h>

+#include <pwd.h>

+#include <unistd.h>

+#include <sys/types.h>

+#include <errno.h>

+

+#include <unistd.h>

+#include <sys/syscall.h>

+

+#include "sss_client/sss_cli.h"

+

+const uint8_t pac[] = {

+0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x10,

+0x02, 0x00, 0x00, 0x58, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a, 0x00,

+0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x68, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,

+0x00, 0x0c, 0x00, 0x00, 0x00, 0x40, 0x00, 0x00, 0x00, 0x78, 0x02, 0x00, 0x00,

+0x00, 0x00, 0x00, 0x00, 0x06, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0xb8,

+0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x10, 0x00,

+0x00, 0x00, 0xc8, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x10, 0x08,

+0x00, 0xcc, 0xcc, 0xcc, 0xcc, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

+0x00, 0x00, 0x02, 0x00, 0x30, 0xe3, 0xd6, 0x9e, 0x99, 0x2b, 0xd3, 0x01, 0xff,

+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,

+0xff, 0x7f, 0xe2, 0xf7, 0x8a, 0xaf, 0x00, 0x0f, 0xd0, 0x01, 0xe2, 0xb7, 0xf4,

+0xd9, 0xc9, 0x0f, 0xd0, 0x01, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f,

+0x06, 0x00, 0x06, 0x00, 0x04, 0x00, 0x02, 0x00, 0x06, 0x00, 0x06, 0x00, 0x08,

+0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0c, 0x00, 0x02, 0x00, 0x00, 0x00,

+0x00, 0x00, 0x10, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x14, 0x00, 0x02,

+0x00, 0x00, 0x00, 0x00, 0x00, 0x18, 0x00, 0x02, 0x00, 0x45, 0x02, 0x00, 0x00,

+0x50, 0x04, 0x00, 0x00, 0x01, 0x02, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x1c,

+0x00, 0x02, 0x00, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x12, 0x00, 0x14,

+0x00, 0x20, 0x00, 0x02, 0x00, 0x04, 0x00, 0x06, 0x00, 0x24, 0x00, 0x02, 0x00,

+0x28, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,

+0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

+0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x02, 0x00,

+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03,

+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x74, 0x00,

+0x75, 0x00, 0x31, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

+0x00, 0x03, 0x00, 0x00, 0x00, 0x74, 0x00, 0x20, 0x00, 0x75, 0x00, 0x00, 0x00,

+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00,

+0xfd, 0xa2, 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x01, 0x02, 0x00, 0x00, 0x07,

+0x00, 0x00, 0x00, 0x5c, 0x04, 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x56, 0x04,

+0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x89, 0xa6, 0x00, 0x00, 0x07, 0x00, 0x00,

+0x00, 0x0a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x09, 0x00, 0x00, 0x00,

+0x41, 0x00, 0x44, 0x00, 0x2d, 0x00, 0x53, 0x00, 0x45, 0x00, 0x52, 0x00, 0x56,

+0x00, 0x45, 0x00, 0x52, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00,

+0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x41, 0x00, 0x44, 0x00, 0x04, 0x00, 0x00,

+0x00, 0x01, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x15, 0x00, 0x00, 0x00,

+0xf8, 0x12, 0x13, 0xdc, 0x47, 0xf3, 0x1c, 0x76, 0x47, 0x2f, 0x2e, 0xd7, 0x02,

+0x00, 0x00, 0x00, 0x30, 0x00, 0x02, 0x00, 0x07, 0x00, 0x00, 0x00, 0x34, 0x00,

+0x02, 0x00, 0x07, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x01, 0x05, 0x00,

+0x00, 0x00, 0x00, 0x00, 0x05, 0x15, 0x00, 0x00, 0x00, 0x29, 0xc9, 0x4f, 0xd9,

+0xc2, 0x3c, 0xc3, 0x78, 0x36, 0x55, 0x87, 0xf8, 0x54, 0x04, 0x00, 0x00, 0x05,

+0x00, 0x00, 0x00, 0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x15, 0x00,

+0x00, 0x00, 0x25, 0xe1, 0xff, 0x1c, 0xf7, 0x87, 0x6b, 0x2c, 0x25, 0xd2, 0x0c,

+0xe3, 0xf2, 0x03, 0x00, 0x00, 0x00, 0x2c, 0x29, 0x89, 0x65, 0x2d, 0xd3, 0x01,

+0x06, 0x00, 0x74, 0x00, 0x75, 0x00, 0x31, 0x00, 0x20, 0x00, 0x10, 0x00, 0x10,

+0x00, 0x30, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x74, 0x00,

+0x75, 0x00, 0x31, 0x00, 0x74, 0x00, 0x65, 0x00, 0x73, 0x00, 0x74, 0x00, 0x40,

+0x00, 0x61, 0x00, 0x64, 0x00, 0x2e, 0x00, 0x64, 0x00, 0x65, 0x00, 0x76, 0x00,

+0x65, 0x00, 0x6c, 0x00, 0x41, 0x00, 0x44, 0x00, 0x2e, 0x00, 0x44, 0x00, 0x45,

+0x00, 0x56, 0x00, 0x45, 0x00, 0x4c, 0x00, 0x10, 0x00, 0x00, 0x00, 0x76, 0x8e,

+0x25, 0x32, 0x7c, 0x85, 0x00, 0x32, 0xac, 0x8f, 0x02, 0x2c, 0x10, 0x00, 0x00,

+0x00, 0x6b, 0xe8, 0x51, 0x03, 0x30, 0xed, 0xca, 0x7d, 0xe2, 0x12, 0xa5, 0xde};

+

+enum nss_status _nss_sss_getpwuid_r(uid_t uid, struct passwd *result,

+                                    char *buffer, size_t buflen, int *errnop);

+static void *pac_client(void *arg)

+{

+    struct sss_cli_req_data sss_data = { sizeof(pac), pac };

+    int errnop = -1;

+    int ret;

+    size_t c;

+

+    fprintf(stderr, "[%ld][%d][%ld][%s] started\n", time(NULL), getpid(),

+                                                    syscall(SYS_gettid),

+                                                    (char *) arg);

+    for (c = 0; c < 1000; c++) {

+        /* sss_pac_make_request() does not protect the client's file

+         * descriptor to the PAC responder. With this one thread will miss a

+         * reply for a SSS_GET_VERSION request and will wait until

+         * SSS_CLI_SOCKET_TIMEOUT is passed.

+

+        ret = sss_pac_make_request(SSS_PAC_ADD_PAC_USER, &sss_data,

+                                   NULL, NULL, &errnop);

+         */

+        ret = sss_pac_make_request_with_lock(SSS_PAC_ADD_PAC_USER, &sss_data,

+                                             NULL, NULL, &errnop);

+        if (ret != NSS_STATUS_SUCCESS

+                && !(ret == NSS_STATUS_UNAVAIL && errnop != ECONNREFUSED)) {

+                /* NSS_STATUS_UNAVAIL is returned if the PAC responder rejects

+                 * the request which is ok becasue the client is waiting for a

+                 * response here as well. Only errnop == ECONNREFUSED should

+                 * be treated as error becasue this means that the PAC

+                 * responder is not running. */

+            fprintf(stderr, "pac: [%s][%d][%d]\n", (char *)arg, ret, errnop);

+            return ((void *)((uintptr_t)("X")));

+        }

+    }

+

+    fprintf(stderr, "[%ld][%s] done\n", time(NULL),(char *) arg);

+    return NULL;

+}

+

+int main(void)

+{

+    pthread_t thread1;

+    pthread_t thread2;

+    int ret;

+    void *t_ret;

+

+    pthread_create(&thread1, NULL, pac_client,

+                   ((void *)((uintptr_t)("Thread 1"))));

+    pthread_create(&thread2, NULL, pac_client,

+                   ((void *)((uintptr_t)("Thread 2"))));

+

+    ret = pthread_join(thread1, &t_ret);

+    if (ret != 0 || t_ret != NULL) {

+        fprintf(stderr, "Thread 1 failed.\n");

+        return EIO;

+    }

+

+    ret = pthread_join(thread2, &t_ret);

+    if (ret != 0 || t_ret != NULL) {

+        fprintf(stderr, "Thread 1 failed.\n");

+        return EIO;

+    }

+

+    return 0;

+}

diff --git a/src/sss_client/sssd_pac.c b/src/sss_client/sssd_pac.c

index 1d98e38826b36aed199b32880a7e27de905a4592..8444834a7f148787e847f5e8e21186c8701b2de7 100644

--- a/src/sss_client/sssd_pac.c

+++ b/src/sss_client/sssd_pac.c

@@ -169,8 +169,8 @@ static krb5_error_code sssdpac_verify(krb5_context kcontext,

     sss_data.len = sssdctx->data.length;

     sss_data.data = sssdctx->data.data;

-    ret = sss_pac_make_request(SSS_PAC_ADD_PAC_USER, &sss_data,

-                               NULL, NULL, &errnop);

+    ret = sss_pac_make_request_with_lock(SSS_PAC_ADD_PAC_USER, &sss_data,

+                                         NULL, NULL, &errnop);

     if (ret != 0) {

         /* Ignore the error */

     }

diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am

index 8566106e9017a8d3c9e7a3898a3a886e2966e346..0af7c62ca243822d919619f3d0ebc852a317efc4 100644

--- a/src/tests/intg/Makefile.am

+++ b/src/tests/intg/Makefile.am

@@ -29,6 +29,7 @@ dist_noinst_DATA = \

     kdc.py \

     krb5utils.py \

     test_kcm.py \

+    test_pac_responder.py \

     $(NULL)

 config.py: config.py.m4

diff --git a/src/tests/intg/test_pac_responder.py b/src/tests/intg/test_pac_responder.py

new file mode 100644

index 0000000000000000000000000000000000000000..4354a5d78da6a6627a27d0ca85c8a1d47419cedf

--- /dev/null

+++ b/src/tests/intg/test_pac_responder.py

@@ -0,0 +1,120 @@

+#

+# SSSD PAC responder tests

+#

+# Copyright (c) 2017 Red Hat, Inc.

+# Author: Sumit Bose <sbose@redhat.com>

+#

+# This is free software; you can redistribute it and/or modify it

+# under the terms of the GNU General Public License as published by

+# the Free Software Foundation; version 2 only

+#

+# This program is distributed in the hope that it will be useful, but

+# WITHOUT ANY WARRANTY; without even the implied warranty of

+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

+# General Public License for more details.

+#

+# You should have received a copy of the GNU General Public License

+# along with this program.  If not, see <http://www.gnu.org/licenses/>.

+#

+import os

+import stat

+import time

+import config

+import signal

+import subprocess

+import pytest

+from util import unindent

+

+

+def stop_sssd():

+    with open(config.PIDFILE_PATH, "r") as pid_file:

+        pid = int(pid_file.read())

+    os.kill(pid, signal.SIGTERM)

+    while True:

+        try:

+            os.kill(pid, signal.SIGCONT)

+        except:

+            break

+        time.sleep(1)

+

+

+def create_conf_fixture(request, contents):

+    """Generate sssd.conf and add teardown for removing it"""

+    conf = open(config.CONF_PATH, "w")

+    conf.write(contents)

+    conf.close()

+    os.chmod(config.CONF_PATH, stat.S_IRUSR | stat.S_IWUSR)

+    request.addfinalizer(lambda: os.unlink(config.CONF_PATH))

+

+

+def create_sssd_fixture(request):

+    """Start sssd and add teardown for stopping it and removing state"""

+    if subprocess.call(["sssd", "-D", "-f"]) != 0:

+        raise Exception("sssd start failed")

+

+    def teardown():

+        try:

+            stop_sssd()

+        except:

+            pass

+        for path in os.listdir(config.DB_PATH):

+            os.unlink(config.DB_PATH + "/" + path)

+        for path in os.listdir(config.MCACHE_PATH):

+            os.unlink(config.MCACHE_PATH + "/" + path)

+    request.addfinalizer(teardown)

+

+

+@pytest.fixture

+def local_domain_only(request):

+    conf = unindent("""\

+        [sssd]

+        domains = LOCAL

+        services = nss, pac

+

+        [nss]

+        memcache_timeout = 0

+

+        [domain/LOCAL]

+        id_provider = local

+        min_id = 10000

+        max_id = 20000

+    """).format(**locals())

+    create_conf_fixture(request, conf)

+    create_sssd_fixture(request)

+    return None

+

+

+@pytest.fixture

+def sssd_pac_test_client(request):

+    path = os.path.join(config.ABS_BUILDDIR,

+                        "..", "..", "..", "sssd_pac_test_client")

+    if os.access(path, os.X_OK):

+        return path

+

+    return None

+

+

+def timeout_handler(signum, frame):

+    raise Exception("Timeout")

+

+

+def test_multithreaded_pac_client(local_domain_only, sssd_pac_test_client):

+    """

+    Test for ticket

+    https://pagure.io/SSSD/sssd/issue/3518

+    """

+

+    if not sssd_pac_test_client:

+        pytest.skip("The sssd_pac_test_client is not available, skipping test")

+

+    signal.signal(signal.SIGALRM, timeout_handler)

+    signal.alarm(10)

+

+    try:

+        subprocess.check_call(sssd_pac_test_client)

+    except:

+        # cancel alarm

+        signal.alarm(0)

+        raise Exception("sssd_pac_test_client failed")

+

+    signal.alarm(0)

-- 

2.13.5