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

At Thu, 16 Dec 2021 18:44:54 +0000, Jacob Champion <pchampion(at)vmware(dot)com> wrote in
> On Thu, 2021-12-16 at 14:54 +0900, Kyotaro Horiguchi wrote:
> > It seems like saying that we must search for iPAddress and mustn't use
> > CN nor dNSName if the client connected using IP address. Otherwise, if
> > the host name is a domain name, we use only dNSName if present, and
> > use CN otherwise. That behavior seems agreeing to what you wrote as
> > NSS's behavior.
>
> NSS departs slightly from the spec and will additionally try to match
> an IP address against the CN, but only if there are no iPAddresses in
> the SAN. It roughly matches the logic for DNS names.

OpenSSL seems different. X509_check_host() tries SAN then CN iff SAN
doesn't exist. X509_check_ip() tries SAN and completely ignores
iPAdress and CN.

> Here's the description of the NSS behavior and some of the reasoning
> behind it, quoted from a developer on Bugzilla [1]:
>
> > Elsewhere in RFC 2818, it says
> >
> > If a subjectAltName extension of type dNSName is present, that MUST
> > be used as the identity. Otherwise, the (most specific) Common Name
> > field in the Subject field of the certificate MUST be used.
> >
> > Notice that this section is not conditioned upon the URI being a hostname
> > and not an IP address. So this statement conflicts with the one cited
> > above.
> >
> > I implemented this policy:
> >
> > if the URI contains a host name
> > if the subject alt name is present and has one or more DNS names
> > use the DNS names in that extension as the server identity
> > else
> > use the subject common name as the server identity
> > else if the URI contains an IP address
> > if the subject alt name is present and has one or more IP addresses
> > use the IP addresses in that extension as the server identity
> > else
> > compare the URI IP address string with the subject common name.
(Wow. The article is 20-years old.)

*I* am fine with it.

> It sounds like both you and Andrew might be comfortable with that same
> behavior? I think it looks like a sane solution, so I'll implement that
> and we can see what it looks like. (My work on this will be paused over
> the end-of-year holidays.)

> > I'm not sure about ipv4 comptible addresses. However, I think we can
> > identify ipv4 compatible address easily.
>
> Yeah, it would probably not be a difficult feature to add later.

I agree.

> > + * pg_inet_net_pton() will accept CIDR masks, which we don't want to
> > + * match, so skip the comparison if the host string contains a slash.
> > + */
> > + if (!strchr(host, '/')
> > + && pg_inet_net_pton(PGSQL_AF_INET6, host, addr, -1) == 128)
> >
> > If a cidr is given, pg_inet_net_pton returns a number less than 128 so
> > we don't need to check '/' explicity? (I'm not sure '/128' is
> > sensible but doesn't harm..)
>
> Personally I think that, if someone wants your libpq to connect to a
> server with a hostname of "some:ipv6::address/128", then they are
> trying to pull something (evading a poorly coded blocklist, perhaps?)
> and we should not allow that to match an IP. Thoughts?

If the client could connect to the network-address, it could be said
that we can assume that address is the name:p Just kidding.

As the name suggests, the function reads a network address. And the
only user is network_in(). I think we should provide pg_inet_pton()
instead of abusing pg_inet_net_pton(). inet_net_pton_*() functions
can be modified to reject /cidr part without regression so we are able
to have pg_inet_pton() with a small amount of change.

- inet_net_pton_ipv4(const char *src, u_char *dst)
+ inet_net_pton_ipv4_internal(const char *src, u_char *dst, bool netaddr)

+ inet_net_pton_ipv4(const char *src, u_char *dst)
(calls inet_net_pton_ipv4_internal(src, dst, true))
+ inet_pton_ipv4(const char *src, u_char *dst)
(calls inet_net_pton_ipv4_internal(src, dst, false))

> Thanks for the review!
> --Jacob
>
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=103752

--
Kyotaro Horiguchi
NTT Open Source Software Center