[PATCH] Fix pam_conv in pam_wrapper

Andreas Schneider asn at samba.org
Fri Sep 21 09:11:17 UTC 2018


Hello,

I have a patchset for pam_wrapper to fix the pam conversation and add support 
for running with Address Sanitizer.


However, the pam_conv fix is important that I can update pam_wrapper to add 
tests for some pam_winbind features:

https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/master-pwrap


Please review.


Thanks,


	Andreas



-- 
Andreas Schneider                      asn at samba.org
Samba Team                             www.samba.org
GPG-ID:     8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D
-------------- next part --------------
>From a331e42539d54a5cfdb6df41b5ec0d1b60043648 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Mon, 25 Jun 2018 10:39:51 +0200
Subject: [PATCH 1/5] libpamtest: Check that message count matches response
 count

Signed-off-by: Andreas Schneider <asn at samba.org>
Reviewed-by: Jakub Hrozek <jakub.hrozek at posteo.se>
---
 src/libpamtest.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/src/libpamtest.c b/src/libpamtest.c
index 0a26c19..74da180 100644
--- a/src/libpamtest.c
+++ b/src/libpamtest.c
@@ -214,12 +214,11 @@ static int pamtest_simple_conv(int num_msg,
 			       struct pam_response **response,
 			       void *appdata_ptr)
 {
-	int i, ri = 0;
+	int i = 0;
 	int ret;
 	struct pam_response *reply = NULL;
 	const char *prompt;
-	struct pamtest_conv_ctx *cctx = \
-				    (struct pamtest_conv_ctx *) appdata_ptr;
+	struct pamtest_conv_ctx *cctx = (struct pamtest_conv_ctx *)appdata_ptr;
 
 	if (cctx == NULL) {
 		return PAM_CONV_ERR;
@@ -241,15 +240,12 @@ static int pamtest_simple_conv(int num_msg,
 
 			if (reply != NULL) {
 				if (prompt != NULL) {
-					ret = add_to_reply(&reply[ri], prompt);
+					ret = add_to_reply(&reply[i], prompt);
 					if (ret != PAM_SUCCESS) {
 						free_reply(reply, num_msg);
 						return ret;
 					}
-				} else {
-					reply[ri].resp = NULL;
 				}
-				ri++;
 			}
 
 			cctx->echo_off_idx++;
@@ -264,13 +260,12 @@ static int pamtest_simple_conv(int num_msg,
 
 			if (reply != NULL) {
 				if (prompt != NULL) {
-					ret = add_to_reply(&reply[ri], prompt);
+					ret = add_to_reply(&reply[i], prompt);
 					if (ret != PAM_SUCCESS) {
 						free_reply(reply, num_msg);
 						return ret;
 					}
 				}
-				ri++;
 			}
 
 			cctx->echo_on_idx++;
@@ -298,7 +293,7 @@ static int pamtest_simple_conv(int num_msg,
 		}
 	}
 
-	if (response && ri > 0) {
+	if (response != NULL) {
 		*response = reply;
 	} else {
 		free(reply);
-- 
2.18.0


>From a3851def86755da09186622380e9be0b74aa6aea Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Tue, 26 Jun 2018 08:46:44 +0200
Subject: [PATCH 2/5] pwrap: Add PAM_TEXT_INFO and PAM_ERROR_MSG to responses

Signed-off-by: Andreas Schneider <asn at samba.org>
Reviewed-by: Jakub Hrozek <jakub.hrozek at posteo.se>
---
 src/libpamtest.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/libpamtest.c b/src/libpamtest.c
index 74da180..4474736 100644
--- a/src/libpamtest.c
+++ b/src/libpamtest.c
@@ -271,6 +271,14 @@ static int pamtest_simple_conv(int num_msg,
 			cctx->echo_on_idx++;
 			break;
 		case PAM_ERROR_MSG:
+			if (reply != NULL) {
+				ret = add_to_reply(&reply[i], msgm[i]->msg);
+				if (ret != PAM_SUCCESS) {
+					free_reply(reply, num_msg);
+					return ret;
+				}
+			}
+
 			if (cctx->data->out_err != NULL) {
 				memcpy(cctx->data->out_err[cctx->err_idx],
 				       msgm[i]->msg,
@@ -280,6 +288,14 @@ static int pamtest_simple_conv(int num_msg,
 			}
 			break;
 		case PAM_TEXT_INFO:
+			if (reply != NULL) {
+				ret = add_to_reply(&reply[i], msgm[i]->msg);
+				if (ret != PAM_SUCCESS) {
+					free_reply(reply, num_msg);
+					return ret;
+				}
+			}
+
 			if (cctx->data->out_info != NULL) {
 				memcpy(cctx->data->out_info[cctx->info_idx],
 				       msgm[i]->msg,
-- 
2.18.0


>From 66d70c8249e0050dcf594d6bf1b8d79c6e5eab11 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Mon, 25 Jun 2018 11:02:45 +0200
Subject: [PATCH 3/5] pypamtest: Fix number of responses

The number of responses needs to match the number of provided messages.

Signed-off-by: Andreas Schneider <asn at samba.org>
Reviewed-by: Jakub Hrozek <jakub.hrozek at posteo.se>
---
 src/python/pypamtest.c | 67 +++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/src/python/pypamtest.c b/src/python/pypamtest.c
index 6bb1e20..905c652 100644
--- a/src/python/pypamtest.c
+++ b/src/python/pypamtest.c
@@ -166,11 +166,13 @@ static char **new_conv_list(const size_t list_size)
 	return list;
 }
 
-static const char **sequence_as_string_list(PyObject *seq,
-					    const char *paramname)
+static int sequence_as_string_list(PyObject *seq,
+				   const char *paramname,
+				   const char **str_list[],
+				   size_t *num_str_list)
 {
 	const char *p = paramname ? paramname : "attribute values";
-	const char **ret;
+	const char **result;
 	PyObject *utf_item;
 	int i;
 	Py_ssize_t len;
@@ -179,18 +181,18 @@ static const char **sequence_as_string_list(PyObject *seq,
 	if (!PySequence_Check(seq)) {
 		PyErr_Format(PyExc_TypeError,
 			     "The object must be a sequence\n");
-		return NULL;
+		return -1;
 	}
 
 	len = PySequence_Size(seq);
 	if (len == -1) {
-		return NULL;
+		return -1;
 	}
 
-	ret = PyMem_New(const char *, (len + 1));
-	if (!ret) {
+	result = PyMem_New(const char *, (len + 1));
+	if (result == NULL) {
 		PyErr_NoMemory();
-		return NULL;
+		return -1;
 	}
 
 	for (i = 0; i < len; i++) {
@@ -202,20 +204,24 @@ static const char **sequence_as_string_list(PyObject *seq,
 		utf_item = get_utf8_string(item, p);
 		if (utf_item == NULL) {
 			Py_DECREF(item);
-			return NULL;
+			return -1;
 		}
 
-		ret[i] = py_strdup(PyBytes_AsString(utf_item));
+		result[i] = py_strdup(PyBytes_AsString(utf_item));
 		Py_DECREF(utf_item);
-		if (!ret[i]) {
+		if (result[i] == NULL) {
 			Py_DECREF(item);
-			return NULL;
+			return -1;
 		}
 		Py_DECREF(item);
 	}
 
-	ret[i] = NULL;
-	return ret;
+	result[i] = NULL;
+
+	*str_list = result;
+	*num_str_list = (size_t)len;
+
+	return 0;
 }
 
 static PyObject *string_list_as_tuple(char **str_list)
@@ -225,7 +231,7 @@ static PyObject *string_list_as_tuple(char **str_list)
 	PyObject *tup;
 	PyObject *py_str;
 
-	for (len=0; len < PAM_CONV_MSG_MAX; len++) {
+	for (len=0; str_list[len] != NULL; len++) {
 		if (str_list[len][0] == '\0') {
 			/* unused string, stop counting */
 			break;
@@ -808,31 +814,46 @@ static int fill_conv_data(PyObject *py_echo_off,
 			  PyObject *py_echo_on,
 			  struct pamtest_conv_data *conv_data)
 {
+	size_t conv_count = 0;
+	size_t count = 0;
+	int rc;
+
 	conv_data->in_echo_on = NULL;
 	conv_data->in_echo_off = NULL;
 	conv_data->out_err = NULL;
 	conv_data->out_info = NULL;
 
 	if (py_echo_off != NULL) {
-		conv_data->in_echo_off = sequence_as_string_list(py_echo_off,
-								 "echo_off");
-		if (conv_data->in_echo_off == NULL) {
+		rc = sequence_as_string_list(py_echo_off,
+					     "echo_off",
+					     &conv_data->in_echo_off,
+					     &count);
+		if (rc != 0) {
 			free_conv_data(conv_data);
 			return ENOMEM;
 		}
+		conv_count += count;
 	}
 
 	if (py_echo_on != NULL) {
-		conv_data->in_echo_on = sequence_as_string_list(py_echo_on,
-								"echo_on");
-		if (conv_data->in_echo_on == NULL) {
+		rc = sequence_as_string_list(py_echo_on,
+					     "echo_on",
+					     &conv_data->in_echo_on,
+					     &count);
+		if (rc != 0) {
 			free_conv_data(conv_data);
 			return ENOMEM;
 		}
+		conv_count += count;
+	}
+
+	if (conv_count > PAM_CONV_MSG_MAX) {
+		free_conv_data(conv_data);
+		return ENOMEM;
 	}
 
-	conv_data->out_info = new_conv_list(PAM_CONV_MSG_MAX);
-	conv_data->out_err = new_conv_list(PAM_CONV_MSG_MAX);
+	conv_data->out_info = new_conv_list(conv_count);
+	conv_data->out_err = new_conv_list(conv_count);
 	if (conv_data->out_info == NULL || conv_data->out_err == NULL) {
 		free_conv_data(conv_data);
 		return ENOMEM;
-- 
2.18.0


>From 926c10099d43a5f1f1304e9b9c97ade76fdee2e9 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 20 Sep 2018 10:08:00 +0200
Subject: [PATCH 4/5] pam_set_items: Add logging

Signed-off-by: Andreas Schneider <asn at samba.org>
Reviewed-by: Jakub Hrozek <jakub.hrozek at posteo.se>
---
 src/modules/pam_set_items.c | 100 +++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

diff --git a/src/modules/pam_set_items.c b/src/modules/pam_set_items.c
index 22c2c56..dd09020 100644
--- a/src/modules/pam_set_items.c
+++ b/src/modules/pam_set_items.c
@@ -19,7 +19,9 @@
 
 #include <stdlib.h>
 #include <stdio.h>
+#include <stdarg.h>
 #include <string.h>
+#include <unistd.h>
 
 #ifdef HAVE_SECURITY_PAM_APPL_H
 #include <security/pam_appl.h>
@@ -30,6 +32,89 @@
 
 #include "config.h"
 
+/* GCC have printf type attribute check. */
+#ifdef HAVE_FUNCTION_ATTRIBUTE_FORMAT
+#define PRINTF_ATTRIBUTE(a,b) __attribute__ ((__format__ (__printf__, a, b)))
+#else
+#define PRINTF_ATTRIBUTE(a,b)
+#endif /* HAVE_FUNCTION_ATTRIBUTE_FORMAT */
+
+/*****************
+ * LOGGING
+ *****************/
+
+enum pwrap_dbglvl_e {
+	PWRAP_LOG_ERROR = 0,
+	PWRAP_LOG_WARN,
+	PWRAP_LOG_DEBUG,
+	PWRAP_LOG_TRACE
+};
+
+static void pwrap_log(enum pwrap_dbglvl_e dbglvl,
+		      const char *function,
+		      const char *format, ...) PRINTF_ATTRIBUTE(3, 4);
+# define PWRAP_LOG(dbglvl, ...) pwrap_log((dbglvl), __func__, __VA_ARGS__)
+
+static void pwrap_vlog(enum pwrap_dbglvl_e dbglvl,
+		       const char *function,
+		       const char *format,
+		       va_list args) PRINTF_ATTRIBUTE(3, 0);
+
+static void pwrap_vlog(enum pwrap_dbglvl_e dbglvl,
+		       const char *function,
+		       const char *format,
+		       va_list args)
+{
+	char buffer[1024];
+	const char *d;
+	unsigned int lvl = 0;
+	const char *prefix = "PWRAP";
+
+	d = getenv("PAM_WRAPPER_DEBUGLEVEL");
+	if (d != NULL) {
+		lvl = atoi(d);
+	}
+
+	if (lvl < dbglvl) {
+		return;
+	}
+
+	vsnprintf(buffer, sizeof(buffer), format, args);
+
+	switch (dbglvl) {
+	case PWRAP_LOG_ERROR:
+		prefix = "PWRAP_ERROR";
+		break;
+	case PWRAP_LOG_WARN:
+		prefix = "PWRAP_WARN";
+		break;
+	case PWRAP_LOG_DEBUG:
+		prefix = "PWRAP_DEBUG";
+		break;
+	case PWRAP_LOG_TRACE:
+		prefix = "PWRAP_TRACE";
+		break;
+	}
+
+	fprintf(stderr,
+		"%s(%d) - PAM_SET_ITEM %s: %s\n",
+		prefix,
+		(int)getpid(),
+		function,
+		buffer);
+}
+
+static void pwrap_log(enum pwrap_dbglvl_e dbglvl,
+		      const char *function,
+		      const char *format, ...)
+{
+	va_list va;
+
+	va_start(va, format);
+	pwrap_vlog(dbglvl, function, format, va);
+	va_end(va);
+}
+
 #define ITEM_FILE_KEY	"item_file="
 
 static const char *envs[] = {
@@ -83,6 +168,8 @@ static void pam_setitem_env(pam_handle_t *pamh)
 			continue;
 		}
 
+		PWRAP_LOG(PWRAP_LOG_TRACE, "%s=%s", envs[i], v);
+
 		rv = pam_set_item(pamh, items[i], v);
 		if (rv != PAM_SUCCESS) {
 			continue;
@@ -98,6 +185,8 @@ pam_sm_authenticate(pam_handle_t *pamh, int flags,
 	(void) argc;	/* unused */
 	(void) argv;	/* unused */
 
+	PWRAP_LOG(PWRAP_LOG_TRACE, "AUTHENTICATE");
+
 	pam_setitem_env(pamh);
 	return PAM_SUCCESS;
 }
@@ -110,6 +199,8 @@ pam_sm_setcred(pam_handle_t *pamh, int flags,
 	(void) argc;	/* unused */
 	(void) argv;	/* unused */
 
+	PWRAP_LOG(PWRAP_LOG_TRACE, "SETCRED");
+
 	pam_setitem_env(pamh);
 	return PAM_SUCCESS;
 }
@@ -122,6 +213,8 @@ pam_sm_acct_mgmt(pam_handle_t *pamh, int flags,
 	(void) argc;	/* unused */
 	(void) argv;	/* unused */
 
+	PWRAP_LOG(PWRAP_LOG_TRACE, "ACCT_MGMT");
+
 	pam_setitem_env(pamh);
 	return PAM_SUCCESS;
 }
@@ -134,6 +227,8 @@ pam_sm_open_session(pam_handle_t *pamh, int flags,
 	(void) argc;	/* unused */
 	(void) argv;	/* unused */
 
+	PWRAP_LOG(PWRAP_LOG_TRACE, "OPEN_SESSION");
+
 	pam_setitem_env(pamh);
 	return PAM_SUCCESS;
 }
@@ -146,6 +241,8 @@ pam_sm_close_session(pam_handle_t *pamh, int flags,
 	(void) argc;	/* unused */
 	(void) argv;	/* unused */
 
+	PWRAP_LOG(PWRAP_LOG_TRACE, "CLOSE_SESSION");
+
 	pam_setitem_env(pamh);
 	return PAM_SUCCESS;
 }
@@ -158,7 +255,8 @@ pam_sm_chauthtok(pam_handle_t *pamh, int flags,
 	(void) argc;	/* unused */
 	(void) argv;	/* unused */
 
+	PWRAP_LOG(PWRAP_LOG_TRACE, "CHAUTHTOK");
+
 	pam_setitem_env(pamh);
 	return PAM_SUCCESS;
 }
-
-- 
2.18.0


>From aaeb4549a7b5605faeb5384e9c56db11ba503e99 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Sat, 1 Sep 2018 19:46:36 +0200
Subject: [PATCH 5/5] pwrap: Don't do a deep bind if we run with libasan

Signed-off-by: Andreas Schneider <asn at samba.org>
Reviewed-by: Jakub Hrozek <jakub.hrozek at posteo.se>
---
 src/pam_wrapper.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/pam_wrapper.c b/src/pam_wrapper.c
index 230a45d..48d2c2a 100644
--- a/src/pam_wrapper.c
+++ b/src/pam_wrapper.c
@@ -300,7 +300,15 @@ static void *pwrap_load_lib_handle(enum pwrap_lib lib)
 	void *handle = NULL;
 
 #ifdef RTLD_DEEPBIND
-	flags |= RTLD_DEEPBIND;
+	const char *env = getenv("LD_PRELOAD");
+
+	/* Don't do a deepbind if we run with libasan */
+	if (env != NULL && strlen(env) < PATH_MAX) {
+		const char *p = strstr(env, "libasan.so");
+		if (p == NULL) {
+			flags |= RTLD_DEEPBIND;
+		}
+	}
 #endif
 
 	switch (lib) {
-- 
2.18.0



More information about the samba-technical mailing list