Re: [PATCH] Accept IP addresses in server certificate SANs - Mailing list pgsql-hackers

On Mon, 2022-02-07 at 17:29 +0900, Kyotaro Horiguchi wrote:
> At Fri, 4 Feb 2022 17:06:53 +0000, Jacob Champion <pchampion@vmware.com> wrote in 
> > That works a lot better than what I had in my head. Done that way in
> > v4. Thanks!
> 
> Thanks!
> 
> 0002:
> 
> +#define PGSQL_AF_INET  (AF_INET + 0)
> +#define PGSQL_AF_INET6 (AF_INET + 1)
> ..
> -#define PGSQL_AF_INET  (AF_INET + 0)
> -#define PGSQL_AF_INET6 (AF_INET + 1)
> 
> I feel this should be a part of 0001.  (But the patches will be
> finally merged so maybe no need to bother moving it).

Okay. I can move it easily if you feel like it would help review, but
for now I've kept it in 0002.

> > * The use of inet_aton() instead of inet_pton() is deliberate; the
> > * latter cannot handle alternate IPv4 notations ("numbers-and-dots").
> 
> I think we should be consistent in handling IP addresses.  We have
> both inet_pton and inet_aton to parse IPv4 addresses.
> 
> We use inet_pton in the inet type (network_in).
> We use inet_aton in server addresses.
> 
> # Hmm. I'm surprised to see listen_addresses accepts "0x7f.1".
> # I think we should accept the same by network_in but it is another
> # issue.

Yeah, that's an interesting inconsistency.

> So, inet_aton there seems to be the right choice but the comment
> doesn't describe the reason for that behavior. I think we should add
> an explanation about the reason for the behavior, maybe something like
> this:
> 
> > We accept alternative IPv4 address notations that are accepted by
> > inet_aton but not by inet_pton as server address.

I've pulled this wording into the comment in v5, attached.

> +        * GEN_IPADD is an OCTET STRING containing an IP address in network byte
> +        * order.
> 
> +       /* OK to cast from unsigned to plain char, since it's all ASCII. */
> +       return pq_verify_peer_name_matches_certificate_ip(conn, (const char *) addrdata, len, store_name);
> 
> Aren't the two comments contradicting each other? The retruned general
> name looks like an octet array, which is not a subset of ASCII
> string. So pq_verify_peer_name_matches_certificate_ip should receive
> addrdata as "const unsigned char *", without casting.

Bad copy-paste on my part; thanks for the catch. Fixed.

> +                       if (name->type == host_type)
> +                               check_cn = false;
> 
> Don't we want a concise coment for this?

Added one; see what you think.

> -       if (*names_examined == 0)
> +       if ((rc == 0) && check_cn)
> 
> To me, it seems a bit hard to understand.  We can set false to
> check_cn in the rc != 0 path in the loop on i, like this:
> 
> >                       if (rc != 0)
> > +                     {
> > +                             /*
> > +                              * don't fall back to CN when we have a match or have an error
> > +                              */
> > +                             check_cn = false;
> >                               break;
> > +                     }
> ...
> > -     if ((rc == 0) && check_cn)
> > +     if (check_cn)

If I understand right, that's not quite equivalent (and the new tests
fail if I implement it that way). We have to disable fallback if the
SAN exists, whether it matches or not.

> The following existing code (CN fallback)
> 
> >       rc = openssl_verify_peer_name_matches_certificate_name(conn,
> >       X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject_name, cn_index)),
> >       first_name);
> 
> is expecting that first_name has not been set when it is visited.
> However, with this patch, first_name can be set when the cert has any
> SAN of unmatching type (DNS/IPADD) and the already-set name leaks.  We
> need to avoid that memory leak since the path can be visited multiple
> times from the user-program of libpq. I came up with two directions.
> 
> 1. Completely ignore type-unmatching entries. first_name is not set by
>  such entries.  Such unmatching entreis doesn't increase
>  *names_examined.
> 
> 2. Avoid overwriting first_name there.
> 
> I like 1, but since we don't make distinction between DNS and IPADDR
> in the error message emited by the caller, we would take 2?

Great catch, thanks! I implemented option 2 to start. Option 1 might
make things difficult to debug if you're connecting to a server by IP
address but its certificate only has DNS names.

Thanks!
--Jacob