LDAP URI decoding bugs
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t37673psql -h localhost -U postgresBuilt from patchset v1 (message #1), July 27, 2026 at 09:51 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t37673_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t37673_1 && git checkout t37673_1Patchset v1 (message #1) is on t37673_1
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
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
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
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