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

MSP-Greg · 2020-02-18T17:02:55Z

Description

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.

nateberkopec · 2020-03-05T17:24:23Z

lib/puma/minissl.rb

+      # 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 is because you didn't implement a JRuby version, right? Not because the JRuby SSL version should actually return different values?

Or our JRuby extension doesn't support TLS 1.3?

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.

Ah, got it.

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

@dentarg Yes

nateberkopec · 2020-03-05T17:25:33Z

Wondering what your response is to #2115 (comment)

MSP-Greg · 2020-03-05T17:37:53Z

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 · 2020-03-05T19:57:30Z

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:

Does it affect normal throughput?
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?

dentarg · 2020-04-10T06:08:33Z

lib/puma/minissl.rb

+          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 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)

@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 · 2020-05-08T00:19:07Z

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 · 2020-05-08T00:52:35Z

@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 · 2020-05-08T01:41:23Z

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

MSP-Greg · 2020-05-08T03:10:29Z

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 · 2020-05-08T03:40:15Z

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 · 2020-05-08T03:57:45Z

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 · 2020-05-10T23:11:48Z

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 · 2020-05-10T23:36:52Z

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 · 2020-05-11T00:12:38Z

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

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 · 2020-05-13T18:02:11Z

Yanked out some code that wasn't needed.
macOS JRuby failure is a stable test issue?
Haven't rerun PR Run test_http_rejection without code updates [changelog skip] #2117, but it failed locally without the lib/ext code changes.

nateberkopec · 2020-05-15T03:48:54Z

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

This was referenced Feb 18, 2020

Run test_http_rejection without code updates [changelog skip] #2117

Closed

Puma errors and performs badly when receiving a non-HTTPS request while TLSv1.3-configured #2115

Closed

nateberkopec added bug waiting-for-review Waiting on review from anyone labels Feb 20, 2020

nateberkopec reviewed Mar 5, 2020

View reviewed changes

nateberkopec added c-ext ssl waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Mar 5, 2020

dentarg reviewed Apr 10, 2020

View reviewed changes

MSP-Greg force-pushed the ssl-vers branch from 83bbe7d to 315d76a Compare May 13, 2020 17:28

MSP-Greg added 2 commits May 13, 2020 12:37

test/test_puma_server_ssl.rb - add test_http_rejection

1f439c0

Add History.md item

MSP-Greg force-pushed the ssl-vers branch from 315d76a to 1f439c0 Compare May 13, 2020 17:42

MSP-Greg marked this pull request as ready for review May 13, 2020 17:44

nateberkopec merged commit 91e57f4 into puma:master May 15, 2020

MSP-Greg deleted the ssl-vers branch September 20, 2020 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MSP-Greg commented Feb 18, 2020

nateberkopec Mar 5, 2020

nateberkopec Mar 5, 2020

MSP-Greg Mar 5, 2020 •

edited

nateberkopec Mar 5, 2020

dentarg Mar 5, 2020

MSP-Greg Mar 5, 2020

nateberkopec commented Mar 5, 2020 •

edited

MSP-Greg commented Mar 5, 2020

MSP-Greg commented Mar 5, 2020

dentarg Apr 10, 2020

MSP-Greg Apr 10, 2020

nateberkopec commented May 8, 2020

MSP-Greg commented May 8, 2020

nateberkopec commented May 8, 2020

MSP-Greg commented May 8, 2020

nateberkopec commented May 8, 2020

MSP-Greg commented May 8, 2020

nateberkopec commented May 10, 2020

MSP-Greg commented May 10, 2020

nateberkopec commented May 11, 2020

MSP-Greg commented May 13, 2020

nateberkopec commented May 15, 2020

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

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

Conversation

MSP-Greg commented Feb 18, 2020

Description

nateberkopec Mar 5, 2020

Choose a reason for hiding this comment

nateberkopec Mar 5, 2020

Choose a reason for hiding this comment

MSP-Greg Mar 5, 2020 • edited

Choose a reason for hiding this comment

nateberkopec Mar 5, 2020

Choose a reason for hiding this comment

dentarg Mar 5, 2020

Choose a reason for hiding this comment

MSP-Greg Mar 5, 2020

Choose a reason for hiding this comment

nateberkopec commented Mar 5, 2020 • edited

MSP-Greg commented Mar 5, 2020

MSP-Greg commented Mar 5, 2020

dentarg Apr 10, 2020

Choose a reason for hiding this comment

MSP-Greg Apr 10, 2020

Choose a reason for hiding this comment

nateberkopec commented May 8, 2020

MSP-Greg commented May 8, 2020

nateberkopec commented May 8, 2020

MSP-Greg commented May 8, 2020

nateberkopec commented May 8, 2020

MSP-Greg commented May 8, 2020

nateberkopec commented May 10, 2020

MSP-Greg commented May 10, 2020

nateberkopec commented May 11, 2020

MSP-Greg commented May 13, 2020

nateberkopec commented May 15, 2020

MSP-Greg Mar 5, 2020 •

edited

nateberkopec commented Mar 5, 2020 •

edited