Re: [HACKERS] LDAP URI decoding bugs
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
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
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
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
