small issue with host names in hba

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.
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.

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

On Tue, Aug 9, 2011 at 2:16 PM, Peter Eisentraut <peter_e(at)gmx(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Aug 9, 2011 at 2:16 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> But I'm a little confused by what this code is really trying
>> to accomplish: ...

> 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.

POSIX v7 says

If the node's name cannot be located, the numeric form of the
address contained in the socket address structure pointed to by
the sa argument is returned instead of its name.

If you read a bit further, apparently this is just supposed to be the
default behavior if neither NI_NUMERICHOST nor NI_NAMEREQD is set (in
the first case, it doesn't try to locate a node name, and in the second,
it fails if it can't locate a node name). But certainly the dance in
postmaster.c is not necessary if you believe the spec.

I believe that the existing coding is meant to cope with the behavior of
our stub version of getnameinfo(), which simply fails outright if
NI_NUMERICHOST isn't set. However, if we just removed that test and
behaved as per spec (return a numeric address anyway), then we could
eliminate the second call in postmaster.c.

>> 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.

Well, it's not just that it "doesn't support that flag". It's
fundamentally incapable of returning anything but a numeric address,
and therefore the only "support" it could offer would be to fail. So
that approach seems like a dead end.

I don't really think that there's anything to fix here with respect to
Peter's original concern, but it might be worth getting rid of the
double call in postmaster.c.

regards, tom lane

I assume we didn't feel any action was necessary on this issue.

---------------------------------------------------------------------------

On Thu, Aug 11, 2011 at 01:50:02PM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Tue, Aug 9, 2011 at 2:16 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> >> But I'm a little confused by what this code is really trying
> >> to accomplish: ...
>
> > 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.
>
> POSIX v7 says
>
> If the node's name cannot be located, the numeric form of the
> address contained in the socket address structure pointed to by
> the sa argument is returned instead of its name.
>
> If you read a bit further, apparently this is just supposed to be the
> default behavior if neither NI_NUMERICHOST nor NI_NAMEREQD is set (in
> the first case, it doesn't try to locate a node name, and in the second,
> it fails if it can't locate a node name). But certainly the dance in
> postmaster.c is not necessary if you believe the spec.
>
> I believe that the existing coding is meant to cope with the behavior of
> our stub version of getnameinfo(), which simply fails outright if
> NI_NUMERICHOST isn't set. However, if we just removed that test and
> behaved as per spec (return a numeric address anyway), then we could
> eliminate the second call in postmaster.c.
>
> >> 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.
>
> Well, it's not just that it "doesn't support that flag". It's
> fundamentally incapable of returning anything but a numeric address,
> and therefore the only "support" it could offer would be to fail. So
> that approach seems like a dead end.
>
> I don't really think that there's anything to fix here with respect to
> Peter's original concern, but it might be worth getting rid of the
> double call in postmaster.c.
>
> regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Tue, 2012-08-14 at 18:52 -0400, Bruce Momjian wrote:
> I assume we didn't feel any action was necessary on this issue.

I propose the attached patch to reduce the redundant code as discussed.

>
> ---------------------------------------------------------------------------
>
> On Thu, Aug 11, 2011 at 01:50:02PM -0400, Tom Lane wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > > On Tue, Aug 9, 2011 at 2:16 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > >> But I'm a little confused by what this code is really trying
> > >> to accomplish: ...
> >
> > > 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.
> >
> > POSIX v7 says
> >
> > If the node's name cannot be located, the numeric form of the
> > address contained in the socket address structure pointed to by
> > the sa argument is returned instead of its name.
> >
> > If you read a bit further, apparently this is just supposed to be the
> > default behavior if neither NI_NUMERICHOST nor NI_NAMEREQD is set (in
> > the first case, it doesn't try to locate a node name, and in the second,
> > it fails if it can't locate a node name). But certainly the dance in
> > postmaster.c is not necessary if you believe the spec.
> >
> > I believe that the existing coding is meant to cope with the behavior of
> > our stub version of getnameinfo(), which simply fails outright if
> > NI_NUMERICHOST isn't set. However, if we just removed that test and
> > behaved as per spec (return a numeric address anyway), then we could
> > eliminate the second call in postmaster.c.
> >
> > >> 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.
> >
> > Well, it's not just that it "doesn't support that flag". It's
> > fundamentally incapable of returning anything but a numeric address,
> > and therefore the only "support" it could offer would be to fail. So
> > that approach seems like a dead end.
> >
> > I don't really think that there's anything to fix here with respect to
> > Peter's original concern, but it might be worth getting rid of the
> > double call in postmaster.c.
> >
> > regards, tom lane
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
>
> --
> Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + It's impossible for everything to be true. +
>
>