poison for X509_VERIFY_PARAM's

Tighten up checks for various X509_VERIFY_PARAM functions, and
allow for the verify param to be poisoned (preculding future
successful cert validation) if the setting of host, ip, or email
for certificate validation fails. (since many callers do not
check the return code in the wild and blunder along anyway)

Inspired by some discussions with Adam Langley.

ok jsing@

4 files changed, 107 insertions, 45 deletions

diff --git a/src/lib/libcrypto/man/X509_VERIFY_PARAM_set_flags.3 b/src/lib/libcrypto/man/X509_VERIFY_PARAM_set_flags.3
index 4f3261c975..9c0150700d 100644
--- a/src/lib/libcrypto/man/X509_VERIFY_PARAM_set_flags.3
+++ b/src/lib/libcrypto/man/X509_VERIFY_PARAM_set_flags.3
@@ -1,4 +1,4 @@
-.\" $OpenBSD: X509_VERIFY_PARAM_set_flags.3,v 1.12 2018/03/23 14:26:40 schwarze Exp $
+.\" $OpenBSD: X509_VERIFY_PARAM_set_flags.3,v 1.13 2018/04/06 07:08:20 beck Exp $
 .\" full merge up to: OpenSSL d33def66 Feb 9 14:17:13 2016 -0500
 .\" selective merge up to: OpenSSL 48e5119a Jan 19 10:49:22 2018 +0100
 .\"
@@ -68,7 +68,7 @@
 .\" ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
 .\" OF THE POSSIBILITY OF SUCH DAMAGE.
 .\"
-.Dd $Mdocdate: March 23 2018 $
+.Dd $Mdocdate: April 6 2018 $
 .Dt X509_VERIFY_PARAM_SET_FLAGS 3
 .Os
 .Sh NAME
@@ -344,14 +344,14 @@ is
 .Dv NULL
 or empty, the list of hostnames is cleared, and name checks are not
 performed on the peer certificate.
-If
+.Fa namelen
+should be set to the length of
+.Fa name .
+For historical compatibility, if 
 .Fa name
 is NUL-terminated,
 .Fa namelen
-may be zero, otherwise
-.Fa namelen
-must be set to the length of
-.Fa name .
+may be specified as zero.
 When a hostname is specified, certificate verification automatically
 invokes
 .Xr X509_check_host 3
@@ -360,6 +360,10 @@ with flags equal to the
 argument given to
 .Fn X509_VERIFY_PARAM_set_hostflags
 (default zero).
+.Fn X509_VERIFY_PARAM_set1_host
+will fail if
+.Fa name
+contains any embedded 0 bytes.
 .Pp
 .Fn X509_VERIFY_PARAM_add1_host
 adds
@@ -376,6 +380,18 @@ No change is made if
 is
 .Dv NULL
 or empty.
+.Fa namelen
+should be set to the length of
+.Fa name .
+For historical compatibility, if 
+.Fa name
+is NUL-terminated,
+.Fa namelen
+may be specified as zero.
+.Fn X509_VERIFY_PARAM_add1_host
+will fail if
+.Fa name
+contains any embedded 0 bytes.
 When multiple names are configured, the peer is considered verified when
 any name matches.
 .Pp
@@ -390,14 +406,18 @@ identifier respectively.
 .Fn X509_VERIFY_PARAM_set1_email
 sets the expected RFC822 email address to
 .Fa email .
-If
+.Fa emaillen
+should be set to the length of
+.Fa email .
+For historical compatibility, if 
 .Fa email
 is NUL-terminated,
 .Fa emaillen
-may be zero, otherwise
-.Fa emaillen
-must be set to the length of
-.Fa email .
+may be specified as zero,
+.Fn X509_VERIFY_PARAM_set1_email
+will fail if
+.Fa email
+is NULL, an empty string, or contains embedded 0 bytes.
 When an email address is specified, certificate verification
 automatically invokes
 .Xr X509_check_email 3 .
@@ -410,6 +430,12 @@ The
 argument is in binary format, in network byte-order, and
 .Fa iplen
 must be set to 4 for IPv4 and 16 for IPv6.
+.Fn X509_VERIFY_PARAM_set1_ip
+will fail if 
+.Fa ip
+is NULL or if
+.Fa iplen
+is not 4 or 16.
 When an IP address is specified,
 certificate verification automatically invokes
 .Xr X509_check_ip 3 .
@@ -422,6 +448,10 @@ The
 argument is a NUL-terminal ASCII string:
 dotted decimal quad for IPv4 and colon-separated hexadecimal for IPv6.
 The condensed "::" notation is supported for IPv6 addresses.
