openssl and mbedtls x509 fixes (CVE-2017-7521 & -7522)

Hi,

This patch set contains the five commits that fix the X509 issues found by Guido in both our mbedtls and openssl code. It is based on the current master branch.

Compared to the previously sent patches, patch 4/5 ('Restrict --x509-alt-username extension types') was changed to include more user-friendly error reporting, and fix a typo in the man page. The other patches have only slightly tweaked commit messages and Changes.rst entries, mostly just mentioning the CVE ID.

-Steffan

asn1_buf_to_c_string() returned a literal string if the input ASN.1 string contained a NUL character, while the caller expects a mutable string. The caller will attempt to change this string, which allows a client to crash a server by sending a certificate with an embedded NUL character.

(The other way around is not interesting, as servers are allowed to stop a client by design.)

Impact analysis:
* applies to mbedtls builds only
* introduced in 2.4 (so 2.3 is not affected)
* can only be exploited if the --x509-track option is used
* requires the CA to sign a certificate with an embedded NUL in the certificate subject

This bug was discovered and reported to the OpenVPN security team by Guido Vranken.

---
 Changes.rst                      | 8 ++++++++
 src/openvpn/ssl_verify_mbedtls.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

index 94cba07..f1aed2d 100644
@@ -306,6 +306,14 @@ Maintainer-visible changes
 Version 2.4.3
 =============
 
+Security
+--------
+- CVE-2017-7522: Fix --x509-track post-authentication remote DoS
+  A client could crash a 2.4+ mbedtls server, if that server uses the
+  --x509-track option and the client has a correct, signed and unrevoked
+  certificate that contains an embedded NUL in the certificate subject.
+  Discovered and reported to the OpenVPN security team by Guido Vranken.
+
 User-visible Changes
 --------------------
 - ``--verify-hash`` can now take an optional flag which changes the hashing

index 2b7056c..d3b36dc 100644
@@ -271,7 +271,7 @@ asn1_buf_to_c_string(const mbedtls_asn1_buf *orig, struct gc_arena *gc)
     {
         if (orig->p[i] == '\0')
         {
-            return "ERROR: embedded null value";
+            return string_alloc("ERROR: embedded null value", gc);
         }
     }
     val = gc_malloc(orig->len+1, false, gc);

2.7.4

Your patch has been applied to the master and release/2.4 branch.

commit 426392940c7060300a10077c389f5156c790c2f6 (master) commit 67edada0beaf5ce6e47f13526b9f678dad4fc126 (release/2.4)

Author: Steffan Karger
Date:   Mon Jun 19 11:28:36 2017 +0200

     mbedtls: fix --x509-track post-authentication remote DoS (CVE-2017-7522)

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

In the --x509-username-field extenstion, we handle the subject string as if it is a C string. Make this assumption explicit and reject incomatible ASN.1 string types.

---
 src/openvpn/ssl_verify_mbedtls.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index d3b36dc..838c217 100644
@@ -267,6 +267,14 @@ asn1_buf_to_c_string(const mbedtls_asn1_buf *orig, struct gc_arena *gc)
     size_t i;
     char *val;
 
+    if (!(orig->tag == MBEDTLS_ASN1_UTF8_STRING
+          || orig->tag == MBEDTLS_ASN1_PRINTABLE_STRING
+          || orig->tag == MBEDTLS_ASN1_IA5_STRING))
+    {
+        /* Only support C-string compatible types */
+        return string_alloc("ERROR: unsupported ASN.1 string type", gc);
+    }
+
     for (i = 0; i < orig->len; ++i)
     {
         if (orig->p[i] == '\0')

2.7.4

Your patch has been applied to the master and release/2.4 branch.

commit 0007b2dbd12a83be3e4aeabc20550a5e16faf214 (master) commit 20f1a472031f0e8ad207ed96acc46ddf51616b5e (release/2.4)

Author: Steffan Karger
Date:   Mon Jun 19 11:28:37 2017 +0200

     mbedtls: require C-string compatible types for --x509-username-field

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Several of our OpenSSL-specific certificate-parsing code paths did not always clear all allocated memory. Since a client can cause a few bytes of memory to be leaked for each connection attempt, a client can cause a server to run out of memory and thereby kill the server. That makes this a (quite inefficient) DoS attack.

When using the --x509-alt-username option on openssl builds with an extension (argument prefixed with "ext:", e.g. "ext:subjectAltName"), the code would not free all allocated memory. Fix this by using the proper free function.

If ASN1_STRING_to_UTF8() returns 0, it didn't fail and *did* allocate memory. So also free the returned buffer if it returns 0.

These issues were found, analysed and reported to the OpenVPN team by Guido Vranken.

---
 Changes.rst                      | 5 +++++
 src/openvpn/ssl_verify_openssl.c | 9 ++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

index f1aed2d..89cfae8 100644
@@ -313,6 +313,11 @@ Security
   --x509-track option and the client has a correct, signed and unrevoked
   certificate that contains an embedded NUL in the certificate subject.
   Discovered and reported to the OpenVPN security team by Guido Vranken.
+- CVE-2017-7521: Fix post-authentication remote-triggerable memory leaks
+  A client could cause a server to leak a few bytes each time it connects to the
+  server.  That can eventuall cause the server to run out of memory, and thereby
+  causing the server process to terminate. Discovered and reported to the
+  OpenVPN security team by Guido Vranken.  (OpenSSL builds only.)
 
 User-visible Changes
 --------------------

index 31c1638..7c1a481 100644
@@ -163,7 +163,7 @@ extract_x509_extension(X509 *cert, char *fieldname, char *out, int size)
                     break;
             }
         }
