Security audit finding regarding null ciphers being reported by customer:

Vulnerability:
SSL Null Cipher Suites Supported Port: 389/tcp Here is the list of null SSL ciphers supported by the remote server : Null Ciphers (no encryption) TLSv1 NULL-SHA Kx=RSA Au=RSA Enc=None Mac=SHA1 The fields above are : {OpenSSL ciphername} Kx={key exchange} Au={authentication} Enc={symmetric encryption method} Mac={message authentication code} {export flag}

To Do:
Reconfigure the affected application, if possible to avoid the use ofnull ciphers.

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1117979

your tests always set nsSSL3Ciphers, but by default this is not set in dse.ldif, so all ciphers would be disabled. One part of the request to have a "hardened" default set of ciphers if nsSSL3Ciphers is not specified.

Before the patch, if "all" (without a "+" or "-") is present in the ciphers string, what happens?

Replying to [comment:8 rmeggins]:

Before the patch, if "all" (without a "+" or "-") is present in the ciphers string, what happens?

It's not accepted (It's been designed like this from the beginnning, I guess).
It issues this this error.
{{{
PR_snprintf(err, sizeof(err), "invalid ciphers <%s>: format is +cipher1,-cipher2...", raw);
}}}
Do we want to accept "all (no + or -)"?

Replying to [comment:7 lkrispen]:

your tests always set nsSSL3Ciphers, but by default this is not set in dse.ldif, so all ciphers would be disabled. One part of the request to have a "hardened" default set of ciphers if nsSSL3Ciphers is not specified.

Thank you, Ludwig! A good point. The case is not covered. I'm working on it...

Replying to [comment:9 nhosoi]:

Replying to [comment:8 rmeggins]:

Before the patch, if "all" (without a "+" or "-") is present in the ciphers string, what happens?

It's not accepted (It's been designed like this from the beginnning, I guess).
It issues this this error.
{{{
PR_snprintf(err, sizeof(err), "invalid ciphers <%s>: format is +cipher1,-cipher2...", raw);
}}}
Do we want to accept "all (no + or -)"?

The only reason is if "all" means "+all". However, if "all" by itself is not, and has never been accepted, then this patch won't break any existing deployments.

git patch file (master) -- revised; As pointed out in comment:7, disable all ciphers if nsSSL3Cipher does not exist or empty.
0001-Ticket-47838-harden-the-list-of-ciphers-available-by.patch

git patch file (master) -- revised; Added more test cases.
0002-Ticket-47838-CI-test-add-test-case-for-ticket-47838.patch

If by default now all cipheres are disabled, this will break all deployments which haven't set nsSSL3Cipher. Before backporting this to 1.3.2 we should verify if this is ok with IPA. Martin isn't sure.

git patch file (master) -- revised; adding "nsSSL3Ciphers: default"
0001-Ticket-47838-harden-the-list-of-ciphers-available-by.2.patch

git patch file (master) -- CI test: add more test cases
0002-Ticket-47838-CI-test-add-test-case-for-ticket-47838.2.patch

ack, but note that this adds even more information that we have to maintain in our internal ssl code. We should move as soon as possible to have this information provided to us by NSS - code to map strings to ciphers, code to inspect which ciphers are considered weak and deprecated by NSS, etc.

git patch file (master) -- revised; abandoned the hardcoced cipher table
0001-Ticket-47838-harden-the-list-of-ciphers-available-by.3.patch

git patch file (master) -- CI test: add test case for ticket 47838
0002-Ticket-47838-CI-test-add-test-case-for-ticket-47838.3.patch

Reviewed by Rich (Thank you!!)

Pushed to master:
f4814bd..03df340 master -> master
commit 13c0d2f
commit 03df340

git patch file (master) -- Coverity 12734 - Uninitialized scalar variable
0001-Ticket-47838-harden-the-list-of-ciphers-available-by.4.patch

Coverity fix:
b85efe4..2b7b9cb master -> master
commit 2b7b9cb

Do we need this?

