389-ds-base

Clone
Source Code
GIT

#50430 Issue 50426 - nsSSL3Ciphers is limited to 1024 characters
Closed 3 years ago by spichugi. Opened 4 years ago by mreynolds.
mreynolds/389-ds-base ticket50426  into  master

file added
+51 
@@ -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])

file modified
+15 -19 
@@ -95,7 +95,6 @@ 
 

  #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) \
 
@@ -694,10 +693,12 @@ 
 

              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';
 
@@ -1694,7 +1695,6 @@ 
 

      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;
 

  
@@ -1735,21 +1735,17 @@ 
 

                             "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))) {

Maybe I'm missing it, but how is ciphers allocated / reallocated to ensure it's large enough? 

          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 */
 
@@ -1820,12 +1816,12 @@ 
 

              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");
 
@@ -1842,8 +1838,8 @@ 
 

                                 "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;
 

              }
 
@@ -1870,7 +1866,7 @@ 
 

                                 "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);

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

Maybe I'm missing it, but how is ciphers allocated / reallocated to ensure it's large enough?

My understanding is that ciphers is allocated reading the config (slapd_ssl_init), checked in _conf_setciphers (slapd_ssl_init2) and then freed. Surprised that it is not used later.

Anyway the fix address the 1024 limit and looks good regarding the previous behavior.
LGTM

Maybe I'm missing it, but how is ciphers allocated / reallocated to ensure it's large enough?

"ciphers" is a static variable, and is set in slapi_ssl_init():

ciphers = slapi_entry_attr_get_charptr(entry, "nsssl3ciphers");

Then in slapd_ssl_init2() we parse it in _conf_setciphers() which copies each cipher substring into the global cipher list struct.

Linter complains here because we don't use the object. Generaly, I think it is better to use log functions from the instances. Like topo.ms["master1"].log.

Linter complains here because we don't use the object. Generaly, I think it is better to use log functions from the instances. Like topo.ms["master1"].log.

This is what create_test.py is doing. Maybe we should change it in there via a different ticket?

Meanwhile I will remove that code from the test script...

rebased onto d690114380bc592ede783826f45ea88262a4db23
4 years ago

Changes applied to CI test please review...

Linter complains here because we don't use the object. Generaly, I think it is better to use log functions from the instances. Like topo.ms["master1"].log.

This is what create_test.py is doing. Maybe we should change it in there via a different ticket?
Meanwhile I will remove that code from the test script...

Yeah, I think we should.

Could you also, please, remove from lib389._constants import * line? Ideally, mass imports with * should be avoided.

So yeah, we should have some create_test.py clean up...

It is really minor. The rest looks good to me!

rebased onto 6b2fd1bd7ac2aa6f9f6c90fc4683614ffb25afdb
4 years ago

rebased onto 22f2f9a
4 years ago

Pull-Request has been merged by mreynolds
4 years ago

All good for me too (sorry for late comment) and thanks for explaining about ciphers allocation!

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3488

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
3 years ago
Metadata
None
No Tags
Changes Summary 2
+51
file added
dirsrvtests/tests/suites/tls/cipher_test.py
+15 -19
file changed
ldap/servers/slapd/ssl.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.