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

At Thu, 16 Dec 2021 18:44:54 +0000, Jacob Champion <pchampion@vmware.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