Skip to contentSkip to conversation

Fix extract_x509_field_ssl for external objects

PATCH In review Hristo Venev 4 participants 6 messages Apr 20, 2017 – May 8, 2017

lore
Share & tools
lore

Hristo Venev

author
#0 lore raw

PATCH 1/1 Fix extract_x509_field_ssl for external objects

Only fields known to OpenSSL have a NID. OBJ_txt2obj allows specifying a numeric OID.

Signed-off-by Hristo Venev <[email protected]...>
---
 src/openvpn/ssl_verify_openssl.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
src/openvpn/ssl_verify_openssl.c +9 -2
index 87f71f21..114cf39c 100644
@@ -191,16 +191,23 @@ extract_x509_field_ssl(X509_NAME *x509, const char *field_name, char *out,
     X509_NAME_ENTRY *x509ne = 0;
     ASN1_STRING *asn1 = 0;
     unsigned char *buf = NULL;
-    int nid = OBJ_txt2nid(field_name);
+    ASN1_OBJECT *obj = OBJ_txt2obj(field_name, 0);
+
+    if (obj == NULL)
+    {
+        crypto_msg(M_FATAL, "Cannot get ASN1_OBJECT for %s", field_name);
+    }
 
     ASSERT(size > 0);
     *out = '\0';
     do
     {
         lastpos = tmp;
-        tmp = X509_NAME_get_index_by_NID(x509, nid, lastpos);
+        tmp = X509_NAME_get_index_by_OBJ(x509, obj, lastpos);
     } while (tmp > -1);
 
+    ASN1_OBJECT_free(obj);
+
     /* Nothing found */
     if (lastpos == -1)
     {
Signature · 1 line
2.12.2
#1 lore raw

Hi,

The change makes sense, so 'Feature-ACK', but one remark:

src/openvpn/ssl_verify_openssl.c extract_x509_field_ssl(X509_NAME *x509, const char *field_name, char *out, · line 199

Quoted 11 lines · Hristo Venev
src/openvpn/ssl_verify_openssl.c +6 -1
@@ -191,16 +191,23 @@ extract_x509_field_ssl(X509_NAME *x509, const char *field_name, char *out,
     X509_NAME_ENTRY *x509ne = 0;
     ASN1_STRING *asn1 = 0;
     unsigned char *buf = NULL;
-    int nid = OBJ_txt2nid(field_name);
+    ASN1_OBJECT *obj = OBJ_txt2obj(field_name, 0);
+
+    if (obj == NULL)
+    {
+        crypto_msg(M_FATAL, "Cannot get ASN1_OBJECT for %s", field_name);
+    }

M_FATAL is currently acceptable, because field_name always comes from the local config file, but I'd prefer to make this non-fatal, and return FAILURE. That ensures that if someone decides to use this function in a way where field_name *is* attacker controlled, we do not introduce a remote DoS.

As for the log level of this message: I think D_TLS_DEBUG_LOW (--verb 3) or possibly D_TLS_ERRORS (--verb 1) are appropriate.

-Steffan

#2 lore raw
Quoted 23 lines · Steffan Karger

Hi,

The change makes sense, so 'Feature-ACK', but one remark:

src/openvpn/ssl_verify_openssl.c extract_x509_field_ssl(X509_NAME *x509, const char *field_name, char *out, · line 199

Quoted 11 lines · Hristo Venev
src/openvpn/ssl_verify_openssl.c +6 -1
@@ -191,16 +191,23 @@ extract_x509_field_ssl(X509_NAME *x509, const char *field_name, char *out,
     X509_NAME_ENTRY *x509ne = 0;
     ASN1_STRING *asn1 = 0;
     unsigned char *buf = NULL;
-    int nid = OBJ_txt2nid(field_name);
+    ASN1_OBJECT *obj = OBJ_txt2obj(field_name, 0);
+
+    if (obj == NULL)
+    {
+        crypto_msg(M_FATAL, "Cannot get ASN1_OBJECT for %s", field_name);
+    }

M_FATAL is currently acceptable, because field_name always comes from the local config file, but I'd prefer to make this non-fatal, and return FAILURE. That ensures that if someone decides to use this function in a way where field_name *is* attacker controlled, we do not introduce a remote DoS.

Quoted 2 lines

As for the log level of this message: I think D_TLS_DEBUG_LOW (--verb 3) or possibly D_TLS_ERRORS (--verb 1) are appropriate.

Agreed! I'm leaning towards D_TLS_ERRORS over D_TLS_DEBUG_LOW. This field is normally configured via the configuration file, IIUC ( like --x509-username-field). So if this fails, it should have a very low threshold of warning the sys-admin. Normally, this shouldn't happen unless a client is truly miss-configured.

Signature · 4 lines
kind regards,

David Sommerseth
OpenVPN Technologies, Inc
author
#3 lore raw

Re: [Openvpn-devel] [PATCH] Fix extract_x509_field_ssl for external objects, v2

In reply to Hristo Venev

Only fields known to OpenSSL have a NID. OBJ_txt2obj allows specifying fields by numeric OID.

Signed-off-by Hristo Venev <[email protected]...>
---
 src/openvpn/ssl_verify_openssl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
src/openvpn/ssl_verify_openssl.c +10 -2
index 87f71f21..b6867c65 100644
@@ -191,16 +191,24 @@ extract_x509_field_ssl(X509_NAME *x509, const char *field_name, char *out,
     X509_NAME_ENTRY *x509ne = 0;
     ASN1_STRING *asn1 = 0;
     unsigned char *buf = NULL;
-    int nid = OBJ_txt2nid(field_name);
+    ASN1_OBJECT *field_name_obj = OBJ_txt2obj(field_name, 0);
+
+    if (field_name_obj == NULL)
+    {
+        msg(D_TLS_ERRORS, "Invalid X509 attribute name '%s'", field_name);
+        return FAILURE;
+    }
 
     ASSERT(size > 0);
     *out = '\0';
     do
     {
         lastpos = tmp;
-        tmp = X509_NAME_get_index_by_NID(x509, nid, lastpos);
+        tmp = X509_NAME_get_index_by_OBJ(x509, field_name_obj, lastpos);
     } while (tmp > -1);
 
+    ASN1_OBJECT_free(field_name_obj);
+
     /* Nothing found */
     if (lastpos == -1)
     {
Signature · 1 line
2.12.2
#4 lore raw

Re: [Openvpn-devel] [PATCH] Fix extract_x509_field_ssl for external objects, v2

Hi,

Quoted 30 lines · Hristo Venev

Only fields known to OpenSSL have a NID. OBJ_txt2obj allows specifying fields by numeric OID.

Signed-off-by Hristo Venev <[email protected]...>
---
 src/openvpn/ssl_verify_openssl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
src/openvpn/ssl_verify_openssl.c +7 -1
index 87f71f21..b6867c65 100644
@@ -191,16 +191,24 @@ extract_x509_field_ssl(X509_NAME *x509, const char *field_name, char *out,
     X509_NAME_ENTRY *x509ne = 0;
     ASN1_STRING *asn1 = 0;
     unsigned char *buf = NULL;
-    int nid = OBJ_txt2nid(field_name);
+    ASN1_OBJECT *field_name_obj = OBJ_txt2obj(field_name, 0);
+
+    if (field_name_obj == NULL)
+    {
+        msg(D_TLS_ERRORS, "Invalid X509 attribute name '%s'", field_name);
+        return FAILURE;
+    }
 
     ASSERT(size > 0);
     *out = '\0';
     do
     {

src/openvpn/ssl_verify_openssl.c extract_x509_field_ssl(X509_NAME *x509, const char *field_name, char *out, · line 214

Quoted 10 lines
src/openvpn/ssl_verify_openssl.c +3 -1
         lastpos = tmp;
-        tmp = X509_NAME_get_index_by_NID(x509, nid, lastpos);
+        tmp = X509_NAME_get_index_by_OBJ(x509, field_name_obj, lastpos);
     } while (tmp > -1);
 
+    ASN1_OBJECT_free(field_name_obj);
+
     /* Nothing found */
     if (lastpos == -1)
     {

Thanks for following up. ACK.

-Steffan

#5 lore raw

Re: [Openvpn-devel] [PATCH applied] Fix extract_x509_field_ssl for external objects, v2

In reply to Hristo Venev

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Your patch has been applied to the following branches

commit 69311687da55b8c0e6966b25c94c72494ea44e57 (master) commit f5d41cc68407f77f8fab41e242fa1d651c55c257 (release/2.4)

Author: Hristo Venev
Date:   Thu May 4 00:10:48 2017 +0100
     Fix extract_x509_field_ssl for external objects, v2

     Signed-off-by: Hristo Venev <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14535.html
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBCAAGBQJZEJsRAAoJEIbPlEyWcf3y3EMP/1CZyQm5kj/CmTe354EZhWTL RkRf6KE0oZLAKk9JqnlV7d93OgLTpyLtgj0ZcM75zIPnEAIMM+KZGkHcYeB7OmS5 WBoA5MxHm1QteBKexEBaFsrdzYNlGymZ+bpnhuL5+AqV998akOQVR6xVdtsoV+c0 ZdCqHWASd9cGvw6EwJGcAq5gdOj3w6+x3q0k8jN5DNZvfkr8tixStf283gBhOkzc d32XWPIHA1Vp5C/8/VTWWEx/4cvK1KK373QeZDwzww6KvhqISX+cUOu08i09DQ/1 S91RzV+54qxa2FO/lIyqS0MRv0lgXzXKKRXTXd8e6hhiyX6AA49ltOnJo6YbAMMk IiLv9YipEiSS9WzKu0LRuBMYRgHFH0+lHXi8+h99+0oaZpXI2fSc+ruvSVMbrcNB TpcOK5aJD0M7IbsjE2jSMpSUXqhhzYwc5H5iK7rwbv89mpCnaynv4nG4Rr4IKSYO BViX6KpZMEWGMoyhbV8UvwDYhkm5qibK/zLWzQQ5AdVxV+2JygvdBz2gwnynIdmB hFzYacWGRhZmtXICXLzDfidJOErnDV4NYzrj5DTvykZlL3QD3gpQO1CLSvgFUjoB UOYxPlsUIBFiy8hQXx2IR+gmSDl1Wc/s7Qte9kEtB3ql1OcgwwxcFBf1AzpySRyY EPKq4oEuBEWpvvcghIxI
=xoYg
-----END PGP SIGNATURE-----