Make the SSL version set to the client library configurable.

[389-ds-bot]

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/48784

• Created at 2016-04-01 02:58:39 by nhosoi (@nhosoi)
• Closed as Fixed
• Assigned to nhosoi (@nhosoi)

---

The value to set to LDAP_OPT_X_TLS_PROTOCOL_MIN is hardcoded:

optval = LDAP_OPT_X_TLS_PROTOCOL_SSL3;

We need to have a control to the min value.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2016-04-01 05:57:38

Hi Noriko,

(void)getSSLVersionRangeOL(&optval, NULL);

But you have :

int
getSSLVersionRangeOL(int *min, int *max) 

Followed by:

 if ((rc = ldap_set_option(ld, LDAP_OPT_X_TLS_PROTOCOL_MIN, &optval))) { 

Can you check this return?

Perhaps:

int olresult = getSSLVersionRangeOL(&optval, NULL);
...
if (olresult == 0 && (rc = ldap_set_option(ld, LDAP_OPT_X_TLS_PROTOCOL_MIN, &optval))) { 

Also, your message in the error says:

  "failed: unable to set minimum TLS protocol level to SSL3\n"); 

But doesn't this change allow us to set other protocol minimums? Perhaps this error should be generic, rather than saying SSL3?

Otherwise, I really like this patch. I think it's important we are able to control this.

Could we perhaps also have the option to use https://fedoraproject.org/wiki/Changes/CryptoPolicy ?? Does this apply here or not? It would be good to have this controlled by the system policy by default, and then can be overriden if we want ... Maybe this is a future extension to this ticket?

I have not built or tested this yet.

Thanks for your time!

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-04-01 06:44:42

Oops, thanks for finding out the wrong error message! I'm going to fix it.

Regarding the return value, it might be a bit confusing, but getSSLVersionRangeOL sets the default value LDAP_OPT_X_TLS_PROTOCOL_TLS1_0 even if it returns non-zero (non-zero is returned if the server side ssl is not initialized). I think there is no problem to use the default value for the client connection, which is independent from the server side SSL init. And TLS1_0 is safe for the POODLE attack. So, it's relevant as a default setting...

If you don't like it, I could change getSSLVersionRangeOL to void and remove the SSL init check...

I'm not so sure about the system policy yet. We may need to have some controle independent from the system itself. For instance, we may need to enable SSLv3 when the replication consumer only supports the SSL version, but the system itself does not want to enable it... Probably, getting the default value from the system policy and still being able to have a control to override it if necessary, everyone could be happier... And yes, it'd be better to open a new ticket for it.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-04-01 23:38:38

git patch file (master) -- revised based upon the reviews by William (Thanks!)
0001-Ticket-48784-Make-the-SSL-version-set-to-the-client-.patch

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2016-04-02 01:04:41

Replying to [comment:3 nhosoi]:

Oops, thanks for finding out the wrong error message! I'm going to fix it.

Regarding the return value, it might be a bit confusing, but getSSLVersionRangeOL sets the default value LDAP_OPT_X_TLS_PROTOCOL_TLS1_0 even if it returns non-zero (non-zero is returned if the server side ssl is not initialized). I think there is no problem to use the default value for the client connection, which is independent from the server side SSL init. And TLS1_0 is safe for the POODLE attack. So, it's relevant as a default setting...

Actually I thought TLS 1.0 was NOT safe from POODLE - only 1.1 and up was safe.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2016-04-04 04:21:31

Replying to [comment:3 nhosoi]:

Oops, thanks for finding out the wrong error message! I'm going to fix it.

Regarding the return value, it might be a bit confusing, but getSSLVersionRangeOL sets the default value LDAP_OPT_X_TLS_PROTOCOL_TLS1_0 even if it returns non-zero (non-zero is returned if the server side ssl is not initialized). I think there is no problem to use the default value for the client connection, which is independent from the server side SSL init. And TLS1_0 is safe for the POODLE attack. So, it's relevant as a default setting...

If you don't like it, I could change getSSLVersionRangeOL to void and remove the SSL init check...

Well, if we return the int, we should be doing something with it. So I think either we check it to confirm a version was found, or we remove it and make the function void, if it is guaranteed to never fail.

I'm not so sure about the system policy yet. We may need to have some controle independent from the system itself. For instance, we may need to enable SSLv3 when the replication consumer only supports the SSL version, but the system itself does not want to enable it... Probably, getting the default value from the system policy and still being able to have a control to override it if necessary, everyone could be happier... And yes, it'd be better to open a new ticket for it.

Probably a new ticket for it I think. It will probably be a bit of investigation to make it work.

Actually I thought TLS 1.0 was NOT safe from POODLE - only 1.1 and up was safe.

I think Mark is right here. By default we should be setting the highest TLS version we can, because often the version changes are due to actual protocol weaknesses in TLS itself. IE we should default to TLS1.2 if we can.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-04-07 01:35:41

The answer from the security team.

On 04/04/2016 10:26 PM, Huzaifa Sidhpurwala wrote:

Currently, we are not aware of any attacks which are feasible against a
proper implementation of TLS 1.0 (openssl, nss, gnutls we ship). However
that being said, the safest option is always to use the highest version
available ie TLS 1.2 and fall back to lower versions only, if you cant
use 1.2.

The above is general advice in all cases. If you have a special case in
mind, let me know and we can discuss.

My answer is based on the bits of information i got from the mail i was
copied on :)

This is the access log snippet of the replication. As you see, even though the min value is TLS1.0 (or even setting to SSL3), the higherst available version is picked. So, we may not have to worry too much about it.

[..] conn=3 TLS1.2 128-bit AES-GCM; client CN=test.localdomain0,OU=389 Directory Server; issuer CN=CAcert
[..] conn=3 TLS1.2 client bound as uid=repl_mgr1,cn=config

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-04-12 07:00:47

git patch file (master) -- CI test
0002-Ticket-48784-CI-test-added-test-cases-for-ticket-487.patch

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2016-04-22 06:06:46

Code looks good, runs on my machine, and passes. Ack.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-04-22 11:59:55

Reviewed by William (Thanks!!)

Pushed to master:
4c66307..fa620fc master -> master
commit 0be073f
commit f295342

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2017-02-11 22:52:03

Metadata Update from @nhosoi:

• Issue assigned to nhosoi
• Issue set to the milestone: 1.3.5.2