Thread: BUG #16695: pg_hba_file_rules NULL address and netmask

The following bug has been logged on the website:

Bug reference:      16695
Logged by:          Peter Vandivier
Email address:      petervandivier@gmail.com
PostgreSQL version: 13.0
Operating system:   FreeBSD 11
Description:

Greetings,

pg_hba_file_rules reports NULL address and netmask values incorrectly on
FreeBSD 11 for tested postgres versions 10-13 (at least). e.g. 

```
sudo su
pkg update -f
pkg install postgresql10-server postgresql10-client
sysrc postgresql_enable=yes
service postgresql initdb
service postgresql start
psql -U postgres -c 'select * from pg_hba_file_rules'
sudo -u postgres cat $(psql -Xtc 'show hba_file') | tail -13
```

All postgres versions appear to report correct values for pg_hba_file_rules
on FreeBSD 12

Cheers,

Peter V

PG Bug reporting form <noreply@postgresql.org> writes:
> pg_hba_file_rules reports NULL address and netmask values incorrectly on
> FreeBSD 11 for tested postgres versions 10-13 (at least). e.g. 

This is kind of an unhelpful bug report: you did not show what results
you got, nor what you expected to get.

            regards, tom lane

PG Bug reporting form <noreply@postgresql.org> writes:
> pg_hba_file_rules reports NULL address and netmask values incorrectly on
> FreeBSD 11 for tested postgres versions 10-13 (at least). e.g. 

So for the archives' sake: what I suppose Peter means is that the addr
and netmask come out as NULL on every line, even where they should not.
At least, that's what I've reproduced here on FreeBSD 11.0.

I traced through this, and the proximate cause seems to be that
getnameinfo(3) is failing because it is expecting the passed "salen"
to be exactly the length it is expecting for the given sa_family.

Which it is not, because alone among our callers of pg_getnameinfo_all(),
fill_hba_line() thinks it can get away with passing sizeof(struct
sockaddr_storage) rather than the actual addrlen previously returned by
getaddrinfo().

The POSIX specification for getnameinfo() saith

    The sa argument points to a socket address structure to be
    translated. The salen argument contains the length of the address
    pointed to by sa.

so it seems to me that fill_hba_line() is clearly in the wrong.  There
are evidently a lot of implementations that either don't check salen
or only insist it be >= required length, but POSIX doesn't say they
need to be that lax.  (It'd be interesting to know if anyone sees
similar failures on any other BSDen.)

The core of the problem is somebody being lazy about what they needed
to put into HbaLine.  Unfortunately that's an exported structure so
there's some small risk of an ABI break, but I guess we can add the
length fields at the end in released branches to minimize the hazard.

Thanks for the report!

            regards, tom lane

I wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
>> pg_hba_file_rules reports NULL address and netmask values incorrectly on
>> FreeBSD 11 for tested postgres versions 10-13 (at least). e.g. 

> So for the archives' sake: what I suppose Peter means is that the addr
> and netmask come out as NULL on every line, even where they should not.
> At least, that's what I've reproduced here on FreeBSD 11.0.

> I traced through this, and the proximate cause seems to be that
> getnameinfo(3) is failing because it is expecting the passed "salen"
> to be exactly the length it is expecting for the given sa_family.
> Which it is not, because alone among our callers of pg_getnameinfo_all(),
> fill_hba_line() thinks it can get away with passing sizeof(struct
> sockaddr_storage) rather than the actual addrlen previously returned by
> getaddrinfo().

The attached seems to fix it for me.  (This is against HEAD, but a quick
check suggests it will apply cleanly down to v10.)  As I mentioned,
I'm planning to put the new fields at the end of struct HbaLine in
the back branches.

            regards, tom lane

diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 4c86fb6087..3a78d2043e 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1188,8 +1188,11 @@ parse_hba_line(TokenizedLine *tok_line, int elevel)

             ret = pg_getaddrinfo_all(str, NULL, &hints, &gai_result);
             if (ret == 0 && gai_result)
+            {
                 memcpy(&parsedline->addr, gai_result->ai_addr,
                        gai_result->ai_addrlen);
+                parsedline->addrlen = gai_result->ai_addrlen;
+            }
             else if (ret == EAI_NONAME)
                 parsedline->hostname = str;
             else
@@ -1238,6 +1241,7 @@ parse_hba_line(TokenizedLine *tok_line, int elevel)
                                         token->string);
                     return NULL;
                 }
+                parsedline->masklen = parsedline->addrlen;
                 pfree(str);
             }
             else if (!parsedline->hostname)
@@ -1288,6 +1292,7 @@ parse_hba_line(TokenizedLine *tok_line, int elevel)

                 memcpy(&parsedline->mask, gai_result->ai_addr,
                        gai_result->ai_addrlen);
+                parsedline->masklen = gai_result->ai_addrlen;
                 pg_freeaddrinfo_all(hints.ai_family, gai_result);

                 if (parsedline->addr.ss_family != parsedline->mask.ss_family)
@@ -2538,20 +2543,26 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
                 }
                 else
                 {
-                    if (pg_getnameinfo_all(&hba->addr, sizeof(hba->addr),
-                                           buffer, sizeof(buffer),
-                                           NULL, 0,
-                                           NI_NUMERICHOST) == 0)
+                    /*
+                     * Note: if pg_getnameinfo_all fails, it'll set buffer to
+                     * "???", which we want to return.
+                     */
+                    if (hba->addrlen > 0)
                     {
-                        clean_ipv6_addr(hba->addr.ss_family, buffer);
+                        if (pg_getnameinfo_all(&hba->addr, hba->addrlen,
+                                               buffer, sizeof(buffer),
+                                               NULL, 0,
+                                               NI_NUMERICHOST) == 0)
+                            clean_ipv6_addr(hba->addr.ss_family, buffer);
                         addrstr = pstrdup(buffer);
                     }
-                    if (pg_getnameinfo_all(&hba->mask, sizeof(hba->mask),
-                                           buffer, sizeof(buffer),
-                                           NULL, 0,
-                                           NI_NUMERICHOST) == 0)
+                    if (hba->masklen > 0)
                     {
-                        clean_ipv6_addr(hba->mask.ss_family, buffer);
+                        if (pg_getnameinfo_all(&hba->mask, hba->masklen,
+                                               buffer, sizeof(buffer),
+                                               NULL, 0,
+                                               NI_NUMERICHOST) == 0)
+                            clean_ipv6_addr(hba->mask.ss_family, buffer);
                         maskstr = pstrdup(buffer);
                     }
                 }
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index d638479d88..8f09b5638f 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -42,6 +42,10 @@ typedef enum UserAuth
 #define USER_AUTH_LAST uaPeer    /* Must be last value of this enum */
 } UserAuth;

+/*
+ * Data structures representing pg_hba.conf entries
+ */
+
 typedef enum IPCompareMethod
 {
     ipCmpMask,
@@ -75,11 +79,12 @@ typedef struct HbaLine
     List       *databases;
     List       *roles;
     struct sockaddr_storage addr;
+    int            addrlen;        /* zero if we don't have a valid addr */
     struct sockaddr_storage mask;
+    int            masklen;        /* zero if we don't have a valid mask */
     IPCompareMethod ip_cmp_method;
     char       *hostname;
     UserAuth    auth_method;
-
     char       *usermap;
     char       *pamservice;
     bool        pam_use_hostname;