LDAP URI decoding bugs

Started by Thomas Munroabout 8 years ago4 messages

thomas.munro@enterprisedb.com

about 8 years ago

1 attachment(s)

Hi hackers,

1. If you set up a pg_hba.conf with a URL that lacks a base DN or
hostname, hba.c will segfault on startup when it tries to pstrdup a
null pointer. Examples: ldapurl="ldap://localhost" and
ldapurl="ldap://".

2. If we fail to bind but have no binddn configured, we'll pass NULL
to ereport (snprint?) for %s, which segfaults on some libc
implementations. That crash requires more effort to reproduce but you
can see pretty clearly a few lines above in auth.c that it can be
NULL. (I'm surprised Coverity didn't complain about that. Maybe it
can't see this code due to macros.)

Please see attached.

--
Thomas Munro
http://www.enterprisedb.com

Attachments:

ldap-fixes.patchapplication/octet-stream; name=ldap-fixes.patchDownload

From d2a7b1729284dfb6e9b140f1629488583fa15767 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Fri, 3 Nov 2017 23:33:09 +1300
Subject: [PATCH] Fix a couple of null pointer deferences in the ldap auth
 code.

Author: Thomas Munro
---
 src/backend/libpq/auth.c | 3 ++-
 src/backend/libpq/hba.c  | 6 ++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index ab74fd8dfd..8b82791d4c 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2520,7 +2520,8 @@ CheckLDAPAuth(Port *port)
 		{
 			ereport(LOG,
 					(errmsg("could not perform initial LDAP bind for ldapbinddn \"%s\" on server \"%s\": %s",
-							port->hba->ldapbinddn, port->hba->ldapserver,
+							port->hba->ldapbinddn ? port->hba->ldapbinddn : "",
+							port->hba->ldapserver,
 							ldap_err2string(r)),
 					 errdetail_for_ldap(ldap)));
 			ldap_unbind(ldap);
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index b2c487a8e8..e997155cc8 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1739,9 +1739,11 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
 			return false;
 		}
 
-		hbaline->ldapserver = pstrdup(urldata->lud_host);
+		if (urldata->lud_host)
+			hbaline->ldapserver = pstrdup(urldata->lud_host);
 		hbaline->ldapport = urldata->lud_port;
-		hbaline->ldapbasedn = pstrdup(urldata->lud_dn);
+		if (urldata->lud_dn)
+			hbaline->ldapbasedn = pstrdup(urldata->lud_dn);
 
 		if (urldata->lud_attrs)
 			hbaline->ldapsearchattribute = pstrdup(urldata->lud_attrs[0]);	/* only use first one */
-- 
2.14.1

Michael Paquier

michael.paquier@gmail.com

about 8 years ago

In reply to: Thomas Munro (#1)

Re: LDAP URI decoding bugs

On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

1. If you set up a pg_hba.conf with a URL that lacks a base DN or
hostname, hba.c will segfault on startup when it tries to pstrdup a
null pointer. Examples: ldapurl="ldap://localhost" and
ldapurl="ldap://".

2. If we fail to bind but have no binddn configured, we'll pass NULL
to ereport (snprint?) for %s, which segfaults on some libc
implementations. That crash requires more effort to reproduce but you
can see pretty clearly a few lines above in auth.c that it can be
NULL. (I'm surprised Coverity didn't complain about that. Maybe it
can't see this code due to macros.)

Good question. Indeed Coverity did not complain here, perhaps because
the compiled build is not using openldap?

Please see attached.

Oops. So...

-        hbaline->ldapserver = pstrdup(urldata->lud_host);
+        if (urldata->lud_host)
+            hbaline->ldapserver = pstrdup(urldata->lud_host);
This prevents the backend to blow up on ldap://.

-       hbaline->ldapbasedn = pstrdup(urldata->lud_dn);
+       if (urldata->lud_dn)
+           hbaline->ldapbasedn = pstrdup(urldata->lud_dn);
And this prevents the crash on ldap://localhost.

-                            port->hba->ldapbinddn, port->hba->ldapserver,
+                            port->hba->ldapbinddn ? port->hba->ldapbinddn : "",
+                            port->hba->ldapserver,
ldapserver should never be NULL thanks to the check on
MANDATORY_AUTH_ARG in parse_hba_line(), still I would tend to be
maniak and do the same check as for ldapbinddn. That feels safer
thinking long-term.

Please note that I have added as well an entry in the next CF to avoid
that bug falling into oblivion:
https://commitfest.postgresql.org/16/1372/
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

about 8 years ago

In reply to: Michael Paquier (#2)

Re: LDAP URI decoding bugs

On 11/6/17 23:30, Michael Paquier wrote:

On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

1. If you set up a pg_hba.conf with a URL that lacks a base DN or
hostname, hba.c will segfault on startup when it tries to pstrdup a
null pointer. Examples: ldapurl="ldap://localhost" and
ldapurl="ldap://".

2. If we fail to bind but have no binddn configured, we'll pass NULL
to ereport (snprint?) for %s, which segfaults on some libc
implementations. That crash requires more effort to reproduce but you
can see pretty clearly a few lines above in auth.c that it can be
NULL. (I'm surprised Coverity didn't complain about that. Maybe it
can't see this code due to macros.)

committed and backpatched

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Thomas Munro

thomas.munro@enterprisedb.com

about 8 years ago

In reply to: Peter Eisentraut (#3)

Re: LDAP URI decoding bugs

On Sat, Nov 11, 2017 at 8:37 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 11/6/17 23:30, Michael Paquier wrote:

On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

1. If you set up a pg_hba.conf with a URL that lacks a base DN or
hostname, hba.c will segfault on startup when it tries to pstrdup a
null pointer. Examples: ldapurl="ldap://localhost" and
ldapurl="ldap://".

2. If we fail to bind but have no binddn configured, we'll pass NULL
to ereport (snprint?) for %s, which segfaults on some libc
implementations. That crash requires more effort to reproduce but you
can see pretty clearly a few lines above in auth.c that it can be
NULL. (I'm surprised Coverity didn't complain about that. Maybe it
can't see this code due to macros.)

committed and backpatched

Thanks!

I suppose someone might eventually want to go further and teach it to
understand such bare URLs or missing options (ie leaving out any bits
you want and falling back to the ldap library's defaults, which come
from places like env variables, .ldaprc and /etc/ldap.conf, the way
that "ldapsearch" and other tools manage to work with reasonable
defaults, or at least only need to be set up in one place for all your
LDAP-client software). I'm not planning to work on that.

--
Thomas Munro
http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers