Re: [PATCH] Accept IP addresses in server certificate SANs

On Mon, 2022-02-07 at 17:29 +0900, Kyotaro Horiguchi wrote:
> At Fri, 4 Feb 2022 17:06:53 +0000, Jacob Champion <pchampion(at)vmware(dot)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