Re: LDAP URI decoding bugs

On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)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