We may be able to add another config attribute allowWeakCipher in cn=encryption. By default, it'd be "on" (same behaviour as we see now). Once set it to "off", weak ciphers would not be allowed and dropped. The problem I could think of is if the ciphers you specifiy are all weak (like in the above log), the server does not start.

As I said in teh corresponding mail thread I do like the idea, it would allow to get easily
(nss3ciphers: +all, allowWeakCipher: off) a reasonable setting of ciphers.
If allowWeakCipher: off is used together with an explicit setting of only weak ciphers I would say that is a user error.

I would aslo like to repeat my suggetsion to seen the enabled (resulting) set of cipehers not only at startup with some log level, but also be able to get the list returned when searching for cn=encryption - or should I open an enhancement ticket for this ?

Thanks for the confirmation, Ludwig. Reopening...

Updating 389-ds from 1.3.2.22-1 to 1.3.3.0-1 on Fedora 21 caused it to fail to start up properly, for me, and this change was the cause.

It adds some code that checks whether the ciphers listed in nsSSL3Ciphers: are available in the current NSS:

{{{
if (lookup) { / lookup with old cipher name and get NSS cipherSuiteName /
for (i = 0; _lookup_cipher[i].alias; i++) {
if (!PL_strcasecmp(ciphers, _lookup_cipher[i].alias)) {
if (!_lookup_cipher[i].name[0]) {
slapd_SSL_warn("Cipher suite %s is not available in NSS %d.%d",
ciphers, NSS_VMAJOR, NSS_VMINOR);
break;
}
}}}

that check fails for all fortezza suites, for me:

Sep 09 20:38:40 ipa001.domain1.local ns-slapd[2454]: [09/Sep/2014:20:38:40 -0700] - SSL alert: Cipher suite fortezza is not available in NSS 3.17

but several fortezza suites are in the default cipher list you get when you deploy 1.3.2.22, and nothing changes nsSSL3Ciphers when you upgrade from 1.3.2.22 to 1.3.3.0, on Fedora at least. So you wind up with a cipher suite that fails the test, even though you never touched it manually.

I confirmed that manually editing all 'fortezza' references out of /etc/dirsrv/slapd-DOMAIN1-LOCAL/dse.ldif caused this problem to go away.

Downstream report: https://bugzilla.redhat.com/show_bug.cgi?id=1139954

the failure to change the existing cipher suite on upgrade has another fatal consequence even after you work around the fortezza bug: pki-server fails to start. /var/log/pki/pki-tomcat/ca/debug shows connection to the LDAP server failing, and /var/log/dirsrv/slapd-DOMAIN1-LOCAL/access shows:

conn-8 op=-1 fd=69 closed - Cannot communicate securely with peer: no common encryption algorithm(s).

so I gave up on little tweaks and just went back into dse.ldif and entirely dropped the nsSSL3Ciphers: setting, which I believe after this patch causes the server to simply use its 'default' list of ciphers, and bingo - 389-ds starts successfully, pki-server starts successfully, and we have lift-off.

So, I think the bug here is basically - this change forgot to handle upgrades. Ideally it would know if the sysadmin had ever manually touched the cipher suite, and if not, it would simply drop that config item entirely and rely on the new 'default' function. I'm not sure what appropriate behaviour would be in cases where the admin had manually touched the cipher suite, or what upgrade should do if it can't tell whether the admin's touched it, but the current situation seems pretty bad for upgrades.

Replying to [comment:24 adamwill]:

the failure to change the existing cipher suite on upgrade has another fatal consequence even after you work around the fortezza bug: pki-server fails to start. /var/log/pki/pki-tomcat/ca/debug shows connection to the LDAP server failing, and /var/log/dirsrv/slapd-DOMAIN1-LOCAL/access shows:

conn-8 op=-1 fd=69 closed - Cannot communicate securely with peer: no common encryption algorithm(s).

so I gave up on little tweaks and just went back into dse.ldif and entirely dropped the nsSSL3Ciphers: setting, which I believe after this patch causes the server to simply use its 'default' list of ciphers, and bingo - 389-ds starts successfully, pki-server starts successfully, and we have lift-off.

So, I think the bug here is basically - this change forgot to handle upgrades. Ideally it would know if the sysadmin had ever manually touched the cipher suite, and if not, it would simply drop that config item entirely and rely on the new 'default' function.

The problem is that the documented procedure for enabling TLS/SSL in the directory server is for the admin to manually set the cipher suite list. I believe it is possible, but it will be tricky, to verify that the lists match.

I'm not sure what appropriate behaviour would be in cases where the admin had manually touched the cipher suite, or what upgrade should do if it can't tell whether the admin's touched it, but the current situation seems pretty bad for upgrades.

We need to somehow warn admins "HEY! YOU ARE USING INSECURE CIPHER SUITES! PLEASE CHANGE THEM ASAP!" Should we also add "SERVER WILL RUN INSECURELY UNTIL YOU FIX THIS, SO HURRY!" ???

git patch file (master) -- additional features; 1. allowWeakCipher, 2. ignore unsupported cipher suite in nsSSL3Ciphers
0001-Ticket-47838-harden-the-list-of-ciphers-available-by.5.patch

git patch file (master) -- additional CI test cases
0003-Ticket-47838-CI-test-add-test-case-for-ticket-47838.patch

yeah, I realize now I forgot to mention (though it's probably obvious) that I was deploying FreeIPA, so presumably my 389-ds cipher suite list was provided by FreeIPA. All the stuff about 'clean installs' and 'defaults' should be read as applying to a FreeIPA deployment, not a 389-ds only one. Of course the good(?) news there is that for FreeIPA cases we may be able to handle the upgrade better at the FreeIPA level?

Replying to [comment:28 adamwill]:

yeah, I realize now I forgot to mention (though it's probably obvious) that I was deploying FreeIPA, so presumably my 389-ds cipher suite list was provided by FreeIPA. All the stuff about 'clean installs' and 'defaults' should be read as applying to a FreeIPA deployment, not a 389-ds only one. Of course the good(?) news there is that for FreeIPA cases we may be able to handle the upgrade better at the FreeIPA level?

We had a discussion about the "good" ciphers. This combination would be the easiest to set safe cipher suites which information is provided by NSS.
{{{
dn: cn=encryption,cn=config
nsSSL3Ciphers: +all
allowWeakCipher: off
}}}
Also, the patch 0001-Ticket-47838-harden-the-list-of-ciphers-available-by.5.patch ignores the existing unsupported cipher suites. Regarding fortezza family, we could not get them since SSL_GetImplementedCiphers does not return it.

On 0001-Ticket-47838-harden-the-list-of-ciphers-available-by.5.patch - there seem to be some formatting changes mixed up in the functional changes, which makes it hard to separate one from the other (don't know if this is common practice for you folks, but thought I'd mention it).

I like Rich's idea of posting a big warning if allowWeakCipher is set, particularly if it's not set explicitly by the sysadmin but has been used because it's the default setting. It'd be good to add that.

also, just an idea, but perhaps allowWeakCipher could only default to on if nsSSL3Ciphers is explicitly set? That way you could get a strong suite by just relying on the default setting (which I think is what the original version of the patch wanted to do), but people who have legacy explicit cipher suites set are less likely to run into trouble as I did?

git patch file (master) -- revised based upon the suggestions by Adam (Thank you!!)
0001-Ticket-47838-harden-the-list-of-ciphers-available-by.6.patch

git patch file (master) -- adjusting the test cases to the new behaviour
0002-Ticket-47838-47895-CI-test-add-test-cases-for-ticket.patch

{{{
129 #define CIPHER_SET_ISDEFAULT(flag) \
130 ((((flag)&CIPHER_SET_CORE) == CIPHER_SET_DEFAULT) ? PR_TRUE : PR_FALSE)
}}}
If flag == CIPHER_SET_DEFAULT|CIPHER_SET_ALLOWWEAKCIPHER, then CIPHER_SET_ISDEFAULT(flag) will evaluate to PR_FALSE. Same with the other CIPHER_SET_XXX macros. Is this as intended?

{{{
507 slapd_SSL_warn("Cipher %s is weak. It is enabled since allowWeakCipher is \"on\" "
508 "(default setting for the backward compatibility). "
509 "We strongly recommend to set it to \"off\"", ciphers);
}}}
We should tell the user the exact steps to set the cipher to "off". Same with here:
{{{
539 slapd_SSL_warn("Cipher %s is weak. "
540 "It is enabled since allowWeakCipher is \"on\" "
541 "(default setting for the backward compatibility). "
542 "We strongly recommend to set it to \"off\"",
543 ciphers);
}}}

About this:
{{{
_conf_setallciphers(CIPHER_SET_ALL|CIPHER_SET_DISABLE_ALLOWSWEAKCIPHER(flags), &suplist, NULL);
}}}

flags is passed in as the parameter to _conf_setciphers. The value will be either 0 or CIPHER_SET_ALLOWWEAKCIPHER (0x10).

If flags = 0x10, then the above evaluates to

CIPHER_SET_ALL|((flag)&~CIPHER_SET_ALLOWWEAKCIPHER)

0x1|((0x10)&~0x10)

0x1|((0x10)&0x111111101)

0x1|(0x0)

0x1

If flags = 0x0, then this evaluates to

0x1|((0x00)&~0x10)

0x1|((0x00)&0x111111101)

0x1|((0x00)&0x111111101)

0x1|(0x0)

0x1

So I guess I don't understand what the intention is if the flags parameter doesn't affect the function?

Replying to [comment:32 rmeggins]:

{{{
129 #define CIPHER_SET_ISDEFAULT(flag) \
130 ((((flag)&CIPHER_SET_CORE) == CIPHER_SET_DEFAULT) ? PR_TRUE : PR_FALSE)
}}}
If flag == CIPHER_SET_DEFAULT|CIPHER_SET_ALLOWWEAKCIPHER, then CIPHER_SET_ISDEFAULT(flag) will evaluate to PR_FALSE. Same with the other CIPHER_SET_XXX macros. Is this as intended?
{{{

define CIPHER_SET_ALL 0x1

define CIPHER_SET_NONE 0x0

define CIPHER_SET_DEFAULT 0x2

define CIPHER_SET_CORE (CIPHER_SET_ALL|CIPHER_SET_DEFAULT|CIPHER_SET_NONE)

define CIPHER_SET_ALLOWWEAKCIPHER 0x10 / can be or'ed with other CIPHER_SET flags /

If flag == CIPHER_SET_DEFAULT|CIPHER_SET_ALLOWWEAKCIPHER,
(flag)&CIPHER_SET_CORE == (CIPHER_SET_DEFAULT|CIPHER_SET_ALLOWWEAKCIPHER)&(CIPHER_SET_ALL|CIPHER_SET_DEFAULT|CIPHER_SET_NONE)
== (0x2|0x10)&(0x1|0x2) == 0x2 == CIPHER_SET_DEFAULT ??
}}}

{{{
507 slapd_SSL_warn("Cipher %s is weak. It is enabled since allowWeakCipher is \"on\" "
508 "(default setting for the backward compatibility). "
509 "We strongly recommend to set it to \"off\"", ciphers);
}}}
We should tell the user the exact steps to set the cipher to "off".
How about this?
507 slapd_SSL_warn("Cipher %s is weak. It is enabled since allowWeakCipher is \"on\ " "
508 "(default setting for the backward compatibility). "
509 "We strongly recommend to set it to \"off\". "
510 "Please replace the value of allowWeakCipher with \"off\" in "
511 "the encryption config entry cn=encryption,cn=config and "
512 "restart the server.", ciphers);

Same with here:
{{{
539 slapd_SSL_warn("Cipher %s is weak. "
540 "It is enabled since allowWeakCipher is \"on\" "
541 "(default setting for the backward compatibility). "
542 "We strongly recommend to set it to \"off\"",
543 ciphers);
}}}

About this:
{{{
_conf_setallciphers(CIPHER_SET_ALL|CIPHER_SET_DISABLE_ALLOWSWEAKCIPHER(flags), &suplist, NULL);
}}}

flags is passed in as the parameter to _conf_setciphers. The value will be either 0 or CIPHER_SET_ALLOWWEAKCIPHER (0x10).

If flags = 0x10, then the above evaluates to

CIPHER_SET_ALL|((flag)&~CIPHER_SET_ALLOWWEAKCIPHER)

0x1|((0x10)&~0x10)

0x1|((0x10)&0x111111101)

0x1|(0x0)

0x1

If flags = 0x0, then this evaluates to

0x1|((0x00)&~0x10)

0x1|((0x00)&0x111111101)

0x1|((0x00)&0x111111101)

0x1|(0x0)

0x1

So I guess I don't understand what the intention is if the flags parameter doesn't affect the function?

Yes, your question is completely valid and this is the one I need to clean up. Currently, "allowWeakCipher: on" never be applied to the cases "nsSSL3Ciphers: +all | default". I'd like to change it to 1) if no allowWeakCipher is set, +all or default does NOT include weak ciphers. 2) If allowWeakCipher is "on", +all or default includes weak ciphers. Note: for specific cipher suites given by the users, the default allowWeakCipher setting is "on".

But I'm leaning toward to putting that fix in the next patch... I'm attaching a patch with the warning fixes...

git patch file (master) -- revised following the comments by Rich (Thank you!!)
0001-Ticket-47838-harden-the-list-of-ciphers-available-by.7.patch

The new messages look good.

Is the intention of CIPHER_SET_ISDEFAULT(flag) to return PR_TRUE if flag == 0x2 and to return PR_FALSE if flag == 0x2|0x10? Because, to me, that is the way it works.

Both default_allow and default_only are returned TRUE... Am I missing something?
test.c

Hi Rich, since trac does not send an email if just attaching a file, I'm writing this comment. :) Could you please take a look at the above attachment?

Ok, you are correct, sorry about that. CIPHER_SET_ISDEFAULT look ok.

I'm still confused about the

_conf_setallciphers(CIPHER_SET_ALL|CIPHER_SET_DISABLE_ALLOWSWEAKCIPHER(flags), &suplist, NULL);

but you said you are going to fix that in another patch?

Replying to [comment:36 rmeggins]:

Ok, you are correct, sorry about that. CIPHER_SET_ISDEFAULT look ok.

I'm still confused about the

_conf_setallciphers(CIPHER_SET_ALL|CIPHER_SET_DISABLE_ALLOWSWEAKCIPHER(flags), &suplist, NULL);

but you said you are going to fix that in another patch?
Yes, that's correct. Sorry about the confusion caused by CIPHER_SET_DISABLE_ALLOWSWEAKCIPHER... That needs to be fixed. So, I keep opening this ticket for the fix... Thanks, Rich!

Reviewed by Rich (Thank you!!)

Pushed to master:
de57632..0f1a203 master -> master
commit 5f3c87e
commit 4fb1a04

Pushed to 389-ds-base-1.3.3:
55e317f..cad5b96 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 58cb12a
commit 3877981

I have installed 389 with new build :: SSL + default allowWeakCipher + CIPHERS="-rsa_null_md5,+rsa_fips_3des_sha,+rsa_fips_des_sha,+rsa_3des_sha,+rsa_rc4_128_md5,+rsa_des_sha,+rsa_rc2_40_md5,+rsa_rc4_40_md5,+fortezza"

Error Logs

[11/Sep/2014:06:22:01 -0400] - SSL alert: Security Initialization: Failed to set SSL cipher preference information: unknown cipher rsa_des_sha (Netscape Portable Runtime error 0 - no error)

1. Why it is giving this error? when rsa_des_sha is valid cipher, Is it becasue it is not in NSS3?

1. allowWeakCipher is accepting any invalid value - Please fix this - not super critical though!
   [root@dhcp201-109 export]# ldapmodify -x -h localhost -p 389 -D "cn=Directory Manager" -w Secret123 << EOF
   dn: cn=encryption,cn=config
   changetype: modify
   replace: allowWeakCipher
   allowWeakCipher: poor

   EOF
   modifying entry "cn=encryption,cn=config"

2. What is the effect of allowWeakCipher:OFF , description of patch says it rejects weak ciphers, but how and what is the impact? It just stopped logging messages for weak ciphers.
   My instance started normally with SSL enabled and in secure mode with weak ciphers while I have allowWeakCipher:OFF.

Error Logs

[11/Sep/2014:06:56:51 -0400] - slapd shutting down - signaling operation threads - op stack size 2 max work q size 1 max work q stack size 1
[11/Sep/2014:06:56:51 -0400] - slapd shutting down - waiting for 1 thread to terminate
[11/Sep/2014:06:56:51 -0400] - slapd shutting down - closing down internal subsystems and plugins
[11/Sep/2014:06:56:51 -0400] - Waiting for 4 database threads to stop
[11/Sep/2014:06:56:51 -0400] - All database threads now stopped
[11/Sep/2014:06:56:51 -0400] - slapd shutting down - freed 1 work q stack objects - freed 2 op stack objects
[11/Sep/2014:06:56:51 -0400] - slapd stopped.
[11/Sep/2014:06:56:52 -0400] - SSL alert: Configured NSS Ciphers
[11/Sep/2014:06:56:52 -0400] SSL Initialization - SSL version range: min: SSL3, max: TLS1.2
[11/Sep/2014:06:56:52 -0400] - 389-Directory/1.3.3.2.a1 B2014.254.627 starting up
[11/Sep/2014:06:56:52 -0400] - I'm resizing my cache now...cache was 1677721 and is now 1342176
[11/Sep/2014:06:56:52 -0400] - slapd started. Listening on All Interfaces port 389 for LDAP requests
[11/Sep/2014:06:56:52 -0400] - Listening on All Interfaces port 636 for LDAPS requests

My kung fu was too weak to figure this out for sure, so I'll just ask - did the final version of this patch include my suggestion that allowWeakCipher be off by default if nsSSL3Cipher is not set or is set to "default"?

If so FreeIPA could simplify their implementation further by not explicitly setting it at all (currently they're setting nsSSL3Cipher: +all , allowWeakCipher: off , which is a substantial improvement on the previous case but is still an explicit setting and it'd be best if they could entirely rely on upstream default).

Replying to [comment:40 amsharma]:

I have installed 389 with new build :: SSL + default allowWeakCipher + CIPHERS="-rsa_null_md5,+rsa_fips_3des_sha,+rsa_fips_des_sha,+rsa_3des_sha,+rsa_rc4_128_md5,+rsa_des_sha,+rsa_rc2_40_md5,+rsa_rc4_40_md5,+fortezza"

Error Logs

[11/Sep/2014:06:22:01 -0400] - SSL alert: Security Initialization: Failed to set SSL cipher preference information: unknown cipher rsa_des_sha (Netscape Portable Runtime error 0 - no error)

1. Why it is giving this error? when rsa_des_sha is valid cipher, Is it becasue it is not in NSS3?

A bug in the mapping table bug. Fixed in ticket #47908.

=================================================================================================

1. allowWeakCipher is accepting any invalid value - Please fix this - not super critical though!
   [root@dhcp201-109 export]# ldapmodify -x -h localhost -p 389 -D "cn=Directory Manager" -w Secret123 << EOF
   dn: cn=encryption,cn=config
   changetype: modify
   replace: allowWeakCipher
   allowWeakCipher: poor

   EOF
   modifying entry "cn=encryption,cn=config"

Thanks. Taking a look...

1. What is the effect of allowWeakCipher:OFF , description of patch says it rejects weak ciphers, but how and what is the impact? It just stopped logging messages for weak ciphers.
   My instance started normally with SSL enabled and in secure mode with weak ciphers while I have allowWeakCipher:OFF.

Please attach your cn=encryption,cn=config entry to this ticket. Also, error log with nsslapd-errorlog-level: 64.

Please note that you are getting a new build with the fix for #47908, which would behave a bit differently from the current... So, I recommend you to come back to the bug once the new build is ready.

Thanks, Ami!

Replying to [comment:41 adamwill]:

My kung fu was too weak to figure this out for sure, so I'll just ask - did the final version of this patch include my suggestion that allowWeakCipher be off by default if nsSSL3Cipher is not set or is set to "default"?

Thank you for your input, Adam. I'm going to change the spec so that...
by default (no explicit allowWeakCipher setting in cn=encryption,cn=config),
allowWeakCipher is on for user specified cipher list
allowWeakCipher is off for "+all" and "default"

And explicit allowWeakCipher setting (allowWeakCipher: on|off) is applied to all cases (all, default, and user specified cipher list).

If so FreeIPA could simplify their implementation further by not explicitly setting it at all (currently they're setting nsSSL3Cipher: +all , allowWeakCipher: off , which is a substantial improvement on the previous case but is still an explicit setting and it'd be best if they could entirely rely on upstream default).

Agreed.

Awesome! That's exactly what I was suggesting. Thanks very much.

git patch file (master) -- phase 2
0001-Ticket-47838-harden-the-list-of-ciphers-available-by.8.patch

git patch file (master) -- CI test: adjusted test cases based on the phase 2 fixes for ticket 47838
0001-Ticket-47838-CI-test-adjusted-test-cases-based-on-th.patch

Found a culprit. "enabled" message is logged even for a cipher preceded by "-" and it's not really "enabled". Fixing it.

git patch file (master) -- phase 2 with the fix for "enabled" message is logged even for a cipher preceded by "-" and it's not really "enabled"
0001-Ticket-47838-harden-the-list-of-ciphers-available-by.9.patch

Hello Noriko,

The fix looks good to me. I have just a concern regarding the semantic.
If nsSSL3Cipher=+all and allowWeakCipher is not set in cn=encryption,cn=config.
Then weak ciphers are not allowed. IMHO if a customer wants to allow any ciphers (even the weak ones), +all should be enough.

thanks

Replying to [comment:48 tbordaz]:

Hello Noriko,

The fix looks good to me. I have just a concern regarding the semantic.
If nsSSL3Cipher=+all and allowWeakCipher is not set in cn=encryption,cn=config.
Then weak ciphers are not allowed. IMHO if a customer wants to allow any ciphers (even the weak ones), +all should be enough.

Thanks, Thierry. Actually that was the request from IPA. :) I think IPA is prioritizing the security.

but I believe with the change discussed in https://fedorahosted.org/389/ticket/47838#comment:43 , IPA could simply leave the setting unspecified, and the default configuration would be what they wanted - so they would no longer need the behaviour described in /47838#comment:48 and /47838#comment:49 . Does that make sense? :) It would be something to check with that team.

Replying to [comment:50 adamwill]:

but I believe with the change discussed in https://fedorahosted.org/389/ticket/47838#comment:43 , IPA could simply leave the setting unspecified, and the default configuration would be what they wanted - so they would no longer need the behaviour described in /47838#comment:48 and /47838#comment:49 . Does that make sense? :) It would be something to check with that team.

I agree that if nsSSL3Cipher is not set or set to "default", then the default (allowWeakCipher not set) value for allowWeakCipher=off. That was your suggestion.
However, if nsSSL3Cipher=+all, I think the default value for allowWeakCipher would better be 'on'.

If IPA configuration would eventually not set nsSSL3Cipher/allowWeakCipher, then nsSSL3Cipher=default and allowWeakCipher=off. My understanding is that it is different from nsSSL3Cipher=+all/allowWeakCipher=off (that is the current setting)

Hum, I believe you're correct. I'd possibly argue that using NSS' default cipher list, not all ciphers excepting those it considers 'weak', might be a better choice, but I agree that the approach I suggest would be a change to the current behaviour.

Discussion agreed that when nsSSL3Cipher="+all" the default value for allowWeakCipher (when allowWeakCipher is not explicitely set) should be 'off'. This guaranty a better level of security.

The fix is looking good to me.

Reviewed by Thierry (Thank you!!)

Pushed to master:
50820f8..ce73789 master -> master
commit c6febe3
commit c6c73e6

Pushed to 389-ds-base-1.3.3:
ab36560..b922e5d 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 411ca8f
commit b5ce880

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 issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1169

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.