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

Fix rejecting HTTP requests when TLS1.3 is used by server #2116

Merged
merged 3 commits into from May 15, 2020

Conversation

@MSP-Greg
Copy link
Contributor

MSP-Greg commented Feb 18, 2020

Description

Closes #2115

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.
# see OpenSSL ssl/ssl_stat.c SSL_state_string for info
#
def ssl_version_state
::Puma::IS_JRUBY ? ['', ''] : @engine.ssl_vers_st.map(&:rstrip)

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Mar 5, 2020

Member

This is because you didn't implement a JRuby version, right? Not because the JRuby SSL version should actually return different values?

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Mar 5, 2020

Member

Or our JRuby extension doesn't support TLS 1.3?

This comment has been minimized.

Copy link
@MSP-Greg

MSP-Greg Mar 5, 2020

Author Contributor

I wrote the code quite a while ago. At the time, JRuby didn't support TLSv1.3 yet, and I opened an issue somewhere asking about it, didn't really get a response.

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Mar 5, 2020

Member

Ah, got it.

This comment has been minimized.

Copy link
@dentarg

dentarg Mar 5, 2020

Contributor

@MSP-Greg is this the issue? jruby/jruby-openssl#184

This comment has been minimized.

Copy link
@MSP-Greg

MSP-Greg Mar 5, 2020

Author Contributor

@dentarg Yes

@nateberkopec
Copy link
Member

nateberkopec commented Mar 5, 2020

Wondering what your response is to #2115 (comment)

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Mar 5, 2020

Wondering what your response is to

Part of the issue is slow connections, so the code has to wait for some interval to determine if a handshake has completed. The test seems to show that it's rejected, whereas currently it is not. Also, I don't recall what 'state' is as a protocol is negotiated down from TLSv1.3 to TLSv1.2.

What should the code do with the connection as far a response or logging? It's kind like dictionary attacks on mail servers. What's the best way to respond? Let it disappear, respond with unknown whatever, etc...

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Mar 5, 2020

As to why I made this a draft PR...

This is one potential fix for what could be an attack vector, in that http requests to a Puma https server do need to be 'cleaned out' in a timely fashion. But:

  1. Does it affect normal throughput?

  2. How should these https requests be handled? Should the handling change depending on whether Puma is the 'front end' server or behind something like NGINX? If so, how to allow configuration of the handling?

output = @engine.read
return output if bad_tlsv1_3?
rescue Puma::MiniSSL::SSLError => e
if e.message.end_with? 'System error: Undefined error: 0 - 0'

This comment has been minimized.

Copy link
@dentarg

dentarg Apr 10, 2020

Contributor

This doesn't look robust to me, matching against the message... do we know where does this error come from? What does Undefined error: 0 - 0 mean?

At #1708 (comment) I saw the similar but different error

Error in reactor loop escaped: System error: Success - 0 (Puma::MiniSSL::SSLError)

This comment has been minimized.

Copy link
@MSP-Greg

MSP-Greg Apr 10, 2020

Author Contributor

@dentarg

matching against the message

I wasn't happy with using a string match either. Let me get back into this and see what can be done.

What does Undefined error: 0 - 0 mean?

Similar to what happened with 1.1.1e. The error previously had a numeric code of 0, then with 1.1.1e they changed the code, which was a breaking change. I'll look at the code for Puma::MiniSSL::SSLError and see if better info can be passed from the c code.

This issue is messy, as flooding an https server with http connections could be used as an DOS attack.

We want Puma to reject http connections, and we want that done as quickly as possible so a minimal amount of resources are used by them. We also don't want to slow down real https connections.

The issue is made worse by the fact that TLSv1.3 and TLSv1.2 handle http connections differently, and right now, I can't recall that exact difference. As I recall, TLSv1.2 handled the problem itself, TLSv1.3 forces Puma to deal with it...

@nateberkopec
Copy link
Member

nateberkopec commented May 8, 2020

How should these https requests be handled? Should the handling change depending on whether Puma is the 'front end' server or behind something like NGINX? If so, how to allow configuration of the handling?

@MSP-Greg Any further thoughts on this front?

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented May 8, 2020

@nateberkopec

Any further thoughts on this front?

Maybe. Question - OpenSSL 1.0.2 is EOL. Are you ok with adding methods that will only be available with OpenSSL 1.1.0 or later? I don't think many people are using 1.1.0, so it's really about 1.1.1. For instance, the context min/max protocol methods in newer Ruby versions maps directly to new OpenSSL methods, and translating them back into 1.0.2 is a PITA.

I think I mentioned it somewhere else, let me look at the SSL error issue and see what can be done. I've got time this weekend...

@nateberkopec
Copy link
Member

nateberkopec commented May 8, 2020

This is related to the Ruby version issue, no? What's the compat between Ruby versions and OpenSSL versions look like?

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented May 8, 2020

What's the compat between Ruby versions and OpenSSL versions look like?

It's a little grey.

Generally, Ruby 2.2 and 2.3 are considered incompatible with OpenSSL 1.1.0. The 2.2 and 2.3 builds available on Actions for all OS's use 1.0.2.