-        sk_GENERAL_NAME_free(extensions);
+        GENERAL_NAMES_free(extensions);
     }
     return retval;
 }

@@ -225,8 +225,7 @@ extract_x509_field_ssl(X509_NAME *x509, const char *field_name, char *out,
     {
         return FAILURE;
     }
-    tmp = ASN1_STRING_to_UTF8(&buf, asn1);
-    if (tmp <= 0)
+    if (ASN1_STRING_to_UTF8(&buf, asn1) < 0)
     {
         return FAILURE;
     }

@@ -466,7 +465,7 @@ x509_setenv_track(const struct x509_track *xt, struct env_set *es, const int dep
                         {
                             ASN1_STRING *val = X509_NAME_ENTRY_get_data(ent);
                             unsigned char *buf = NULL;
-                            if (ASN1_STRING_to_UTF8(&buf, val) > 0)
+                            if (ASN1_STRING_to_UTF8(&buf, val) >= 0)
                             {
                                 do_setenv_x509(es, xt->name, (char *)buf, depth);
                                 OPENSSL_free(buf);

@@ -553,7 +552,7 @@ x509_setenv(struct env_set *es, int cert_depth, openvpn_x509_cert_t *peer_cert)
         {
             continue;
         }
-        if (ASN1_STRING_to_UTF8(&buf, val) <= 0)
+        if (ASN1_STRING_to_UTF8(&buf, val) < 0)
         {
             continue;
         }

2.7.4

Your patch has been applied to the master, release/2.4 and release/2.3 branch.

commit 2d032c7fcdfd692c851ea2fa858b4c2d9ea7d52d (master) commit 2341f716198fa90193e040b3fdb16959a47c6c27 (release/2.4) commit 84e1775961de1c9d2ab32159fc03f758591f5238 (release/2.3)

Author: Steffan Karger
Date:   Mon Jun 19 11:28:38 2017 +0200

     Fix remote-triggerable memory leaks (CVE-2017-7521)

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Acked-by: Guido Vranken <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

The code never supported all extension types. Make this explicit by only allowing subjectAltName and issuerAltName (for which the current code does work).

Using unsupported extension fields would most likely cause OpenVPN to crash as soon as a client connects. This does not have a real-world security impact, as such a configuration would not be possible to use in practice.

This bug was discovered, analysed and reported to the OpenVPN team by Guido Vranken.

---
 Changes.rst                      |  3 +++
 doc/openvpn.8                    |  2 ++
 src/openvpn/options.c            |  4 ++++
 src/openvpn/ssl_verify_backend.h |  8 ++++++++
 src/openvpn/ssl_verify_openssl.c | 19 ++++++++++++++++---
 5 files changed, 33 insertions(+), 3 deletions(-)

index 89cfae8..6fa1c0c 100644
@@ -324,6 +324,9 @@ User-visible Changes
 - ``--verify-hash`` can now take an optional flag which changes the hashing
   algorithm. It can be either SHA1 or SHA256.  The default if not provided is
   SHA1 to preserve backwards compatibility with existing configurations.
+- Restrict the supported --x509-alt-username extension fields to subjectAltName
+  and issuerAltName.  Other extensions probably didn't work anyway, and would
+  cause OpenVPN to crash when a client connects.
 
 Bugfixes
 --------

index 3f183e6..20bdd91 100644
@@ -5307,6 +5307,8 @@ option will match against the chosen
 .B fieldname
 instead of the Common Name.
 
+Only the subjectAltName and issuerAltName X.509 extensions are supported.
+
 .B Please note:
 This option has a feature which will convert an all-lowercase
 .B fieldname

index 76a8550..505c5b2 100644
@@ -8083,6 +8083,10 @@ add_option(struct options *options,
                     "configuration", p[1]);
             }
         }
+        else if (!x509_username_field_ext_supported(s+4))
+        {
+            msg(msglevel, "Unsupported x509-username-field extension: %s", s);
+        }
         options->x509_username_field = p[1];
     }
 #endif /* ENABLE_X509ALTUSERNAME */

index 978e54f..e8eaabe 100644
@@ -124,6 +124,14 @@ struct buffer x509_get_sha256_fingerprint(openvpn_x509_cert_t *cert,
 result_t backend_x509_get_username(char *common_name, int cn_len,
                                    char *x509_username_field, openvpn_x509_cert_t *peer_cert);
 
+#ifdef ENABLE_X509ALTUSERNAME
+/**
+ * Return true iff the supplied extension field is supported by the
+ * --x509-username-field option.
+ */
+bool x509_username_field_ext_supported(const char *extname);
+#endif
+
 /*
  * Return the certificate's serial number in decimal string representation.
  *

index 7c1a481..08451f2 100644
@@ -113,16 +113,29 @@ cleanup:
 }
 
 #ifdef ENABLE_X509ALTUSERNAME
+bool x509_username_field_ext_supported(const char *fieldname)
+{
+    int nid = OBJ_txt2nid(fieldname);
+    return nid == NID_subject_alt_name || nid == NID_issuer_alt_name;
+}
+
 static
 bool
 extract_x509_extension(X509 *cert, char *fieldname, char *out, int size)
 {
     bool retval = false;
     char *buf = 0;
-    GENERAL_NAMES *extensions;
-    int nid = OBJ_txt2nid(fieldname);
 
-    extensions = (GENERAL_NAMES *)X509_get_ext_d2i(cert, nid, NULL, NULL);
+    if (!x509_username_field_ext_supported(fieldname))
+    {
+        msg(D_TLS_ERRORS,
+            "ERROR: --x509-alt-username field 'ext:%s' not supported",
+            fieldname);
+        return false;
+    }
+
+    int nid = OBJ_txt2nid(fieldname);
+    GENERAL_NAMES *extensions = X509_get_ext_d2i(cert, nid, NULL, NULL);
     if (extensions)
     {
         int numalts;

2.7.4

Your patch has been applied to the master, release/2.4 and release/2.3 branch.

commit d2a19185fd78030ce4a1bba6c9f83e0dac9e15a6 (master) commit b72472baa5f228acf211542a7511f6960479f4c8 (release/2.4) commit a6dbec1cb481d6f0237372a7dec059f1c572b7b7 (release/2.3)

Author: Steffan Karger
Date:   Mon Jun 19 11:28:39 2017 +0200

     Restrict --x509-alt-username extension types

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Acked-by: Guido Vranken <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

We didn't check the return value of ASN1_STRING_to_UTF8() in extract_x509_extension(). Ignoring such a failure could result in buf being free'd twice. An error in ASN1_STRING_to_UTF8() can be caused remotely if the peer can make the local process run out of memory.

The problem can only be triggered for configurations that use the --x509-alt-username option with an x509 extension (i.e. the option parameter starts with "ext:").

This issue was discovered, analysed and reported to the OpenVPN team by Guido Vranken.

Extensive testing by Guido Vranken gives confidence that this function is very unlikely to fail in real-world usage (using subjectAltName or issuerAltName extensions) for other reasons than memory exhaustion.

---
 Changes.rst                      | 7 +++++++
 src/openvpn/ssl_verify_openssl.c | 5 ++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

index 6fa1c0c..726e591 100644
@@ -318,6 +318,13 @@ Security
   server.  That can eventuall cause the server to run out of memory, and thereby
   causing the server process to terminate. Discovered and reported to the
   OpenVPN security team by Guido Vranken.  (OpenSSL builds only.)
+- CVE-2017-7521: Fix a potential post-authentication remote code execution
+  attack on servers that use the ``--x509-alt-username`` option with an X.509
+  extension field (option argument prefixed with ``ext:``).  A client that can
+  cause a server to run out-of-memory (see above) might be able to cause the
+  server to double free, which in turn might lead to remote code execution.
+  Discovered and reported to the OpenVPN security team by Guido Vranken.
+  (OpenSSL builds only.)
 
 User-visible Changes
 --------------------

index 08451f2..f9e889f 100644
@@ -156,7 +156,10 @@ extract_x509_extension(X509 *cert, char *fieldname, char *out, int size)
             switch (name->type)
             {
                 case GEN_EMAIL:
-                    ASN1_STRING_to_UTF8((unsigned char **)&buf, name->d.ia5);
+                    if (ASN1_STRING_to_UTF8((unsigned char **)&buf, name->d.ia5) < 0)
+                    {
+                        continue;
+                    }
                     if (strlen(buf) != name->d.ia5->length)
                     {
                         msg(D_TLS_ERRORS, "ASN1 ERROR: string contained terminating zero");

2.7.4

Your patch has been applied to the master, release/2.4 and release/2.3 branch.

commit cb4e35ece4a5b70b10ef9013be3bff263d82f32b (master) commit 040084067119dd5a9e15eb3bcfc0079debaa3777 (release/2.4) commit 1dde0cd6e5e6a0f2f45ec9969b7ff1b6537514ad (release/2.3)

Author: Steffan Karger
Date:   Mon Jun 19 11:28:40 2017 +0200

     Fix potential double-free in --x509-alt-username (CVE-2017-7521)

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Acked-by: Guido Vranken <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering