Re: small issue with host names in hba

On Tue, Aug 9, 2011 at 2:16 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> When a host name is used in pg_hba.conf, then we call
> pg_getnameinfo_all() to get the host name for the client's IP address,
> either in postmaster.c or in hba.c, whichever happens first.  But if the
> IP address has no host name, the getnameinfo calls return the IP address
> in text form, which is then stored into port->remote_hostname as the
> "host name".  This "host name" is then compared to the name in
> pg_hba.conf.  It cannot match, because we only do this dance if the
> entry in pg_hba.conf does not parse as an IP address.  So I don't think
> there is a direct security problem here, but it still seems odd.
>
> The fix would appear to be using the NI_NAMEREQD flag to getnameinfo.

If you want to do that, you're going to have to fix the version of
getnameinfo() in src/port/getaddrinfo.c, which apparently doesn't
support that flag.  Per the header comment:

/** Convert an ipv4 address to a hostname.** Bugs:        - Only supports NI_NUMERICHOST and NI_NUMERICSERV*
   It will never resolv a hostname.*              - No IPv6 support.*/ 

> We could do that in hba.c, but in postmaster.c we would have to move
> some code around to maintain a string version of the IP address for
> logging.  But I'm a little confused by what this code is really trying
> to accomplish:
>
>    if (pg_getnameinfo_all(&port->raddr.addr, port->raddr.salen,
>                           remote_host, sizeof(remote_host),
>                           remote_port, sizeof(remote_port),
>                       (log_hostname ? 0 : NI_NUMERICHOST) | NI_NUMERICSERV))
>    {
>        int         ret = pg_getnameinfo_all(&port->raddr.addr, port->raddr.salen,
>                                             remote_host, sizeof(remote_host),
>                                             remote_port, sizeof(remote_port),
>                                             NI_NUMERICHOST | NI_NUMERICSERV);
>
>        if (ret)
>            ereport(WARNING,
>                    (errmsg_internal("pg_getnameinfo_all() failed: %s",
>                                     gai_strerror(ret))));
>    }
>
> If log_hostname is off, this just make the same function call twice.  If
> log_hostname is on (which is what we are concerned about here), it makes
> the same call but with the addition of the NI_NUMERICHOST flag.  But
> getnameinfo already returns a numeric representation of no actual host
> name is available, unless said NI_NAMEREQD is used.

I think the intended behavior of NI_NUMERICHOST is to suppress the
name lookup, and return the text format *even if* the name lookup
would have worked.  So the intention of this code may be to ensure
that we convert the the sockaddr to some sort of string even if we
can't resolve the hostname - i.e. if the first call fails, try again
with NI_NUMERICHOST added to make sure that we didn't fail solely due
to some kind of DNS hiccup.  I suspect that at some time this was
defending against an actual hazard but I don't know whether it's still
a problem on modern operating systems.  Of course, if log_hostname is
off then it's purely redundancy, although I doubt that there is any
practical performance consequence.

> So, do we need to fix the issue described in the first paragraph?

I'm not sure I see the point, but I won't argue with you too much if
you feel strongly about it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company