Blob Blame Raw
From bc9ae5a810b8024e7ab1179f492c425793e0ddcf Mon Sep 17 00:00:00 2001
From: Mark Reynolds <mreynolds@redhat.com>
Date: Fri, 7 Jun 2019 09:21:31 -0400
Subject: [PATCH] Issue 50426 - nsSSL3Ciphers is limited to 1024 characters

Bug Description:  There was a hardcoded buffer for processing TLS ciphers.
                  Anything over 1024 characters was truncated and was not
                  applied.

Fix Description:  Don't use a fixed size buffer and just use the entire
                  string.  When printing errors about invalid format then
                  we must use a fixed sized buffer, but we will truncate
                  that log value as to not exceed the ssl logging function's
                  buffer, and still output a useful message.

ASAN approved

https://pagure.io/389-ds-base/issue/50426

Reviewed by: firstyear, tbordaz, and spichugi (Thanks!!!)

(cherry picked from commit 22f2f9a1502e63bb169b7d599b5a3b35ddb31b8a)
---
 dirsrvtests/tests/suites/tls/cipher_test.py | 51 +++++++++++++++++++++
 ldap/servers/slapd/ssl.c                    | 34 ++++++--------
 2 files changed, 66 insertions(+), 19 deletions(-)
 create mode 100644 dirsrvtests/tests/suites/tls/cipher_test.py

diff --git a/dirsrvtests/tests/suites/tls/cipher_test.py b/dirsrvtests/tests/suites/tls/cipher_test.py
new file mode 100644
index 000000000..058931046
--- /dev/null
+++ b/dirsrvtests/tests/suites/tls/cipher_test.py
@@ -0,0 +1,51 @@
+import pytest
+import os
+from lib389.config import Encryption
+from lib389.topologies import topology_st as topo
+
+
+def test_long_cipher_list(topo):
+    """Test a long cipher list, and makre sure it is not truncated
+
+    :id: bc400f54-3966-49c8-b640-abbf4fb2377d
+    :setup: Standalone Instance
+    :steps:
+        1. Set nsSSL3Ciphers to a very long list of ciphers
+        2. Ciphers are applied correctly
+    :expectedresults:
+        1. Success
+        2. Success
+    """
+    ENABLED_CIPHER = "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384::AES-GCM::AEAD::256"
+    DISABLED_CIPHER = "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256::AES-GCM::AEAD::128"
+    CIPHER_LIST = (
+            "-all,-SSL_CK_RC4_128_WITH_MD5,-SSL_CK_RC4_128_EXPORT40_WITH_MD5,-SSL_CK_RC2_128_CBC_WITH_MD5,"
+            "-SSL_CK_RC2_128_CBC_EXPORT40_WITH_MD5,-SSL_CK_DES_64_CBC_WITH_MD5,-SSL_CK_DES_192_EDE3_CBC_WITH_MD5,"
+            "-TLS_RSA_WITH_RC4_128_MD5,-TLS_RSA_WITH_RC4_128_SHA,-TLS_RSA_WITH_3DES_EDE_CBC_SHA,"
+            "-TLS_RSA_WITH_DES_CBC_SHA,-SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA,-SSL_RSA_FIPS_WITH_DES_CBC_SHA,"
+            "-TLS_RSA_EXPORT_WITH_RC4_40_MD5,-TLS_RSA_EXPORT_WITH_RC2_CBC_40_MD5,-TLS_RSA_WITH_NULL_MD5,"
+            "-TLS_RSA_WITH_NULL_SHA,-TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA,-SSL_FORTEZZA_DMS_WITH_FORTEZZA_CBC_SHA,"
+            "-SSL_FORTEZZA_DMS_WITH_RC4_128_SHA,-SSL_FORTEZZA_DMS_WITH_NULL_SHA,-TLS_DHE_DSS_WITH_DES_CBC_SHA,"
+            "-TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA,-TLS_DHE_RSA_WITH_DES_CBC_SHA,-TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA,"
+            "+TLS_RSA_WITH_AES_128_CBC_SHA,-TLS_DHE_DSS_WITH_AES_128_CBC_SHA,-TLS_DHE_RSA_WITH_AES_128_CBC_SHA,"
+            "+TLS_RSA_WITH_AES_256_CBC_SHA,-TLS_DHE_DSS_WITH_AES_256_CBC_SHA,-TLS_DHE_RSA_WITH_AES_256_CBC_SHA,"
+            "-TLS_RSA_EXPORT1024_WITH_RC4_56_SHA,-TLS_DHE_DSS_WITH_RC4_128_SHA,-TLS_ECDHE_RSA_WITH_RC4_128_SHA,"
+            "-TLS_RSA_WITH_NULL_SHA,-TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA,-SSL_CK_DES_192_EDE3_CBC_WITH_MD5,"
+            "-TLS_RSA_WITH_RC4_128_MD5,-TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,-TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,"
+            "-TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,+TLS_AES_128_GCM_SHA256,+TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384"
+        )
+
+    topo.standalone.enable_tls()
+    enc = Encryption(topo.standalone)
+    enc.set('nsSSL3Ciphers', CIPHER_LIST)
+    topo.standalone.restart()
+    enabled_ciphers = enc.get_attr_vals_utf8('nssslenabledciphers')
+    assert ENABLED_CIPHER in enabled_ciphers
+    assert DISABLED_CIPHER not in enabled_ciphers
+
+
+if __name__ == '__main__':
+    # Run isolated
+    # -s for DEBUG mode
+    CURRENT_FILE = os.path.realpath(__file__)
+    pytest.main(["-s", CURRENT_FILE])
diff --git a/ldap/servers/slapd/ssl.c b/ldap/servers/slapd/ssl.c
index b8eba2da4..ed054db44 100644
--- a/ldap/servers/slapd/ssl.c
+++ b/ldap/servers/slapd/ssl.c
@@ -95,7 +95,6 @@ static char *configDN = "cn=encryption,cn=config";
 #define CIPHER_SET_ALLOWWEAKDHPARAM 0x200    /* allowWeakDhParam is on */
 #define CIPHER_SET_DISALLOWWEAKDHPARAM 0x400 /* allowWeakDhParam is off */
 
