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

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



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