Commit 0a94cf8f authored by Timothy B. Terriberry's avatar Timothy B. Terriberry

Fix two minor errors in hostname validation.

RFC 6125 says that if the host is an IP address, a subjectAltName of
 type iPAddress must (no 2119 caps) be present and must be used.
We would still fall back to checking the Common Name if no
 subjectAltName was present.

https://marc.info/?l=openssl-dev&m=139617145216047&w=2 interprets
 RFC 6125 to say that if the host is a DNS name, but the certificate
 only contains a subjectAltName of type iPAddress, then we should
 still fall back to checking the Common Name.
We would only check the Common Name if there was no subjectAltName
 of any type.

Restructure the hostname validation to check IP addresses up-front
 and fall back to checking the Common Name in the proper cases.
parent 56d33b2c
...@@ -1713,11 +1713,14 @@ static struct addrinfo *op_inet_pton(const char *_host){ ...@@ -1713,11 +1713,14 @@ static struct addrinfo *op_inet_pton(const char *_host){
the procedure from Section 6 of RFC 6125. the procedure from Section 6 of RFC 6125.
Return: 0 if the certificate doesn't match, and a non-zero value if it does.*/ Return: 0 if the certificate doesn't match, and a non-zero value if it does.*/
static int op_http_verify_hostname(OpusHTTPStream *_stream,SSL *_ssl_conn){ static int op_http_verify_hostname(OpusHTTPStream *_stream,SSL *_ssl_conn){
X509 *peer_cert; X509 *peer_cert;
STACK_OF(GENERAL_NAME) *san_names; struct addrinfo *addr;
char *host; char *host;
size_t host_len; size_t host_len;
int ret; unsigned char *ip;
int ip_len;
int check_cn;
int ret;
host=_stream->url.host; host=_stream->url.host;
host_len=strlen(host); host_len=strlen(host);
peer_cert=SSL_get_peer_certificate(_ssl_conn); peer_cert=SSL_get_peer_certificate(_ssl_conn);
...@@ -1725,139 +1728,150 @@ static int op_http_verify_hostname(OpusHTTPStream *_stream,SSL *_ssl_conn){ ...@@ -1725,139 +1728,150 @@ static int op_http_verify_hostname(OpusHTTPStream *_stream,SSL *_ssl_conn){
if(OP_UNLIKELY(peer_cert==NULL))return 0; if(OP_UNLIKELY(peer_cert==NULL))return 0;
ret=0; ret=0;
OP_ASSERT(host_len<INT_MAX); OP_ASSERT(host_len<INT_MAX);
/*RFC 2818 says (after correcting for Eratta 1077): "If a subjectAltName /*By default, fall back to checking the Common Name if we don't check any
extension of type dNSName is present, that MUST be used as the identity. subjectAltNames of type dNSName.*/
Otherwise, the (most specific) Common Name field in the Subject field of check_cn=1;
the certificate MUST be used. /*Check to see if the host was specified as a simple IP address.*/
Although the use of the Common Name is existing practice, it is deprecated addr=op_inet_pton(host);
and Certification Authorities are encouraged to use the dNSName ip=NULL;
instead." ip_len=0;
"Matching is performed using the matching rules specified by RFC 2459. if(addr!=NULL){
If more than one identity of a given type is present in the certificate switch(addr->ai_family){
(e.g., more than one dNSName name), a match in any one of the set is case AF_INET:{
considered acceptable. struct sockaddr_in *s;
Names may contain the wildcard character * which is condered to match any s=(struct sockaddr_in *)addr->ai_addr;
single domain name component or component fragment. OP_ASSERT(addr->ai_addrlen>=sizeof(*s));
E.g., *.a.com matches foo.a.com but not bar.foo.a.com. ip=(unsigned char *)&s->sin_addr;
f*.com matches foo.com but not bar.com." ip_len=sizeof(s->sin_addr);
"In some cases, the URI is specified as an IP address rather than a /*RFC 6125 says, "In this case, the iPAddress subjectAltName must [sic]
hostname. be present in the certificate and must [sic] exactly match the IP in
In this case, the iPAddress subjectAltName must be present in the the URI."
certificate and must exactly match the IP in the URI."*/ So don't allow falling back to a Common Name.*/
san_names=X509_get_ext_d2i(peer_cert,NID_subject_alt_name,NULL,NULL); check_cn=0;
if(san_names!=NULL){ }break;
struct addrinfo *addr; case AF_INET6:{
unsigned char *ip; struct sockaddr_in6 *s;
int ip_len; s=(struct sockaddr_in6 *)addr->ai_addr;
int nsan_names; OP_ASSERT(addr->ai_addrlen>=sizeof(*s));
int sni; ip=(unsigned char *)&s->sin6_addr;
/*Check to see if the host was specified as a simple IP address.*/ ip_len=sizeof(s->sin6_addr);
addr=op_inet_pton(host); check_cn=0;
ip=NULL; }break;
ip_len=0;
if(addr!=NULL){
switch(addr->ai_family){
case AF_INET:{
struct sockaddr_in *s;
s=(struct sockaddr_in *)addr->ai_addr;
OP_ASSERT(addr->ai_addrlen>=sizeof(*s));
ip=(unsigned char *)&s->sin_addr;
ip_len=sizeof(s->sin_addr);
}break;
case AF_INET6:{
struct sockaddr_in6 *s;
s=(struct sockaddr_in6 *)addr->ai_addr;
OP_ASSERT(addr->ai_addrlen>=sizeof(*s));
ip=(unsigned char *)&s->sin6_addr;
ip_len=sizeof(s->sin6_addr);
}break;
}
} }
/*We can only verify fully-qualified domain names. }
To quote RFC 6125: "The extracted data MUST include only information that /*We can only verify IP addresses and "fully-qualified" domain names.
can be securely parsed out of the inputs (e.g., parsing the fully To quote RFC 6125: "The extracted data MUST include only information that
qualified DNS domain name out of the "host" component (or its can be securely parsed out of the inputs (e.g., parsing the fully
equivalent) of a URI or deriving the application service type from the qualified DNS domain name out of the "host" component (or its
scheme of a URI) ..." equivalent) of a URI or deriving the application service type from the
We don't have a way to check (without relying on DNS records, which might scheme of a URI) ..."
be subverted) if this address is fully-qualified. We don't have a way to check (without relying on DNS records, which might
This is particularly problematic when using a CONNECT tunnel, as it is be subverted) if this address is fully-qualified.
the server that does DNS lookup, not us. This is particularly problematic when using a CONNECT tunnel, as it is
However, we are certain that if the hostname has no '.', it is definitely the server that does DNS lookup, not us.
not a fully-qualified domain name (with the exception of crazy TLDs that However, we are certain that if the hostname has no '.', it is definitely
actually resolve, like "uz", but I am willing to ignore those). not a fully-qualified domain name (with the exception of crazy TLDs that
RFC 1535 says "...in any event where a '.' exists in a specified name it actually resolve, like "uz", but I am willing to ignore those).
should be assumed to be a fully qualified domain name (FQDN) and SHOULD RFC 1535 says "...in any event where a '.' exists in a specified name it
be tried as a rooted name first." should be assumed to be a fully qualified domain name (FQDN) and SHOULD
That doesn't give us any security guarantees, of course (a subverted DNS be tried as a rooted name first."
could fail the original query and our resolver might still retry with a That doesn't give us any security guarantees, of course (a subverted DNS
local domain appended). could fail the original query and our resolver might still retry with a
If we don't have a FQDN, just set the number of names to 0, so we'll fail local domain appended).*/
and clean up any resources we allocated.*/ if(ip!=NULL||strchr(host,'.')!=NULL){
if(ip==NULL&&strchr(host,'.')==NULL)nsan_names=0; STACK_OF(GENERAL_NAME) *san_names;
/*RFC 2459 says there MUST be at least one, but we don't depend on it.*/ /*RFC 2818 says (after correcting for Eratta 1077): "If a subjectAltName
else nsan_names=sk_GENERAL_NAME_num(san_names); extension of type dNSName is present, that MUST be used as the identity.
for(sni=0;sni<nsan_names;sni++){ Otherwise, the (most specific) Common Name field in the Subject field of
const GENERAL_NAME *name; the certificate MUST be used.
name=sk_GENERAL_NAME_value(san_names,sni); Although the use of the Common Name is existing practice, it is
if(ip==NULL){ deprecated and Certification Authorities are encouraged to use the
if(name->type==GEN_DNS dNSName instead."
&&op_http_hostname_match(host,host_len,name->d.dNSName)){ "Matching is performed using the matching rules specified by RFC 2459.
ret=1; If more than one identity of a given type is present in the certificate
break; (e.g., more than one dNSName name), a match in any one of the set is
considered acceptable.
Names may contain the wildcard character * which is condered to match any
single domain name component or component fragment.
E.g., *.a.com matches foo.a.com but not bar.foo.a.com.
f*.com matches foo.com but not bar.com."
"In some cases, the URI is specified as an IP address rather than a
hostname.
In this case, the iPAddress subjectAltName must be present in the
certificate and must exactly match the IP in the URI."*/
san_names=X509_get_ext_d2i(peer_cert,NID_subject_alt_name,NULL,NULL);
if(san_names!=NULL){
int nsan_names;
int sni;
/*RFC 2459 says there MUST be at least one, but we don't depend on it.*/
nsan_names=sk_GENERAL_NAME_num(san_names);
for(sni=0;sni<nsan_names;sni++){
const GENERAL_NAME *name;
name=sk_GENERAL_NAME_value(san_names,sni);
if(ip==NULL){
if(name->type==GEN_DNS){
/*We have a subjectAltName extension of type dNSName, so don't fall
back to a Common Name.
https://marc.info/?l=openssl-dev&m=139617145216047&w=2 says that
subjectAltNames of other types do not trigger this restriction,
(e.g., if they are all IP addresses, we will still check a
non-IP hostname against a Common Name).*/
check_cn=0;
if(op_http_hostname_match(host,host_len,name->d.dNSName)){
ret=1;
break;
}
}
} }
} else if(name->type==GEN_IPADD){
else if(name->type==GEN_IPADD){ unsigned const char *cert_ip;
unsigned const char *cert_ip; /*If we do have an IP address, compare it directly.
/*If we do have an IP address, compare it directly. RFC 6125: "When the reference identity is an IP address, the
RFC 6125: "When the reference identity is an IP address, the identity identity MUST be converted to the 'network byte order' octet
MUST be converted to the 'network byte order' octet string string representation.
representation. For IP Version 4, as specified in RFC 791, the octet string will
For IP Version 4, as specified in RFC 791, the octet string will contain exactly four octets.
contain exactly four octets. For IP Version 6, as specified in RFC 2460, the octet string will
For IP Version 6, as specified in RFC 2460, the octet string will contain exactly sixteen octets.
contain exactly sixteen octets. This octet string is then compared against subjectAltName values of
This octet string is then compared against subjectAltName values of type iPAddress.
type iPAddress. A match occurs if the reference identity octet string and the value
A match occurs if the reference identity octet string and the value octet strings are identical."*/
octet strings are identical."*/ cert_ip=ASN1_STRING_get0_data(name->d.iPAddress);
cert_ip=ASN1_STRING_get0_data(name->d.iPAddress); if(ip_len==ASN1_STRING_length(name->d.iPAddress)
if(ip_len==ASN1_STRING_length(name->d.iPAddress) &&memcmp(ip,cert_ip,ip_len)==0){
&&memcmp(ip,cert_ip,ip_len)==0){ ret=1;
ret=1; break;
break; }
} }
} }
sk_GENERAL_NAME_pop_free(san_names,GENERAL_NAME_free);
} }
sk_GENERAL_NAME_pop_free(san_names,GENERAL_NAME_free); /*If we're supposed to fall back to a Common Name, match against it here.*/
if(addr!=NULL)freeaddrinfo(addr); if(check_cn){
} int last_cn_loc;
/*Do the same FQDN check we did above. int cn_loc;
We don't do this once in advance for both cases, because in the /*RFC 6125 says that at least one significant CA is known to issue certs
subjectAltName case we might have an IPv6 address without a dot.*/ with multiple CNs, although it SHOULD NOT.
else if(strchr(host,'.')!=NULL){ It also says: "The server's identity may also be verified by comparing
int last_cn_loc; the reference identity to the Common Name (CN) value in the last
int cn_loc; Relative Distinguished Name (RDN) of the subject field of the server's
/*If there is no subjectAltName, match against commonName. certificate (where "last" refers to the DER-encoded order...)."
RFC 6125 says that at least one significant CA is known to issue certs So find the last one and check it.*/
with multiple CNs, although it SHOULD NOT. cn_loc=-1;
It also says: "The server's identity may also be verified by comparing do{
the reference identity to the Common Name (CN) value in the last last_cn_loc=cn_loc;
Relative Distinguished Name (RDN) of the subject field of the server's cn_loc=X509_NAME_get_index_by_NID(X509_get_subject_name(peer_cert),
certificate (where "last" refers to the DER-encoded order...)." NID_commonName,last_cn_loc);
So find the last one and check it.*/ }
cn_loc=-1; while(cn_loc>=0);
do{ ret=last_cn_loc>=0
last_cn_loc=cn_loc; &&op_http_hostname_match(host,host_len,
cn_loc=X509_NAME_get_index_by_NID(X509_get_subject_name(peer_cert), X509_NAME_ENTRY_get_data(
NID_commonName,last_cn_loc); X509_NAME_get_entry(X509_get_subject_name(peer_cert),last_cn_loc)));
} }
while(cn_loc>=0);
ret=last_cn_loc>=0
&&op_http_hostname_match(host,host_len,
X509_NAME_ENTRY_get_data(
X509_NAME_get_entry(X509_get_subject_name(peer_cert),last_cn_loc)));
} }
if(addr!=NULL)freeaddrinfo(addr);
X509_free(peer_cert); X509_free(peer_cert);
return ret; return ret;
} }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment