Re: [HACKERS] LDAP URI decoding bugs

[Thomas Munro]

On Sat, Nov 11, 2017 at 8:37 AM, Peter Eisentraut
 wrote:
> On 11/6/17 23:30, Michael Paquier wrote:
>> On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
>>  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

Re: [HACKERS] LDAP URI decoding bugs

[Peter Eisentraut]

On 11/6/17 23:30, Michael Paquier wrote:
> On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
>  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

Re: [HACKERS] LDAP URI decoding bugs

[Michael Paquier]

On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
 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

[HACKERS] LDAP URI decoding bugs

[Thomas Munro]

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


ldap-fixes.patch
Description: Binary data

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