Skip to content

Commit

Permalink
Update TLS identity checks to simplify literal address test, avoid
Browse files Browse the repository at this point in the history 
matching a DNS name against an IP address, and remove URI altname
support since this is not specified by RFC 9110/6125.

* src/ne_openssl.c (check_identity): Take the host_info rather than a
  URI; only test DNS names for a non-literal identifier, and only test
  IP addresses for a literal identifier.
  (check_certificate): Adjust accordingly.

* src/ne_gnutls.c (check_identity, check_certificate): Update
  similarly.

* src/ne_session.c (ne__ssl_match_hostname): Drop now unnecessary
  safety check for IP addresses here.

* test/ssl.c (uri_altname, fail_bad_urialtname): Removed tests.
  (fail_ssl_request_with_error2): Update to use
  fakeproxied_session_server.
  (cert_identities): Drop altname8.cert (URI altname).

* test/makekeys.sh, test/openssl.conf: Drop URI altname cert generation.
  • Loading branch information
@notroj
notroj committed Jan 28, 2024
1 parent 3203d95 commit 7b7ee9d
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 148 deletions.
50 changes: 11 additions & 39 deletions src/ne_gnutls.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ void ne_ssl_cert_validity_time(const ne_ssl_certificate *cert,
* If 'identity' is non-NULL, store the malloc-allocated identity in
* *identity. If 'server' is non-NULL, it must be the network address
* of the server in use, and identity must be NULL. */
static int check_identity(const ne_uri *server, gnutls_x509_crt_t cert,
static int check_identity(const struct host_info *server, gnutls_x509_crt_t cert,
char **identity)
{
char name[255];
Expand All @@ -374,7 +374,7 @@ static int check_identity(const ne_uri *server, gnutls_x509_crt_t cert,
size_t len;
const char *hostname;

hostname = server ? server->host : "";
hostname = server ? server->hostname : "";

do {
len = sizeof name - 1;
Expand All @@ -384,7 +384,10 @@ static int check_identity(const ne_uri *server, gnutls_x509_crt_t cert,
case GNUTLS_SAN_DNSNAME:
name[len] = '\0';
if (identity && !found) *identity = ne_strdup(name);
match = ne__ssl_match_hostname(name, len, hostname);
/* Only match if the server was not identified by a
* literal IP address; avoiding wildcard matches. */
if (server && !server->literal)
match = ne__ssl_match_hostname(name, len, hostname);
found = 1;
break;
case GNUTLS_SAN_IPADDRESS: {
Expand All @@ -396,44 +399,17 @@ static int check_identity(const ne_uri *server, gnutls_x509_crt_t cert,
else
ia = NULL;
if (ia) {
char buf[128];

match = strcmp(hostname,
ne_iaddr_print(ia, buf, sizeof buf)) == 0;
if (identity) *identity = ne_strdup(buf);
if (server && server->literal)
match = ne_iaddr_cmp(ia, server->literal) == 0;
found = 1;
ne_iaddr_free(ia);
} else {
}
else {
NE_DEBUG(NE_DBG_SSL, "iPAddress name with unsupported "
"address type (length %" NE_FMT_SIZE_T "), skipped.\n",
len);
}
} break;
case GNUTLS_SAN_URI: {
ne_uri uri;

name[len] = '\0';

if (ne_uri_parse(name, &uri) == 0 && uri.host && uri.scheme) {
ne_uri tmp;

if (identity && !found) *identity = ne_strdup(name);
found = 1;

if (server) {
/* For comparison purposes, all that matters is
* host, scheme and port; ignore the rest. */
memset(&tmp, 0, sizeof tmp);
tmp.host = uri.host;
tmp.scheme = uri.scheme;
tmp.port = uri.port;

match = ne_uri_cmp(server, &tmp) == 0;
}
}

ne_uri_free(&uri);
} break;

default:
break;
Expand Down Expand Up @@ -940,13 +916,9 @@ static int check_certificate(ne_session *sess, gnutls_session_t sock,
ne_ssl_certificate *chain)
{
int ret, failures = 0;
ne_uri server;
unsigned int status;

memset(&server, 0, sizeof server);
ne_fill_server_uri(sess, &server);
ret = check_identity(&server, chain->subject, NULL);
ne_uri_free(&server);
ret = check_identity(&sess->server, chain->subject, NULL);

if (ret < 0) {
ne_set_error(sess, _("Server certificate was missing commonName "
Expand Down
53 changes: 13 additions & 40 deletions src/ne_openssl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,13 +244,14 @@ void ne_ssl_cert_validity_time(const ne_ssl_certificate *cert,
* identity does not match, or <0 if the certificate had no identity.
* If 'identity' is non-NULL, store the malloc-allocated identity in
* *identity. Logic specified by RFC 2818 and RFC 3280. */
static int check_identity(const ne_uri *server, X509 *cert, char **identity)
static int check_identity(const struct host_info *server, X509 *cert,
char **identity)
{
STACK_OF(GENERAL_NAME) *names;
int match = 0, found = 0;
const char *hostname;

hostname = server ? server->host : "";
hostname = server ? server->hostname : "";

names = X509_get_ext_d2i(cert, NID_subject_alt_name, NULL, NULL);
if (names) {
Expand All @@ -264,12 +265,16 @@ static int check_identity(const ne_uri *server, X509 *cert, char **identity)
if (nm->type == GEN_DNS) {
char *name = dup_ia5string(nm->d.ia5);
if (identity && !found) *identity = ne_strdup(name);
match = ne__ssl_match_hostname(name, strlen(name), hostname);

/* Only match if the server was not identified by a
* literal IP address; avoiding wildcard matches. */
if (server && !server->literal)
match = ne__ssl_match_hostname(name, strlen(name), hostname);
ne_free(name);
found = 1;
}
else if (nm->type == GEN_IPADD) {
/* compare IP address with server IP address. */
else if (nm->type == GEN_IPADD && server && server->literal) {
/* compare IP addfress with server literal IP address. */
ne_inet_addr *ia;
if (nm->d.ip->length == 4)
ia = ne_iaddr_make(ne_iaddr_ipv4, nm->d.ip->data);
Expand All @@ -279,44 +284,16 @@ static int check_identity(const ne_uri *server, X509 *cert, char **identity)
ia = NULL;
/* ne_iaddr_make returns NULL if address type is unsupported */
if (ia != NULL) { /* address type was supported. */
char buf[128];

match = strcmp(hostname,
ne_iaddr_print(ia, buf, sizeof buf)) == 0;
match = ne_iaddr_cmp(ia, server->literal) == 0;
found = 1;
ne_iaddr_free(ia);
} else {
NE_DEBUG(NE_DBG_SSL, "iPAddress name with unsupported "
"address type (length %d), skipped.\n",
nm->d.ip->length);
}
}
else if (nm->type == GEN_URI) {
char *name = dup_ia5string(nm->d.ia5);
ne_uri uri;

if (ne_uri_parse(name, &uri) == 0 && uri.host && uri.scheme) {
ne_uri tmp;

if (identity && !found) *identity = ne_strdup(name);
found = 1;

if (server) {
/* For comparison purposes, all that matters is
* host, scheme and port; ignore the rest. */
memset(&tmp, 0, sizeof tmp);
tmp.host = uri.host;
tmp.scheme = uri.scheme;
tmp.port = uri.port;

match = ne_uri_cmp(server, &tmp) == 0;
}
}

ne_uri_free(&uri);
ne_free(name);
}
}
}
/* free the whole stack. */
sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free);
}
Expand Down Expand Up @@ -459,7 +436,6 @@ static int check_certificate(ne_session *sess, SSL *ssl, ne_ssl_certificate *cha
{
X509 *cert = chain->subject;
int ret, failures = sess->ssl_context->failures;
ne_uri server;

/* If the verification callback hit a case which can't be mapped
* to one of the exported error bits, it's treated as a hard
Expand All @@ -476,10 +452,7 @@ static int check_certificate(ne_session *sess, SSL *ssl, ne_ssl_certificate *cha

/* Check certificate was issued to this server; pass URI of
* server. */
memset(&server, 0, sizeof server);
ne_fill_server_uri(sess, &server);
ret = check_identity(&server, cert, NULL);
ne_uri_free(&server);
ret = check_identity(&sess->server, cert, NULL);
if (ret < 0) {
ne_set_error(sess, _("Server certificate was missing commonName "
"attribute in subject name"));
Expand Down
19 changes: 0 additions & 19 deletions src/ne_session.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -610,25 +610,6 @@ int ne__ssl_match_hostname(const char *cn, size_t cnlen, const char *hostname)

if (strncmp(cn, "*.", 2) == 0 && cnlen > 2
&& (dot = strchr(hostname, '.')) != NULL) {
ne_inet_addr *ia;

/* Prevent wildcard CN matches against anything which can be
* parsed as an IP address (i.e. a CN of "*.1.1.1" should not
* be match 8.1.1.1). draft-saintandre-tls-server-id-check
* will require some more significant changes to cert ID
* verification which will probably obviate this check, but
* this is a desirable policy tightening in the mean time. */
ia = ne_iaddr_parse(hostname, ne_iaddr_ipv4);
if (ia == NULL)
ia = ne_iaddr_parse(hostname, ne_iaddr_ipv6);

if (ia) {
NE_DEBUG(NE_DBG_SSL, "ssl: Denying wildcard match for numeric "
"IP address.\n");
ne_iaddr_free(ia);
return 0;
}

hostname = dot + 1;
cn += 2;
cnlen -= 2;
Expand Down
2 changes: 1 addition & 1 deletion test/makekeys.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ ${CA} -startdate `asn1date "2 days ago"` -enddate `asn1date "yesterday"` -in exp

${CA} -startdate `asn1date "tomorrow"` -enddate `asn1date "2 days"` -in notyet.csr -out notyet.cert

for n in 1 2 3 4 5 6 7 8 9; do
for n in 1 2 3 4 5 6 9; do
${CA} -extensions altExt${n} -days 900 \
-in altname${n}.csr -out altname${n}.cert
done
Expand Down
8 changes: 0 additions & 8 deletions test/openssl.conf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,6 @@ subjectAltName = IP:127.0.0.1
[altExt6]
subjectAltName = IP:1.2.3.4

# an AltName with a good URI
[altExt7]
subjectAltName = URI:https://localhost:7777/

# an AltName with a bad URI
[altExt8]
subjectAltName = URI:http://nohost.example.com/

# AltName with wildcard
[altExt9]
subjectAltName = DNS:*.example.com
Expand Down
51 changes: 10 additions & 41 deletions test/ssl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -592,11 +592,6 @@ static int ipaddr_altname(void)
return accept_signed_cert_for_hostname("altname5.cert", "127.0.0.1");
}

static int uri_altname(void)
{
return accept_signed_cert_for_hostname("altname7.cert", "localhost");
}

/* test that the *most specific* commonName attribute is used. */
static int multi_commonName(void)
{
Expand Down Expand Up @@ -788,42 +783,25 @@ static int fail_ssl_request_with_error2(char *cert, char *key, char *cacert,
const char *msg, int failures,
const char *errstr)
{
ne_session *sess = ne_session_create("https", host, 7777);
ne_session *sess;
int gotf = 0, ret;
struct ssl_server_args args = {0};
ne_sock_addr *addr = NULL;
const ne_inet_addr **list = NULL;

if (realhost) {
size_t n;
const ne_inet_addr *ia;

addr = ne_addr_resolve(realhost, 0);

ONV(ne_addr_result(addr),
("fake hostname lookup failed for %s", realhost));

NE_DEBUG(NE_DBG_SSL, "ssl: Using fake hostname '%s'\n", realhost);

for (n = 0, ia = ne_addr_first(addr); ia; ia = ne_addr_next(addr))
n++;

NE_DEBUG(NE_DBG_SSL, "ssl: Address count '%lu'\n", n);

list = ne_calloc(n * sizeof(*list));

for (n = 0, ia = ne_addr_first(addr); ia; ia = ne_addr_next(addr))
list[n++] = ia;

ne_set_addrlist(sess, list, n);
}

args.cert = cert;
args.key = key;
args.fail_silently = 1;

ret = any_ssl_request(sess, ssl_server, &args, cacert,
get_failures, &gotf);
CALL(fakeproxied_session_server(&sess, "https", host, 7777, ssl_server, &args));

if (cacert) {
CALL(load_and_trust_cert(sess, cacert));
}

ne_ssl_set_verify(sess, get_failures, &gotf);

ret = any_request(sess, "/expect-to-fail");

ONV(gotf == 0,
("no error in verification callback; request rv %d error string: %s",
Expand Down Expand Up @@ -982,12 +960,6 @@ static int fail_host_ipaltname(void)
"bad IP altname cert", NE_SSL_IDMISMATCH);
}

static int fail_bad_urialtname(void)
{
return fail_ssl_request("altname8.cert", CA_CERT, "localhost",
"bad URI altname cert", NE_SSL_IDMISMATCH);
}

static int fail_wildcard(void)
{
return fail_ssl_request("altname9.cert", CA_CERT, "localhost",
Expand Down Expand Up @@ -1501,7 +1473,6 @@ static int cert_identities(void)
{ "altname2.cert", "nohost.example.com" },
{ "altname4.cert", "localhost" },
{ "ca4.pem", "fourth.example.com" },
{ "altname8.cert", "http://nohost.example.com/" },
{ NULL, NULL }
};
int n;
Expand Down Expand Up @@ -1958,7 +1929,6 @@ ne_test tests[] = {
T(two_subject_altname2),
T(notdns_altname),
T(ipaddr_altname),
T(uri_altname),

T(multi_commonName),
T(commonName_first),
Expand All @@ -1971,7 +1941,6 @@ ne_test tests[] = {
T(fail_missing_CN),
T(fail_host_ipaltname),
T(fail_bad_ipaltname),
T(fail_bad_urialtname),
T(fail_wildcard),
T(fail_wildcard_ip),
T(fail_ca_notyetvalid),
Expand Down

0 comments on commit 7b7ee9d

Please sign in to comment.