+.Fn X509_VERIFY_PARAM_set1_ip_asc
+will fail if
+.Fa ipasc
+is unparsable.
 .Pp
 .Fn X509_VERIFY_PARAM_add0_table
 adds
@@ -476,14 +506,23 @@ on allocation failure.
 .Fn X509_VERIFY_PARAM_set_trust ,
 .Fn X509_VERIFY_PARAM_add0_policy ,
 .Fn X509_VERIFY_PARAM_set1_policies ,
+and
+.Fn X509_VERIFY_PARAM_add0_table
+return 1 for success or 0 for failure.
+.Pp
 .Fn X509_VERIFY_PARAM_set1_host ,
 .Fn X509_VERIFY_PARAM_add1_host ,
 .Fn X509_VERIFY_PARAM_set1_email ,
 .Fn X509_VERIFY_PARAM_set1_ip ,
-.Fn X509_VERIFY_PARAM_set1_ip_asc ,
 and
-.Fn X509_VERIFY_PARAM_add0_table
+.Fn X509_VERIFY_PARAM_set1_ip_asc ,
 return 1 for success or 0 for failure.
+A failure from these routines will poison
+the
+.Vt X509_VERIFY_PARAM
+object so that future calls to
+.Xr X509_verify_cert
+using the poisoned object will fail. 
 .Pp
 .Fn X509_VERIFY_PARAM_get_flags
 returns the current verification flags.
diff --git a/src/lib/libcrypto/x509/vpm_int.h b/src/lib/libcrypto/x509/vpm_int.h
index 6c8061c847..7fc9fef761 100644
--- a/src/lib/libcrypto/x509/vpm_int.h
+++ b/src/lib/libcrypto/x509/vpm_int.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: vpm_int.h,v 1.3 2016/12/21 15:49:29 jsing Exp $ */
+/* $OpenBSD: vpm_int.h,v 1.4 2018/04/06 07:08:20 beck Exp $ */
 /*
  * Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL project
  * 2013.
@@ -69,6 +69,7 @@ struct X509_VERIFY_PARAM_ID_st {
         size_t emaillen;
         unsigned char *ip;          /* If not NULL IP address to match */
         size_t iplen;               /* Length of IP address */
+        int poisoned;
 };
 
 __END_HIDDEN_DECLS
diff --git a/src/lib/libcrypto/x509/x509_vfy.c b/src/lib/libcrypto/x509/x509_vfy.c
index c8ccae5029..8392f509e7 100644
--- a/src/lib/libcrypto/x509/x509_vfy.c
+++ b/src/lib/libcrypto/x509/x509_vfy.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_vfy.c,v 1.68 2018/02/22 17:11:30 jsing Exp $ */
+/* $OpenBSD: x509_vfy.c,v 1.69 2018/04/06 07:08:20 beck Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -182,10 +182,13 @@ check_id_error(X509_STORE_CTX *ctx, int errcode)
 static int
 check_hosts(X509 *x, X509_VERIFY_PARAM_ID *id)
 {
-        size_t i;
-        size_t n = sk_OPENSSL_STRING_num(id->hosts);
+        size_t i, n;
         char *name;
 
+        if (id->poisoned)
+                return 0;
+
+        n = sk_OPENSSL_STRING_num(id->hosts);
         free(id->peername);
         id->peername = NULL;
 
@@ -205,6 +208,10 @@ check_id(X509_STORE_CTX *ctx)
         X509_VERIFY_PARAM_ID *id = vpm->id;
         X509 *x = ctx->cert;
 
+        if (id->poisoned)
+                if (!check_id_error(ctx, X509_V_ERR_INVALID_CALL))
+                        return 0;
+
         if (id->hosts && check_hosts(x, id) <= 0) {
                 if (!check_id_error(ctx, X509_V_ERR_HOSTNAME_MISMATCH))
                         return 0;
diff --git a/src/lib/libcrypto/x509/x509_vpm.c b/src/lib/libcrypto/x509/x509_vpm.c
index 0897137697..baebcf7bca 100644
--- a/src/lib/libcrypto/x509/x509_vpm.c
+++ b/src/lib/libcrypto/x509/x509_vpm.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_vpm.c,v 1.17 2018/03/22 15:54:46 beck Exp $ */
+/* $OpenBSD: x509_vpm.c,v 1.18 2018/04/06 07:08:20 beck Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2004.
  */
@@ -125,7 +125,7 @@ sk_deep_copy(void *sk_void, void *copy_func_void, void *free_func_void)
 }
 
 static int
-int_x509_param_set_hosts(X509_VERIFY_PARAM_ID *id, int mode,
+x509_param_set_hosts_internal(X509_VERIFY_PARAM_ID *id, int mode,
     const char *name, size_t namelen)
 {
         char *copy;
@@ -134,7 +134,6 @@ int_x509_param_set_hosts(X509_VERIFY_PARAM_ID *id, int mode,
                 namelen = strlen(name);
         /*
          * Refuse names with embedded NUL bytes.
-         * XXX: Do we need to push an error onto the error stack?
          */
         if (name && memchr(name, '\0', namelen))
                 return 0;
@@ -197,6 +196,7 @@ x509_verify_param_zero(X509_VERIFY_PARAM *param)
         free(paramid->ip);
         paramid->ip = NULL;
         paramid->iplen = 0;
+        paramid->poisoned = 0;
 }
 
 X509_VERIFY_PARAM *
@@ -367,24 +367,28 @@ X509_VERIFY_PARAM_set1(X509_VERIFY_PARAM *to, const X509_VERIFY_PARAM *from)
 }
 
 static int
-int_x509_param_set1(char **pdest, size_t *pdestlen,  const char *src,
-    size_t srclen)
+x509_param_set1_internal(char **pdest, size_t *pdestlen,  const char *src,
+    size_t srclen, int nonul)
 {
         char *tmp;
-        if (src) {
-                if (srclen == 0) {
-                        if ((tmp = strdup(src)) == NULL)
-                                return 0;
-                        srclen = strlen(src);
-                } else {
-                        if ((tmp = malloc(srclen)) == NULL)
-                                return 0;
-                        memcpy(tmp, src, srclen);
-                }
+
+        if (src == NULL)
+                return 0;
+
+        if (srclen == 0) {
+                srclen = strlen(src);
+                if (srclen == 0)
+                        return 0;
+                if ((tmp = strdup(src)) == NULL)
+                        return 0;
         } else {
-                tmp = NULL;
-                srclen = 0;
+                if (nonul && memchr(src, '\0', srclen))
+                        return 0;
+                if ((tmp = malloc(srclen)) == NULL)
+                        return 0;
+                memcpy(tmp, src, srclen);
         }
+
         if (*pdest)
                 free(*pdest);
         *pdest = tmp;
@@ -505,14 +509,20 @@ int
 X509_VERIFY_PARAM_set1_host(X509_VERIFY_PARAM *param,
     const char *name, size_t namelen)
 {
-        return int_x509_param_set_hosts(param->id, SET_HOST, name, namelen);
+        if (x509_param_set_hosts_internal(param->id, SET_HOST, name, namelen))
+                return 1;
+        param->id->poisoned = 1;
+        return 0;
 }
 
 int
 X509_VERIFY_PARAM_add1_host(X509_VERIFY_PARAM *param,
     const char *name, size_t namelen)
 {
-        return int_x509_param_set_hosts(param->id, ADD_HOST, name, namelen);
+        if (x509_param_set_hosts_internal(param->id, ADD_HOST, name, namelen))
+                return 1;
+        param->id->poisoned = 1;
+        return 0;
 }
 
 void
@@ -531,18 +541,25 @@ int
 X509_VERIFY_PARAM_set1_email(X509_VERIFY_PARAM *param,  const char *email,
     size_t emaillen)
 {
-        return int_x509_param_set1(&param->id->email, &param->id->emaillen,
-            email, emaillen);
+        if (x509_param_set1_internal(&param->id->email, &param->id->emaillen,
+            email, emaillen, 1))
+                return 1;
+        param->id->poisoned = 1;
+        return 0;
 }
 
 int
 X509_VERIFY_PARAM_set1_ip(X509_VERIFY_PARAM *param, const unsigned char *ip,
     size_t iplen)
 {
-        if (iplen != 0 && iplen != 4 && iplen != 16)
-                return 0;
-        return int_x509_param_set1((char **)&param->id->ip, &param->id->iplen,
-            (char *)ip, iplen);
+        if (iplen != 4 && iplen != 16)
+                goto err;
+        if (x509_param_set1_internal((char **)&param->id->ip, &param->id->iplen,
+                (char *)ip, iplen, 0))
+                return 1;
+ err:
+        param->id->poisoned = 1;
+        return 0;
 }
 
 int
@@ -552,8 +569,6 @@ X509_VERIFY_PARAM_set1_ip_asc(X509_VERIFY_PARAM *param, const char *ipasc)
         size_t iplen;
 
         iplen = (size_t)a2i_ipadd(ipout, ipasc);
-        if (iplen == 0)
-                return 0;
         return X509_VERIFY_PARAM_set1_ip(param, ipout, iplen);
 }