From c2637e437d5b9fa33410856d2059c2921a90fdac Mon Sep 17 00:00:00 2001 From: Mark Reynolds 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.17.2