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

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

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

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.

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

+ if (name->type == host_type)
+ check_cn = false;

Don't we want a concise coment for this?

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

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?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center