Re Ruby 2.4, the Actions Ubuntu & macOS builds are done using 1.1.1, but interestingly, you cannot force a TLSv1.3 connection without an error. Whether they'll connect when left alone, I haven't determined.

But, the current Ruby OpenSSL gem (2.1.2) is compatible with Ruby 2.3. So, a crazy person could probably install it using OpenSSL 1.1.1 on Ruby 2.3. I haven't tried that.

Since I wanted to double check some things, I finally created some notes on release dates, etc.

       OpenSSL
Ruby   stdlib               OpenSSL
                            1.1.0 pre 2015-Dec-10
2.3     1.1.1  2015-Dec
                            1.1.0     2016-Aug-25
2.4     2.0.9  2016-Dec
2.5     2.1.2  2017-Dec
                            1.1.1 pre 2018-Feb-13  * includes TLSv1.3
                            1.1.1     2018-Sep-11
2.6     2.1.2  2018-Dec
2.7     2.1.2  2019-Dec
2.8     2.2.0
@nateberkopec
Copy link
Member

nateberkopec commented May 8, 2020

Are you ok with adding methods that will only be available with OpenSSL 1.1.0 or later?

So then IMO the answer is "only if you can still run Puma with SSL on on ruby 2.3+". Adding things in a backwards-compatible way or gating new features behind OpenSSL 1.1.0 is IMO fine.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented May 8, 2020

Adding things in a backwards-compatible way or gating new features behind OpenSSL 1.1.0 is IMO fine.

Just to be clear, if one writes code making use of the new features (using a Ruby compiled with OpenSSL 1.1.1), then one tries to run the same code with an older version of OpenSSL, it's ok that it won't run, since the methods won't be defined?

BTW, I don't recall how much this affects the changes I'd like to make to the SSL error handling.

@nateberkopec
Copy link
Member

nateberkopec commented May 10, 2020

it's ok that it won't run

Maybe even stronger, perhaps, we should blow up and exception in that situation. If you say something in your config explicitly that uses Open SSL 1.1+, and then in production you only have 1.0 available at runtime, we should probably kick and scream about that.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented May 10, 2020

I got a little side tracked with MSYS2 jumping from gcc 9.2.0 to 10.1.0 this weekend...

On the one hand, I agree. On the other hand, if someone is touching a production server and they don't understand the importance of having similar OpenSSL versions for development and production, maybe they need to learn a lesson?

Anyway, we could set it up so the methods that require 1.1.1 are undefined if compiled with 1.0?

@nateberkopec
Copy link
Member

nateberkopec commented May 11, 2020

Anyway, we could set it up so the methods that require 1.1.1 are undefined if compiled with 1.0?

Sure, let's give it a shot.

1. Add method Puma::MiniSSL::Engine#ssl_vers_st.  This returns connection protocol version and SSL_state_string info.

2. Add 12 bit mask for ssl erors of type SSL_ERROR_SSL that do not involve certificate verification.  This translates numbers suffixing error message to match numbers in OpenSSL's 'SSL reason codes' defined  include/openssl/sslerr.h
@MSP-Greg MSP-Greg force-pushed the MSP-Greg:ssl-vers branch from 83bbe7d to 315d76a May 13, 2020
MSP-Greg added 2 commits May 13, 2020
Changes to Puma::MiniSSL

1. Add HAS_TLS1_3 constant.
2. Add #bad_tlsv1_3? method, used to determine if an http  connection to an https server has been made.  TLSv1.3 behaves differently than previous TLS versions.
3. Change #engine_read_all to close http connections.
4. Add #ssl_version_state method, unused at present.
Add History.md item
@MSP-Greg MSP-Greg force-pushed the MSP-Greg:ssl-vers branch from 315d76a to 1f439c0 May 13, 2020
@MSP-Greg MSP-Greg marked this pull request as ready for review May 13, 2020
@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented May 13, 2020

  1. Yanked out some code that wasn't needed.
  2. macOS JRuby failure is a stable test issue?
  3. Haven't rerun PR #2117, but it failed locally without the lib/ext code changes.
@nateberkopec nateberkopec merged commit 91e57f4 into puma:master May 15, 2020
26 of 27 checks passed
26 of 27 checks passed
ubuntu 2.2
Details
build
Details
ubuntu 2.3
Details
ubuntu 2.4
Details
ubuntu 2.5
Details
ubuntu 2.6
Details
ubuntu 2.7
Details
ubuntu head
Details
ubuntu jruby
Details
ubuntu truffleruby-head
Details
macos 2.2
Details
macos 2.3
Details
macos 2.4
Details
macos 2.5
Details
macos 2.6
Details
macos 2.7
Details
macos head
Details
macos jruby macos jruby
Details
macos truffleruby-head
Details
windows 2.2
Details
windows 2.3
Details
windows 2.4
Details
windows 2.5
Details
windows 2.6
Details
windows 2.7
Details
windows mingw
Details
ubuntu jruby-head
Details
@nateberkopec
Copy link
Member

nateberkopec commented May 15, 2020

JRuby definitely a stability issue, made a new issue about it

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

Successfully merging this pull request may close these issues.

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