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

TLS Fallback Signaling Cipher Suite Value #165

Merged
merged 1 commit into from Nov 3, 2017
Merged

TLS Fallback Signaling Cipher Suite Value #165

merged 1 commit into from Nov 3, 2017

Conversation

@aeris
Copy link
Contributor

@aeris aeris commented Oct 29, 2017

Support for fallback SCSV RFC 7507.

Expected behaviour is to refuse connection if the client signals a protocol with
the fallback flag but the server supports a better one (downgrade attack detection).

long modes;

GetSSLCTX(self, ctx);
if(!ctx){

This comment has been minimized.

@rhenium

rhenium Oct 30, 2017
Member

ctx is never NULL.

This comment has been minimized.

@aeris

aeris Oct 30, 2017
Author Contributor

Fixed


modes = SSL_CTX_get_mode(ctx);
modes |= SSL_MODE_SEND_FALLBACK_SCSV;
SSL_CTX_set_mode(ctx, modes);

This comment has been minimized.

@rhenium

rhenium Oct 30, 2017
Member

SSL_CTX_set_mode() preserves the previously set flags, so just SSL_CTX_set_mode(ctx, SSL_MODE_SEND_FALLBACK_SCSV) is fine.

This comment has been minimized.

@aeris

aeris Oct 30, 2017
Author Contributor

Fixed

rescue OpenSSL::SSL::SSLError => e
retry if ignore_listener_error
raise unless e.message =~ /inappropriate fallback/
rescue IOError, Errno::EBADF, Errno::EINVAL,

This comment has been minimized.

@rhenium

rhenium Oct 30, 2017
Member

Are changes in start_server really needed? I'd like to have all SCSV specific code in TestSSL#test_fallback_scsv.

This comment has been minimized.

@aeris

aeris Oct 30, 2017
Author Contributor

An exception occurs on server part too in case of SCSV error.
Without this catch, unit test detect only the server part and not the tested client part as expected.
I didn't find a proper way to do this :'(

This comment has been minimized.

@rhenium

rhenium Oct 30, 2017
Member

You can get a raw TCP socket pair by socketpair defined in TestSSL when the exception raised by SSLSocket#accept has to be tested.

This comment has been minimized.

@rhenium

rhenium Oct 30, 2017
Member

Ah, I misinterpreted your comment. Errors raised in server can be suppressed by passing 'ignore_listener_error: true' to start_server.

This comment has been minimized.

@aeris

aeris Oct 30, 2017
Author Contributor

I fix the test with socketpair, to also test server side error.

static VALUE
ossl_sslctx_enable_fallback_scsv(VALUE self)
{
#ifdef SSL_MODE_SEND_FALLBACK_SCSV

This comment has been minimized.

@rhenium

rhenium Oct 30, 2017
Member

The method should not be defined at all if SSL_MODE_SEND_FALLBACK_SCSV is not usable.

This comment has been minimized.

@aeris

aeris Oct 30, 2017
Author Contributor

Fixed

self.options |= OpenSSL::SSL::OP_ALL
self.ssl_version = version if version
self.enable_fallback_scsv if fallback_scsv

This comment has been minimized.

@rhenium

rhenium Oct 30, 2017
Member

I think #initialize should take all or nothing -- it's just confusing to have only fallback_scsv option.

This comment has been minimized.

@aeris

aeris Oct 30, 2017
Author Contributor

OpenSSL::SSL::SSLContext is generally used with the short form SSLContext.new version, without configuration after that. Having SCSV set on initialize allows to keep short form with SCSV protection SSLContext.new version, fallback_scsv: true without configuration too.

This comment has been minimized.

@rhenium

rhenium Oct 30, 2017
Member

As in the RDoc comment above, use of the form "SSL::SSLContext.new(ssl_method_name)" (and SSL::SSLContext#ssl_version= which is called from it) is not recommended anymore. This is because the version-specific SSL methods has been deprecated in the OpenSSL C API.

In addition to the protocol version which is now preferably set by SSL::SSLContext#{min,max}_version=, clients would usually need to set other options such as verify_mode, too. IMHO SSL::SSLContext has too much options to enumerate in kwargs...

This comment has been minimized.

@rhenium

rhenium Nov 3, 2017
Member

Ping on this?

@@ -1222,6 +1222,49 @@ def test_get_ephemeral_key
end
end

def test_fallback_scsv
ctx_proc = proc { |ctx|
ctx.ciphers = "kRSA"

This comment has been minimized.

@rhenium

rhenium Oct 30, 2017
Member

Avoid kRSA since it will not work with TLS 1.3-capable OpenSSL. It's not finalized yet, but I try to keep the tests pass with the master branch of OpenSSL.

This comment has been minimized.

@aeris

aeris Oct 30, 2017
Author Contributor

Fixed

@rhenium
Copy link
Member

@rhenium rhenium commented Oct 30, 2017

Just out of curiosity (I'm fine with either one), is there any particular reason to choose #enable_fallback_scsv over #{send_,}fallback_scsv=true|false?

@aeris
Copy link
Contributor Author

@aeris aeris commented Oct 30, 2017

fallback_scsv=false is a little non-sense. In practice, you activate SCSV only when you fallback to lower version after a failed connection. "Disabling" SCSV has no meaning.

@rhenium
Copy link
Member

@rhenium rhenium commented Oct 30, 2017

Fair enough, I agree nobody would bother with setting that explicitly.

@@ -1222,6 +1222,56 @@ def test_get_ephemeral_key
end
end

def test_fallback_scsv

This comment has been minimized.

@rhenium

rhenium Nov 3, 2017
Member

This test case is failing with LibreSSL or older patch versions of OpenSSL 1.0.1 because they lack support for TLS_FALLBACK_SCSV. pend if the method does not exist.

https://travis-ci.org/ruby/openssl/jobs/294781400#L660

This comment has been minimized.

@aeris

aeris Nov 3, 2017
Author Contributor

Fixed

}

assert t.join
end

This comment has been minimized.

@rhenium

rhenium Nov 3, 2017
Member

Sockets created by socketpair have to be closed manually to prevent FD leak (which could be an issue when running tests repeatedly for debugging).

This comment has been minimized.

@aeris

aeris Nov 3, 2017
Author Contributor

Fixed

@@ -1222,6 +1222,56 @@ def test_get_ephemeral_key
end
end

def test_fallback_scsv
ctx_proc = proc { |ctx|
ctx.ssl_version = :TLSv1_2

This comment has been minimized.

@rhenium

rhenium Nov 3, 2017
Member

Please use #max_version= and #min_version= in new tests. In this case, specifying just max_version would be enough. (This applies to the other instances too.)

This comment has been minimized.

@aeris

aeris Nov 3, 2017
Author Contributor

Fixed

sock1, sock2 = socketpair
ctx1 = OpenSSL::SSL::SSLContext.new
ctx1.min_version = OpenSSL::SSL::TLS1_1_VERSION
ctx1.max_version = OpenSSL::SSL::TLS1_2_VERSION

This comment has been minimized.

@rhenium

rhenium Nov 3, 2017
Member

The server doesn't need to set neither min_version nor max_version since we know all protocol versions are enabled by default.

ctx1 = OpenSSL::SSL::SSLContext.new
ctx1.min_version = OpenSSL::SSL::TLS1_1_VERSION
ctx1.max_version = OpenSSL::SSL::TLS1_2_VERSION
s1 = OpenSSL::SSL::SSLSocket.new sock1, ctx1

This comment has been minimized.

@rhenium

rhenium Nov 3, 2017
Member

Please parenthesize arguments for consistency with other tests in this file.

* Activate TLS_FALLBACK_SCSV for this context.
* See RFC 7507.
*/
#ifdef SSL_MODE_SEND_FALLBACK_SCSV

This comment has been minimized.

@rhenium

rhenium Nov 3, 2017
Member

I think ossl_sslctx_enable_fallback_scsv() should be placed after ossl_sslctx_set_security_level() so the ordering match with the rb_define_method() lines. (We haven't been always consistent, but ...)

By the way, RDoc seems to fail to find the doc. Moving #ifdef SSL_MODE_SEND_FALLBACK_SCSV above the block comment fixes it for me. This might be considered a bug of RDoc's C parser, though.


start_server do |port|
ctx = OpenSSL::SSL::SSLContext.new
ctx.ssl_version = :TLSv1_2

This comment has been minimized.

@rhenium

rhenium Nov 3, 2017
Member

Please use #max_version= for client SSL contexts, too.

This comment has been minimized.

@aeris

aeris Nov 3, 2017
Author Contributor

Ok, fixed.
But for client, it's strange to have to use min/max, because AFAIK, SSLContext doesn't handle fallback by itself and support only a single version (the max_version).

This comment has been minimized.

@rhenium

rhenium Nov 3, 2017
Member

Yes, in a real use case, the client will surely enable only one protocol version. The point is, in this case, that #ssl_version= should not be used because it's an old and deprecated method. New applications should write as:

ctx.min_version = ctx.max_version = ...

This comment has been minimized.

@aeris

aeris Nov 3, 2017
Author Contributor

In this case, why not reimplement old ssl_version with min/max_version under the hood ?
min/max is cool for server part, but for client part, better to have a single version, no ?

This comment has been minimized.

@rhenium

rhenium Nov 3, 2017
Member

Actually It's been rewritten to call #min_version= and #max_version=. But it's a different story whether we should recommend using that or not.

# call-seq:

It's a decision of the OpenSSL C API that the SSL/TLS protocol version specific SSL methods, such as TLSv1_2_method() which corresponds to :TLSv1_2, are now deprecated. Please see TLSv1_2_method(3).

https://www.openssl.org/docs/manmaster/man3/TLSv1_2_method.html
(The web site seems down now)

@rhenium
Copy link
Member

@rhenium rhenium commented Nov 3, 2017

This looks really nice to me. Thanks for your patience!

Lastly, can you squash the commits into one commit?

@aeris aeris force-pushed the aeris:scsv branch from 1df82e7 to da9be3b Nov 3, 2017
@rhenium
rhenium approved these changes Nov 3, 2017
Support for fallback SCSV [RFC 7507](https://tools.ietf.org/html/rfc7507).

Expected behaviour is to refuse connection if the client signals a protocol with
the fallback flag but the server supports a better one (downgrade attack detection).
@aeris aeris force-pushed the aeris:scsv branch from da9be3b to 7c4028a Nov 3, 2017
@aeris
Copy link
Contributor Author

@aeris aeris commented Nov 3, 2017

Done :)

@rhenium rhenium merged commit 819d7e5 into ruby:master Nov 3, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aeris aeris deleted the aeris:scsv branch Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.