Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force RC4 cipher for our ancient LDAP server #6

Merged
merged 1 commit into from
Oct 24, 2017

Conversation

dncrht
Copy link

@dncrht dncrht commented Oct 23, 2017

Windows 2003's LDAP server only supports RC4 cipher.
Given we only care whether we use SSL or not, when SSL is enabled, encryption is set to simple_tls and the supported ciphers are set.

Recently, Ruby's 2.4 openssl gem has removed RC4 from its default params, however doesn't disallow its usage. See: ruby/openssl#50
By letting our LDAP server we want to use RC4, we are able to communicate with it.

BONUS: By merging this PR means we can remove the LDAP monkey patch from our apps!

Tested by @sivalpatel and @ed-woodfall locally

Our LDAP server runs on Win2003 and only supports RC4 cipher.
We only care whether we use SSL or not. On SSL enabled, encryption
is set to simple_tls and the supported ciphers are set.

Ruby's 2.4 openssl gem has removed RC4 from its default params, however
doesn't disallow its usage. See: ruby/openssl#50
@dncrht dncrht merged commit 5e6f429 into master Oct 24, 2017
@dncrht dncrht deleted the force-rc4-cipher-for-our-ancient-ldap-server branch October 24, 2017 13:42
@pvdb
Copy link

pvdb commented Nov 28, 2017

I'm not so sure at all that this is a very good idea...

We are basically copying the OpenSSL::SSL::SSLContext::DEFAULT_PARAMS from Ruby 2.3.x and hardcoding them into this LDAP authentication gem, which means that any future Ruby changes to the DEFAULT_PARAMS hash will be ignored.

In the process we are also removing :verify_hostname=>true from the default Ruby 2.4.x params, which is arguably a much bigger concern, as it fixed a long-standing security hole in Ruby:

ruby/openssl#60

The reason the monkey patch - https://gist.github.com/pvdb/db19f27571e41f735d24 - was approved at the time is that it was a lot more surgical in its monkey patching... this PR will inevitably impact each and every one of our apps that uses the gem (even the ones that don't have to connect to an RC4-only LDAP server!) so it has a much wider blast radius.

Wouldn't it have been better to surgically (!) put the required ciphers (["ECDHE-ECDSA-RC4-SHA", "ECDHE-RSA-RC4-SHA", "RC4-SHA"]) back into the DEFAULT_PARAMS, if that's what it takes to talk to our ancient LDAP server?


For reference, here's how OpenSSL::SSL::SSLContext::DEFAULT_PARAMS differs between 2.3.3 and 2.4.2 respectively... you can clearly see how the RC4 ciphers have been removed, but how the :verify_hostname option has been added:

--- ruby.2.3.3	2017-11-28 15:25:52.000000000 +0000
+++ ruby.2.4.2	2017-11-28 15:25:54.000000000 +0000
@@ -1,13 +1,13 @@
 ➜ ~  ruby --version
-ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin16]
+ruby 2.4.2p198 (2017-09-14 revision 59899) [x86_64-darwin16]
 ➜ ~  irb -r openssl
 irb(main):001:0> dp = OpenSSL::SSL::SSLContext::DEFAULT_PARAMS ; dp.keys
-=> [:ssl_version, :verify_mode, :ciphers, :options]
+=> [:ssl_version, :verify_mode, :verify_hostname, :options, :ciphers]
 irb(main):002:0> ciphers = dp.delete(:ciphers).split(':') ; ciphers.count
-=> 33
+=> 30
 irb(main):003:0> ciphers.grep /RC4/
-=> ["ECDHE-ECDSA-RC4-SHA", "ECDHE-RSA-RC4-SHA", "RC4-SHA"]
+=> []
 irb(main):004:0> dp.keys
-=> [:ssl_version, :verify_mode, :options]
+=> [:ssl_version, :verify_mode, :verify_hostname, :options]
 irb(main):005:0> dp
-=> {:ssl_version=>"SSLv23", :verify_mode=>1, :options=>2197947391}
+=> {:ssl_version=>"SSLv23", :verify_mode=>1, :verify_hostname=>true, :options=>2197947391}

@pvdb
Copy link

pvdb commented Jan 5, 2018

Maybe add something like...

OpenSSL::SSL::SSLContext::DEFAULT_PARAMS[:ciphers] += [nil, "ECDHE-ECDSA-RC4-SHA", "ECDHE-RSA-RC4-SHA", "RC4-SHA"].join(":")

... to the Monkey Patch somewhere?

@srp2930
Copy link

srp2930 commented Jan 9, 2018

I can confirm that @pvdb 's above suggestion, of amending the Monkey Patch, works for CoCo. Upgrade to 2.4.x caused it to stop working, and currently does not work without the LDAP monkey patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants