Fix i2v_GENERAL_NAME to not assume NUL terminated strings

ASN.1 strings may not be NUL terminated. Don't assume they are.

CVE-2021-3712

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: David Benjamin <davidben@google.com>

diff --git a/crypto/x509/v3_san.c b/crypto/x509/v3_san.c
index ef9200cbaa9e6497a5e9facd1156866cfe93d48f..22cef0537076a916217d9611637d09289f95002b 100644 (file)
--- a/crypto/x509/v3_san.c
+++ b/crypto/x509/v3_san.c
@@ -9,6 +9,7 @@
 
 #include <stdio.h>
 #include "internal/cryptlib.h"
+#include "crypto/x509.h"
 #include <openssl/conf.h>
 #include <openssl/x509v3.h>
 #include <openssl/bio.h>
@@ -87,36 +88,41 @@ STACK_OF(CONF_VALUE) *i2v_GENERAL_NAME(X509V3_EXT_METHOD *method,
         switch (OBJ_obj2nid(gen->d.otherName->type_id)) {
         case NID_id_on_SmtpUTF8Mailbox:
             if (gen->d.otherName->value->type != V_ASN1_UTF8STRING
-                    || !X509V3_add_value_uchar("othername: SmtpUTF8Mailbox:",
+                    || !x509v3_add_len_value_uchar("othername: SmtpUTF8Mailbox:",
                             gen->d.otherName->value->value.utf8string->data,
+                            gen->d.otherName->value->value.utf8string->length,
                             &ret))
                 return NULL;
             break;
         case NID_XmppAddr:
             if (gen->d.otherName->value->type != V_ASN1_UTF8STRING
-                    || !X509V3_add_value_uchar("othername: XmppAddr:",
+                    || !x509v3_add_len_value_uchar("othername: XmppAddr:",
                             gen->d.otherName->value->value.utf8string->data,
+                            gen->d.otherName->value->value.utf8string->length,
                             &ret))
                 return NULL;
             break;
         case NID_SRVName:
             if (gen->d.otherName->value->type != V_ASN1_IA5STRING
-                    || !X509V3_add_value_uchar("othername: SRVName:",
+                    || !x509v3_add_len_value_uchar("othername: SRVName:",
                             gen->d.otherName->value->value.ia5string->data,
+                            gen->d.otherName->value->value.ia5string->length,
                             &ret))
                 return NULL;
             break;
         case NID_ms_upn:
             if (gen->d.otherName->value->type != V_ASN1_UTF8STRING
-                    || !X509V3_add_value_uchar("othername: UPN:",
+                    || !x509v3_add_len_value_uchar("othername: UPN:",
                             gen->d.otherName->value->value.utf8string->data,
+                            gen->d.otherName->value->value.utf8string->length,
                             &ret))
                 return NULL;
             break;
         case NID_NAIRealm:
             if (gen->d.otherName->value->type != V_ASN1_UTF8STRING
-                    || !X509V3_add_value_uchar("othername: NAIRealm:",
+                    || !x509v3_add_len_value_uchar("othername: NAIRealm:",
                             gen->d.otherName->value->value.utf8string->data,
+                            gen->d.otherName->value->value.utf8string->length,
                             &ret))
                 return NULL;
             break;
@@ -129,14 +135,16 @@ STACK_OF(CONF_VALUE) *i2v_GENERAL_NAME(X509V3_EXT_METHOD *method,
 
             /* check if the value is something printable */
             if (gen->d.otherName->value->type == V_ASN1_IA5STRING) {
-                if (X509V3_add_value_uchar(othername,
+                if (x509v3_add_len_value_uchar(othername,
                              gen->d.otherName->value->value.ia5string->data,
+                             gen->d.otherName->value->value.ia5string->length,
                              &ret)) 
                     return ret;
             }
             if (gen->d.otherName->value->type == V_ASN1_UTF8STRING) {
-                if (X509V3_add_value_uchar(othername,
+                if (x509v3_add_len_value_uchar(othername,
                              gen->d.otherName->value->value.utf8string->data,
+                             gen->d.otherName->value->value.utf8string->length,
                              &ret)) 
                     return ret;
             }
@@ -157,17 +165,20 @@ STACK_OF(CONF_VALUE) *i2v_GENERAL_NAME(X509V3_EXT_METHOD *method,
         break;
 
     case GEN_EMAIL:
-        if (!X509V3_add_value_uchar("email", gen->d.ia5->data, &ret))
+        if (!x509v3_add_len_value_uchar("email", gen->d.ia5->data,
+                                        gen->d.ia5->length, &ret))
             return NULL;
         break;
 
     case GEN_DNS:
-        if (!X509V3_add_value_uchar("DNS", gen->d.ia5->data, &ret))
+        if (!x509v3_add_len_value_uchar("DNS", gen->d.ia5->data,
+                                        gen->d.ia5->length, &ret))
             return NULL;
         break;
 
     case GEN_URI:
-        if (!X509V3_add_value_uchar("URI", gen->d.ia5->data, &ret))
+        if (!x509v3_add_len_value_uchar("URI", gen->d.ia5->data,
+                                        gen->d.ia5->length, &ret))
             return NULL;
         break;
 

diff --git a/crypto/x509/v3_utl.c b/crypto/x509/v3_utl.c
index 77d54213494b7b429e5a56dfd632ecc939f00966..4fd1f2cd6001ebbb1aa70a8903ff13c8b27401fb 100644 (file)
--- a/crypto/x509/v3_utl.c
+++ b/crypto/x509/v3_utl.c
@@ -12,6 +12,7 @@
 #include "e_os.h"
 #include "internal/cryptlib.h"
 #include <stdio.h>
+#include <string.h>
 #include "crypto/ctype.h"
 #include <openssl/conf.h>
 #include <openssl/crypto.h>
@@ -36,17 +37,23 @@ static int ipv6_hex(unsigned char *out, const char *in, int inlen);
 
 /* Add a CONF_VALUE name value pair to stack */
 
-int X509V3_add_value(const char *name, const char *value,
-                     STACK_OF(CONF_VALUE) **extlist)
+static int x509v3_add_len_value(const char *name, const char *value,
+                                size_t vallen, STACK_OF(CONF_VALUE) **extlist)
 {
     CONF_VALUE *vtmp = NULL;
     char *tname = NULL, *tvalue = NULL;
     int sk_allocated = (*extlist == NULL);
 
-    if (name && (tname = OPENSSL_strdup(name)) == NULL)
-        goto err;
-    if (value && (tvalue = OPENSSL_strdup(value)) == NULL)
+    if (name != NULL && (tname = OPENSSL_strdup(name)) == NULL)
         goto err;
+    if (value != NULL) {
+        /* We don't allow embeded NUL characters */
+        if (memchr(value, 0, vallen) != NULL)
+            goto err;
+        tvalue = OPENSSL_strndup(value, vallen);
+        if (tvalue == NULL)
+            goto err;
+    }
     if ((vtmp = OPENSSL_malloc(sizeof(*vtmp))) == NULL)
         goto err;
     if (sk_allocated && (*extlist = sk_CONF_VALUE_new_null()) == NULL)
@@ -69,10 +76,26 @@ int X509V3_add_value(const char *name, const char *value,
     return 0;
 }
 
+int X509V3_add_value(const char *name, const char *value,
+                     STACK_OF(CONF_VALUE) **extlist)
+{
+    return x509v3_add_len_value(name, value,
+                                value != NULL ? strlen((const char *)value) : 0,
+                                extlist);
+}
+
 int X509V3_add_value_uchar(const char *name, const unsigned char *value,
                            STACK_OF(CONF_VALUE) **extlist)
 {
-    return X509V3_add_value(name, (const char *)value, extlist);
+    return x509v3_add_len_value(name, (const char *)value,
+                                value != NULL ? strlen((const char *)value) : 0,
+                                extlist);
+}
+
+int x509v3_add_len_value_uchar(const char *name, const unsigned char *value,
+                               size_t vallen, STACK_OF(CONF_VALUE) **extlist)
+{
+    return x509v3_add_len_value(name, (const char *)value, vallen, extlist);
 }
 
 /* Free function for STACK_OF(CONF_VALUE) */

diff --git a/include/crypto/x509.h b/include/crypto/x509.h
index db83db0c923929b937b23fb69493599b23ddd1e0..599db841a7305db13e46374bb14747fe6ae9c894 100644 (file)
--- a/include/crypto/x509.h
+++ b/include/crypto/x509.h
@@ -361,3 +361,6 @@ int ossl_i2d_X448_PUBKEY(const ECX_KEY *a, unsigned char **pp);
 EVP_PKEY *ossl_d2i_PUBKEY_legacy(EVP_PKEY **a, const unsigned char **pp,
                                  long length);
 #endif
+
+int x509v3_add_len_value_uchar(const char *name, const unsigned char *value,
+                               size_t vallen, STACK_OF(CONF_VALUE) **extlist);