Fix read length in ASN1_STRING_print_ex/to_UTF8. Refactor freeing logic in wolfSSL_X509_get0_extensions. Only read GENERAL_NAME types with strings in wolfSSL_X509V3_EXT_print. Properly handle NULLs inside of DNs in X509_NAME_oneline.

src/ssl_asn1.c

@@ -2897,8 +2897,9 @@ int wolfSSL_ASN1_STRING_to_UTF8(unsigned char **out, WOLFSSL_ASN1_STRING *asn1)
         }
     }
     if (len != -1) {
-        /* Copy in string - NUL always put on end of stored string. */
-        XMEMCPY(buf, data, (size_t)(len + 1));
+        /* Copy in string and NUL terminate - data may not be NUL terminated. */
+        XMEMCPY(buf, data, (size_t)len);
+        buf[len] = '\0';
         /* Return buffer. */
         *out = buf;
     }
@@ -3640,10 +3641,12 @@ static int wolfssl_asn1_string_print_esc_2253(WOLFSSL_BIO *bio,
     WOLFSSL_ASN1_STRING *str)
 {
     char* p;
+    int i;
     int str_len = 0;
 
-    /* Write all of string character by character. */
-    for (p = str->data; (*p) != '\0'; p++) {
+    /* Write all of string character by character.
+     * Use length as data is not necessarily NUL terminated. */
+    for (p = str->data, i = 0; i < str->length; p++, i++) {
         /* Check if character needs escaping. */
         if (wolfssl_check_esc_char(*p)){
             /* Update count of written characters. */

src/x509.c

@@ -406,12 +406,14 @@ static WOLFSSL_STACK* generateExtStack(const WOLFSSL_X509 *x)
 }
 
 /**
+ * The returned stack is owned by the X509 and stays valid until the X509 is
+ * freed or its DER encoding is replaced (e.g. by wolfSSL_X509_sign).
+ *
  * @param x Certificate to extract extensions from
  * @return STACK_OF(X509_EXTENSION)*
  */
 const WOLFSSL_STACK *wolfSSL_X509_get0_extensions(const WOLFSSL_X509 *x)
 {
-    int numOfExt;
     WOLFSSL_X509 *x509 = (WOLFSSL_X509*)x;
     WOLFSSL_ENTER("wolfSSL_X509_get0_extensions");
 
@@ -420,10 +422,9 @@ const WOLFSSL_STACK *wolfSSL_X509_get0_extensions(const WOLFSSL_X509 *x)
         return NULL;
     }
 
-    numOfExt = wolfSSL_X509_get_ext_count(x509);
-
-    if (numOfExt != wolfSSL_sk_num(x509->ext_sk_full)) {
-        wolfSSL_sk_pop_free(x509->ext_sk_full, NULL);
+    /* Only build the stack once. Freeing and rebuilding it here would leave
+     * the caller of a previous call holding a dangling pointer. */
+    if (x509->ext_sk_full == NULL) {
         x509->ext_sk_full = generateExtStack(x);
     }
 
@@ -1155,15 +1156,13 @@ WOLFSSL_X509_EXTENSION* wolfSSL_X509_set_ext(WOLFSSL_X509* x509, int loc)
         tmpIdx = idx + length;
 
         /* Get CRITICAL. If not present, defaults to false.
-         * It present, must be a valid TRUE */
+         * Accept any one byte value like the certificate parser does. */
         if ((tmpIdx < (word32)sz) &&
             (input[tmpIdx] == ASN_BOOLEAN))
         {
             if (((tmpIdx + 2) >= (word32)sz) ||
                 /* Check bool length */
-                (input[tmpIdx+1] != 1) ||
-                /* Assert true if CRITICAL present */
-                (input[tmpIdx+2] != 0xff))
+                (input[tmpIdx+1] != 1))
             {
                 WOLFSSL_MSG("Error decoding unknown extension data");
                 wolfSSL_X509_EXTENSION_free(ext);
@@ -1172,7 +1171,7 @@ WOLFSSL_X509_EXTENSION* wolfSSL_X509_set_ext(WOLFSSL_X509* x509, int loc)
                 return NULL;
             }
 
-            ext->crit = 1;
+            ext->crit = (input[tmpIdx+2] != 0);
             tmpIdx += 3;
         }
 
@@ -1189,25 +1188,24 @@ WOLFSSL_X509_EXTENSION* wolfSSL_X509_set_ext(WOLFSSL_X509* x509, int loc)
 
         tmpIdx++;
 
-        if (GetLength(input, &tmpIdx, &length, (word32)sz) <= 0) {
+        /* An empty OCTET STRING is accepted by the certificate parser. */
+        if (GetLength(input, &tmpIdx, &length, (word32)sz) < 0) {
             WOLFSSL_MSG("Error: Invalid Input Length.");
             wolfSSL_X509_EXTENSION_free(ext);
             FreeDecodedCert(cert);
             WC_FREE_VAR_EX(cert, NULL, DYNAMIC_TYPE_DCERT);
             return NULL;
         }
-        ext->value.data = (char*)XMALLOC(length, NULL,
-            DYNAMIC_TYPE_ASN1);
-        ext->value.isDynamic = 1;
-        if (ext->value.data == NULL) {
-            WOLFSSL_MSG("Failed to malloc ASN1_STRING data");
+        /* Copy with wolfSSL_ASN1_STRING_set() so the data is NUL terminated
+         * like every other ASN1_STRING. */
+        if (wolfSSL_ASN1_STRING_set(&ext->value, input + tmpIdx, length)
+                != 1) {
+            WOLFSSL_MSG("Failed to set ASN1_STRING data");
             wolfSSL_X509_EXTENSION_free(ext);
             FreeDecodedCert(cert);
             WC_FREE_VAR_EX(cert, NULL, DYNAMIC_TYPE_DCERT);
             return NULL;
         }
-        XMEMCPY(ext->value.data,input+tmpIdx,length);
-        ext->value.length = length;
 
         break; /* Got the Extension. Now exit while loop. */
 
@@ -1577,8 +1575,31 @@ int wolfSSL_X509V3_EXT_print(WOLFSSL_BIO *out, WOLFSSL_X509_EXTENSION *ext,
             sk = ext->ext_sk;
             while (sk != NULL) {
                 if (sk->type == STACK_TYPE_GEN_NAME && sk->data.gn) {
+                    /* Only the string backed GENERAL_NAME types hold a
+                     * WOLFSSL_ASN1_STRING in the union. otherName, dirName
+                     * and registeredID carry other objects there. */
+                    switch (sk->data.gn->type) {
+                        case WOLFSSL_GEN_EMAIL:
+                        case WOLFSSL_GEN_DNS:
+                        case WOLFSSL_GEN_URI:
+                        case WOLFSSL_GEN_IPADD:
+                        case WOLFSSL_GEN_IA5:
+                            break;
+                        default:
+                            WOLFSSL_MSG("Unsupported GENERAL_NAME type");
+                            sk = sk->next;
+                            continue;
+                    }
                     /* str is GENERAL_NAME for subject alternative name ext */
                     str = sk->data.gn->d.ia5;
+                    if (str == NULL) {
+                        /* Skip it like an unsupported type above: a name
+                         * built through the API can carry the type without
+                         * the string. */
+                        WOLFSSL_MSG("NULL GENERAL_NAME string");
+                        sk = sk->next;
+                        continue;
+                    }
                     len = str->length + 2; /* + 2 for NULL char and "," */
                     if (len > tmpSz) {
                         WOLFSSL_MSG("len greater than buffer size");
@@ -13130,6 +13151,12 @@ static int CertFromX509(Cert* cert, WOLFSSL_X509* x509)
 
         /* Put in the new certificate encoding into the x509 object. */
         FreeDer(&x509->derCert);
+    #if defined(WOLFSSL_QT) || defined(OPENSSL_ALL) || defined(OPENSSL_EXTRA)
+        /* Extension stack cached by wolfSSL_X509_get0_extensions() was built
+         * from the old encoding. */
+        wolfSSL_sk_pop_free(x509->ext_sk_full, NULL);
+        x509->ext_sk_full = NULL;
+    #endif
         type = CERT_TYPE;
     #ifdef WOLFSSL_CERT_REQ
         if (req) {
@@ -14688,6 +14715,47 @@ WOLFSSL_ASN1_OBJECT* wolfSSL_X509_NAME_ENTRY_get_object(
         * HAVE_STUNNEL || WOLFSSL_NGINX || HAVE_POCO_LIB || WOLFSSL_HAPROXY */
 
 #if defined(OPENSSL_EXTRA) || defined(OPENSSL_EXTRA_X509_SMALL)
+    /* Length of the escape a NUL byte is written as. */
+    #define NAME_NUL_ESC_SZ  4
+
+    /* Write a name entry value into a one line name, taking its length from
+     * the entry rather than from the data, and writing a NUL byte as "\x00"
+     * as OpenSSL's X509_NAME_oneline() does. Treating the value as a C string
+     * drops everything from a NUL on and lets the rest of the name follow it,
+     * so two different names can read the same.
+     *
+     * out   - buffer for the value, NULL to only get the length
+     * outSz - space available in out, ignored when out is NULL
+     *
+     * Returns the number of bytes written, or needed when out is NULL.
+     * Returns BUFFER_E when the value does not fit or the length would
+     * overflow.
+     */
+    static int AddEntryValue(char* out, int outSz, const unsigned char* val,
+            int valSz)
+    {
+        int i;
+        int sz = 0;
+
+        for (i = 0; (val != NULL) && (i < valSz); i++) {
+            int need = (val[i] == '\0') ? NAME_NUL_ESC_SZ : 1;
+
+            if (need > INT_MAX - sz)
+                return BUFFER_E;
+            if (out != NULL) {
+                if (need > outSz - sz)
+                    return BUFFER_E;
+                if (need == 1)
+                    out[sz] = (char)val[i];
+                else
+                    XMEMCPY(out + sz, "\\x00", NAME_NUL_ESC_SZ);
+            }
+            sz += need;
+        }
+
+        return sz;
+    }
+
     /* add all entry of type "nid" to the buffer "fullName" and advance "idx"
      * since number of entries is small, a brute force search is used here
      * returns the number of entries added
@@ -14720,15 +14788,16 @@ WOLFSSL_ASN1_OBJECT* wolfSSL_X509_NAME_ENTRY_get_object(
 
                 data = wolfSSL_ASN1_STRING_data(e->value);
                 if (data != NULL) {
-                    sz = (int)XSTRLEN((const char*)data);
-                    XMEMCPY(fullName + *idx, data, sz);
+                    sz = AddEntryValue(fullName + *idx, fullNameSz - *idx,
+                        data, wolfSSL_ASN1_STRING_length(e->value));
+                    if (sz < 0)
+                        return sz;
                     *idx += sz;
                 }
 
                 ret++;
             }
         }
-        (void)fullNameSz;
         return ret;
     }
 
@@ -14737,7 +14806,7 @@ WOLFSSL_ASN1_OBJECT* wolfSSL_X509_NAME_ENTRY_get_object(
      * returns 0 on success */
     static int RebuildFullName(WOLFSSL_X509_NAME* name)
     {
-        int totalLen = 0, i, idx, entryCount = 0;
+        int totalLen = 0, i, idx, entryCount = 0, valLen;
 
         if (name == NULL)
             return BAD_FUNC_ARG;
@@ -14752,8 +14821,20 @@ WOLFSSL_ASN1_OBJECT* wolfSSL_X509_NAME_ENTRY_get_object(
                 if (obj == NULL)
                     return BAD_FUNC_ARG;
 
-                totalLen += (int)XSTRLEN(obj->sName) + 2;/*+2 for '/' and '=' */
-                totalLen += wolfSSL_ASN1_STRING_length(e->value);
+                /* Each addition is checked against the room left below
+                 * INT_MAX - 1, so that totalLen + 1 (the NUL added later)
+                 * stays a positive int. */
+                valLen = (int)XSTRLEN(obj->sName) + 2;/*+2 for '/' and '=' */
+                if (valLen > INT_MAX - 1 - totalLen)
+                    return BUFFER_E;
+                totalLen += valLen;
+
+                valLen = AddEntryValue(NULL, 0,
+                    wolfSSL_ASN1_STRING_data(e->value),
+                    wolfSSL_ASN1_STRING_length(e->value));
+                if ((valLen < 0) || (valLen > INT_MAX - 1 - totalLen))
+                    return BUFFER_E;
+                totalLen += valLen;
             }
         }
 
@@ -17303,6 +17384,10 @@ static int regenX509REQDerBuffer(WOLFSSL_X509* x509)
 
     if (wolfssl_x509_make_der(x509, 1, der, &derSz, 0) == WOLFSSL_SUCCESS) {
         FreeDer(&x509->derCert);
+        /* Extension stack cached by wolfSSL_X509_get0_extensions() was built
+         * from the old encoding. */
+        wolfSSL_sk_pop_free(x509->ext_sk_full, NULL);
+        x509->ext_sk_full = NULL;
         if (AllocDer(&x509->derCert, (word32)derSz, CERT_TYPE,
                                                              x509->heap) == 0) {
             XMEMCPY(x509->derCert->buffer, der, derSz);

tests/api/test_ossl_asn1.c

@@ -1505,8 +1505,9 @@ int test_wolfSSL_ASN1_STRING_print_ex(void)
     /* RFC2253 Escape */
     XMEMSET(rbuf, 0, 255);
     flags = ASN1_STRFLGS_ESC_2253;
-    ExpectIntEQ(p_len = wolfSSL_ASN1_STRING_print_ex(bio, esc_str, flags), 9);
-    ExpectIntEQ(BIO_read(bio, (void*)rbuf, 9), 9);
+    /* 6 bytes of data (incl. NUL) plus 4 escape characters. */
+    ExpectIntEQ(p_len = wolfSSL_ASN1_STRING_print_ex(bio, esc_str, flags), 10);
+    ExpectIntEQ(BIO_read(bio, (void*)rbuf, 10), 10);
     ExpectStrEQ((char*)rbuf, "a\\+\\;\\<\\>");
     ExpectIntEQ(BIO_set_write_buf_size(fixed, 1), 1);
     /* Ensure there is 0 bytes available to write into. */