-
 #define CIPHER_SET_ISDEFAULT(flag) \
     (((flag)&CIPHER_SET_DEFAULT) ? PR_TRUE : PR_FALSE)
 #define CIPHER_SET_ISALL(flag) \
@@ -689,10 +688,12 @@ _conf_setciphers(char *setciphers, int flags)
             active = 0;
             break;
         default:
-            PR_snprintf(err, sizeof(err), "invalid ciphers <%s>: format is "
-                                          "+cipher1,-cipher2...",
-                        raw);
-            return slapi_ch_strdup(err);
+            if (strlen(raw) > MAGNUS_ERROR_LEN) {
+                PR_snprintf(err, sizeof(err) - 3, "%s...", raw);
+                return slapi_ch_smprintf("invalid ciphers <%s>: format is +cipher1,-cipher2...", err);
+            } else {
+                return slapi_ch_smprintf("invalid ciphers <%s>: format is +cipher1,-cipher2...", raw);
+            }
         }
         if ((t = strchr(setciphers, ',')))
             *t++ = '\0';
@@ -1689,7 +1690,6 @@ slapd_ssl_init2(PRFileDesc **fd, int startTLS)
     PRUint16 NSSVersionMax = enabledNSSVersions.max;
     char mymin[VERSION_STR_LENGTH], mymax[VERSION_STR_LENGTH];
     char newmax[VERSION_STR_LENGTH];
-    char cipher_string[1024];
     int allowweakcipher = CIPHER_SET_DEFAULTWEAKCIPHER;
     int_fast16_t renegotiation = (int_fast16_t)SSL_RENEGOTIATE_REQUIRES_XTN;
 
@@ -1730,21 +1730,17 @@ slapd_ssl_init2(PRFileDesc **fd, int startTLS)
                            "Ignoring it and set it to default.", val, configDN);
         }
     }
-    slapi_ch_free((void **)&val);
+    slapi_ch_free_string(&val);
 
     /* Set SSL cipher preferences */
-    *cipher_string = 0;
-    if (ciphers && (*ciphers) && PL_strcmp(ciphers, "blank"))
-        PL_strncpyz(cipher_string, ciphers, sizeof(cipher_string));
-    slapi_ch_free((void **)&ciphers);
-
-    if (NULL != (val = _conf_setciphers(cipher_string, allowweakcipher))) {
+    if (NULL != (val = _conf_setciphers(ciphers, allowweakcipher))) {
         errorCode = PR_GetError();
         slapd_SSL_warn("Failed to set SSL cipher "
                        "preference information: %s (" SLAPI_COMPONENT_NAME_NSPR " error %d - %s)",
                        val, errorCode, slapd_pr_strerror(errorCode));
-        slapi_ch_free((void **)&val);
+        slapi_ch_free_string(&val);
     }
+    slapi_ch_free_string(&ciphers);
     freeConfigEntry(&e);
 
     /* Import pr fd into SSL */
@@ -1815,12 +1811,12 @@ slapd_ssl_init2(PRFileDesc **fd, int startTLS)
             activation = slapi_entry_attr_get_charptr(e, "nssslactivation");
             if ((!activation) || (!PL_strcasecmp(activation, "off"))) {
                 /* this family was turned off, goto next */
-                slapi_ch_free((void **)&activation);
+                slapi_ch_free_string(&activation);
                 freeConfigEntry(&e);
                 continue;
             }
 
-            slapi_ch_free((void **)&activation);
+            slapi_ch_free_string(&activation);
 
             token = slapi_entry_attr_get_charptr(e, "nsssltoken");
             personality = slapi_entry_attr_get_charptr(e, "nssslpersonalityssl");
@@ -1837,8 +1833,8 @@ slapd_ssl_init2(PRFileDesc **fd, int startTLS)
                                "family information. Missing nsssltoken or"
                                "nssslpersonalityssl in %s (" SLAPI_COMPONENT_NAME_NSPR " error %d - %s)",
                                *family, errorCode, slapd_pr_strerror(errorCode));
-                slapi_ch_free((void **)&token);
-                slapi_ch_free((void **)&personality);
+                slapi_ch_free_string(&token);
+                slapi_ch_free_string(&personality);
                 freeConfigEntry(&e);
                 continue;
             }
@@ -1865,7 +1861,7 @@ slapd_ssl_init2(PRFileDesc **fd, int startTLS)
                                "private key for cert %s of family %s (" SLAPI_COMPONENT_NAME_NSPR " error %d - %s)",
                                cert_name, *family,
                                errorCode, slapd_pr_strerror(errorCode));
-                slapi_ch_free((void **)&personality);
+                slapi_ch_free_string(&personality);
                 CERT_DestroyCertificate(cert);
                 cert = NULL;
                 freeConfigEntry(&e);
-- 
2.21.0