AutoFS development
 help / color / mirror / Atom feed
From: Ian Kent <raven@themaw.net>
To: autofs mailing list <autofs@vger.kernel.org>
Cc: Gordon Lack <gordon.m.lack@gsk.com>,
	"Lan Yixun (dlan)" <dennis.yxun@gmail.com>,
	Leonardo Chiquitto <leonardo.lists@gmail.com>,
	Dustin Polke <DuPol@gmx.de>
Subject: [PATCH 05/25] autofs-5.0.7 - fix crash due to thread unsafe use of libldap
Date: Mon, 19 Aug 2013 09:12:27 +0800	[thread overview]
Message-ID: <20130819011226.6472.70626.stgit@perseus.fritz.box> (raw)
In-Reply-To: <20130819010909.6472.32512.stgit@perseus.fritz.box>

From: Leonardo Chiquitto <leonardo.lists@gmail.com>

Add locking around LDAP initialization calls

To prevent corruption inside SSL and LDAP libraries, it's necessary to
serialize all calls to functions that initialize LDAP contexts.

How to reproduce the problem:

- Setup an LDAP server with SSL/TLS support enabled
- Configure AutoFS to fetch the maps from LDAP
- Make sure the OpenLDAP client library is configured to use SSL
  connections and "usetls" is set to yes in autofs_ldap_auth.conf.

In one directory handled by AutoFS (an indirect mount point), trigger in
parallel some dozens of invalid mounts (ie, try to access keys that do not
exist in the AutoFS map). Repeat until it crashes.

Here it always crashes in less than 20 minutes, normally inside OpenSSL.
Core dump inspection shows that internal SSL structures are corrupted,
with function pointers pointing to random addresses.

Trying to find similar reports on the web, I found this email from an
OpenLDAP developer (partial quote, emphasis mine) [1]:

"As far as I know, libldap is thread safe in the sense that multiple
threads can use separate LDAP* handles without running into concurrency
issues; *except for library initialization*, all accesses to common data
(i.e. global variables) is read-only."

[1]http://www.openldap.org/cgi-bin/wilma_hiliter/openldap-software/200606/msg00252.html

AutoFS implements no locking around LDAP initialization libraries and
it's quite common to see multiple threads executing ldap_initialize()
or ldap_start_tls_s() at the same time.
---
 CHANGELOG             |    1 +
 modules/lookup_ldap.c |   35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG b/CHANGELOG
index 3228d6b..fe232f4 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -57,6 +57,7 @@
 - fix a couple of compiler warnings.
 - add after sssd dependency to unit file.
 - dont start readmap unless ready.
+- fix crash due to thread unsafe use of libldap.
 
 25/07/2012 autofs-5.0.7
 =======================
diff --git a/modules/lookup_ldap.c b/modules/lookup_ldap.c
index a2bfafd..655e9fa 100644
--- a/modules/lookup_ldap.c
+++ b/modules/lookup_ldap.c
@@ -52,6 +52,12 @@ static struct ldap_schema common_schema[] = {
 };
 static unsigned int common_schema_count = sizeof(common_schema)/sizeof(struct ldap_schema);
 
+/*
+ * Initialization of LDAP and OpenSSL must be always serialized to
+ * avoid corruption of context structures inside these libraries.
+ */
+pthread_mutex_t ldapinit_mutex = PTHREAD_MUTEX_INITIALIZER;
+
 struct ldap_search_params {
 	struct autofs_point *ap;
 	LDAP *ldap;
@@ -136,6 +142,22 @@ int ldap_parse_page_control(LDAP *ldap, LDAPControl **controls,
 }
 #endif /* HAVE_LDAP_PARSE_PAGE_CONTROL */
 
+static void ldapinit_mutex_lock(void)
+{
+	int status = pthread_mutex_lock(&ldapinit_mutex);
+	if (status)
+		fatal(status);
+	return;
+}
+
+static void ldapinit_mutex_unlock(void)
+{
+	int status = pthread_mutex_unlock(&ldapinit_mutex);
+	if (status)
+		fatal(status);
+	return;
+}
+
 static void uris_mutex_lock(struct lookup_context *ctxt)
 {
 	int status = pthread_mutex_lock(&ctxt->uris_mutex);
@@ -196,7 +218,7 @@ int unbind_ldap_connection(unsigned logopt, LDAP *ldap, struct lookup_context *c
 	return rv;
 }
 
-LDAP *init_ldap_connection(unsigned logopt, const char *uri, struct lookup_context *ctxt)
+LDAP *__init_ldap_connection(unsigned logopt, const char *uri, struct lookup_context *ctxt)
 {
 	LDAP *ldap = NULL;
 	struct timeval timeout     = { ctxt->timeout, 0 };
@@ -277,6 +299,17 @@ LDAP *init_ldap_connection(unsigned logopt, const char *uri, struct lookup_conte
 	return ldap;
 }
 
+LDAP *init_ldap_connection(unsigned logopt, const char *uri, struct lookup_context *ctxt)
+{
+	LDAP *ldap;
+
+	ldapinit_mutex_lock();
+	ldap = __init_ldap_connection(logopt, uri, ctxt);
+	ldapinit_mutex_unlock();
+
+	return ldap;
+}
+
 static int get_query_dn(unsigned logopt, LDAP *ldap, struct lookup_context *ctxt, const char *class, const char *key)
 {
 	char buf[MAX_ERR_BUF];


  parent reply	other threads:[~2013-08-19  1:12 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-19  1:11 [PATCH 00/25] Current autofs patch queue Ian Kent
2013-08-19  1:11 ` [PATCH 01/25] autofs-5.0.7 - don't override LDFLAGS in make rules Ian Kent
2013-08-19  1:12 ` [PATCH 02/25] autofs-5.0.7 - fix a couple of compiler warnings Ian Kent
2013-08-19  1:12 ` [PATCH 03/25] autofs-5.0.7 - add after sssd dependency to unit file Ian Kent
2013-08-19  1:12 ` [PATCH 04/25] autofs-5.0.7 - dont start readmap unless ready Ian Kent
2013-08-19  1:12 ` Ian Kent [this message]
2013-08-19  1:12 ` [PATCH 06/25] autofs-5.0.7 - fix compile error with heimdal support enabled Ian Kent
2013-08-20  3:36   ` Dennis Lan (dlan)
2013-08-20  7:34     ` Ian Kent
2013-08-19  1:12 ` [PATCH 07/25] autofs-5.0.7 - fix typo forced-shutdown should be force-shutdown Ian Kent
2013-08-19  1:12 ` [PATCH 08/25] autofs-5.0.7 - fix hesiod check error and use correct $(LIBS) setting Ian Kent
2013-08-19  1:12 ` [PATCH 09/25] autofs-5.0.7 - fix dead LDAP symbolic link when LDAP support is disabled Ian Kent
2013-08-19  1:13 ` [PATCH 10/25] autofs-5.0.7 - add missing libtirpc lib to mount_nfs.so when TIRPC enabled Ian Kent
2013-08-19  1:13 ` [PATCH 11/25] autofs-5.0.7 - use compiler determined by configure instead of hard-coded ones Ian Kent
2013-08-19  1:13 ` [PATCH 12/25] autofs-5.0.7 - remove hard-coded STRIP variable Ian Kent
2013-08-19  1:13 ` [PATCH 13/25] autofs-5.0.7 - use LIBS for link libraries Ian Kent
2013-08-19  1:13 ` [PATCH 14/25] autofs-5.0.7 - unbundle NOTSTRIP from DEBUG so they dont depend on each other Ian Kent
2013-08-19  1:13 ` [PATCH 15/25] autofs-5.0.7 - fix occasional build error when enable parallel compiling Ian Kent
2013-08-19  1:13 ` [PATCH 16/25] autofs-5.0.7 - fix compilation of lookup_ldap.c without sasl Ian Kent
2013-08-19  1:13 ` [PATCH 17/25] autofs-5.0.7 - fix dumpmaps multi output Ian Kent
2013-08-19  1:13 ` [PATCH 18/25] autofs-5.0.7 - try and cleanup after dumpmaps Ian Kent
2013-08-19  1:14 ` [PATCH 19/25] autofs-5.0.7 - teach dumpmaps to output simple key value pairs Ian Kent
2013-08-19  1:14 ` [PATCH 20/25] autofs-5.0.7 - fix syncronize handle_mounts() shutdown Ian Kent
2013-08-19  1:14 ` [PATCH 21/25] autofs-5.0.7 - fix fix wildcard multi map regression Ian Kent
2013-08-19  1:14 ` [PATCH 22/25] autofs-5.0.7 - improve timeout option description Ian Kent
2013-08-19  1:14 ` [PATCH 23/25] autofs-5.0.7 - only probe specific nfs version when requested Ian Kent
2013-08-19  1:14 ` [PATCH 24/25] autofs-5.0.7 - fix bad mkdir permission on create Ian Kent
2013-08-19  2:13   ` Ian Kent
2013-08-19  1:14 ` [PATCH 25/25] autofs-5.0.7 - setup program map env from macro table Ian Kent
2013-08-19  5:30 ` [PATCH 00/25] Current autofs patch queue Dennis Lan (dlan)
2013-08-20  2:55   ` Ian Kent
2013-08-20  4:52     ` Dennis Lan (dlan)
2013-09-02 10:34 ` Martin Wilck
2013-09-02 10:41   ` Gordon Lack
2013-09-02 11:04     ` Martin Wilck
2013-09-02 11:13       ` Gordon Lack
2013-09-02 12:17         ` Martin Wilck
2013-09-02 12:55           ` Gordon Lack
2013-09-02 13:15             ` Martin Wilck
2013-09-02 13:41               ` Gordon Lack
2013-09-02 14:11                 ` Martin Wilck
2013-09-02 14:20                   ` Gordon Lack
2013-09-02 14:49                     ` Martin Wilck
2013-09-02 15:08                       ` Gordon Lack
2013-09-02 15:23                         ` Martin Wilck
2013-09-02 15:36                           ` Gordon Lack
2013-09-06  8:11   ` Ian Kent

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130819011226.6472.70626.stgit@perseus.fritz.box \
    --to=raven@themaw.net \
    --cc=DuPol@gmx.de \
    --cc=autofs@vger.kernel.org \
    --cc=dennis.yxun@gmail.com \
    --cc=gordon.m.lack@gsk.com \
    --cc=leonardo.lists@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox