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 HTTP::Server crash with self-signed certificate #6590

Merged
merged 2 commits into from Aug 25, 2018

Conversation

@bcardiff
Member

bcardiff commented Aug 22, 2018

Fixes #6577

  • Refactor how socket.accept? is called. handle_client is spawned after the io is returned.
    • It is assumed that socket.accept? wont raise and a STDOUT message is logged if that does not hold.
  • OpenSSL::SSL::Server#accept? is changed to not raise
  • This was optional but OpenSSL::SSL::Socket::Server will now call LibSSL.ssl_shutdown if the accept failed.

While reviewing the current state of HTTP::Server and recent PRs I have some doubts:

  1. #6304 split the behaviour of #sync. I am not sure if that change should have changed https://github.com/crystal-lang/crystal/blob/0.26.0/src/http/server.cr#L379 to also set read_buffering = false. I am not sure if forcing sync = false was for reading or writing. That sync = false was first introduced to fix #834 I am not sure if the performance improvement came from reads or writes changes. From #6304 description it seems that just unbuffered writes are wanted, so there is nothing to change.

  2. The sync = false is, since 0.26.0 applied after the handshake in the SSL, but in 0.25 it was applied before. I am not sure it that could be an issue or not. If so the following snippet might be required (assuming also that read_buffering = false is wanted)

--- a/src/openssl/ssl/socket.cr
+++ b/src/openssl/ssl/socket.cr
@@ -50,6 +50,11 @@ abstract class OpenSSL::SSL::Socket < IO
     def initialize(io, context : Context::Server = Context::Server.new, sync_close : Bool = false)
       super(io, context, sync_close)
 
+      if io.is_a?(IO::Buffered)
+        io.sync = false
+        io.read_buffering = false
+      end
+
       ret = LibSSL.ssl_accept(@ssl)
       unless ret == 1
         self.close
  1. If (2) is not needed, there is still an issue. The OpenSSL::SSL::Socket implementation, it is a IO::Buffered that wraps and IO, and does not forward the sync/read_buffering values to the wrapped io. Since the default tcp socket is buffered, the ssl socket might be unbuffered due to HTTP::Server#handle_client, but the wrapped tcp socket is still buffered. It also make little sense to have a IO::Buffered wrapped by another IO::Buffered, so I think (2) make sense since it turns off the buffering of the inner io.

So, I think the only think pending should be to apply (2) as is in this PR. But I wanted to double check my assumptions of (1) mainly and, of course, the rest of the PR.

@bcardiff bcardiff requested review from RX14 and ysbaddaden Aug 22, 2018

@bcardiff bcardiff added this to the 0.26.1 milestone Aug 22, 2018

if io
# a non nillable version of io is passed to spawn
_io = io

This comment has been minimized.

@Sija

Sija Aug 22, 2018

Contributor

Shoudn't io here be already non-nillable? IIRC if branch filters out Nil.

This comment has been minimized.

@bcardiff

bcardiff Aug 22, 2018

Member

You recall correctly but using spawn affects that rule.

This comment has been minimized.

@asterite

asterite Aug 22, 2018

Contributor

io is closured, so it always retains all types assigned to it. Basically #3093

This comment has been minimized.

@straight-shoota

straight-shoota Aug 22, 2018

Contributor

Strangely it actually compiles without this line...

This comment has been minimized.

@bcardiff

bcardiff Aug 22, 2018

Member

The || break before did the trick because the nil was transformed to a NoReturn.
That stop working in a version were I used spawn with block, but || break could return probably since there is no block involved in spawn, but it was a bit cryptic.

OpenSSL::SSL::Socket::Server.new(socket, @context, sync_close: @sync_close)
begin
OpenSSL::SSL::Socket::Server.new(socket, @context, sync_close: @sync_close)
rescue

This comment has been minimized.

@straight-shoota

straight-shoota Aug 22, 2018

Contributor

I don't think this method should silence any errors.

accept? should only return nil if the server is closed after invoking the method. If the SSL handshake fails, that's an error and should raise and need to be handled by the caller.

io = begin
socket.accept?
rescue e
::puts "#{socket.class}#accept? raised but it shouldn't"

This comment has been minimized.

@straight-shoota

straight-shoota Aug 22, 2018

Contributor

Global scope prefix is not really required here. Is it intentional?

This comment has been minimized.

@bcardiff

bcardiff Aug 22, 2018

Member

I've been puts debugging inside sockets 😅

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 22, 2018

I'd suggest to move the exception handling (printing to stdout) to a private method. This allows it to be overridden in a subclass and implement custom behaviour.

It should probably be the same as in Fiber#run to also print the backtrace.

e.inspect_with_backtrace STDERR
STDERR.flush
@bcardiff

This comment has been minimized.

Member

bcardiff commented Aug 22, 2018

@straight-shoota when a better logging will come that will change and the private method will not be needed.

Do you have any input regarding (1)/(2)/(3)?

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 22, 2018

Better logging is not here yet. And logging won't allow to treat the actual exception.

Additionally, a private method would help adding a spec for this. Something like:

private class CustomHTTPServer < HTTP::Server
  getter last_exception : Exception?
  private def handle_exception(@last_exception)
  end
end

it "handles exception during SSL handshake" do
  server = CustomHTTPServer.new {}
  server_context, _ = ssl_context_pair
  address = server.bind_tls "localhost", server_context

  server_done = false
  spawn do
    server.listen
    server_done = true
  end

  context = OpenSSL::SSL::Context::Client.new
  socket = TCPSocket.new(address.address, address.port)
  expect_raises(OpenSSL::SSL::Error, "certificate verify failed") do
    OpenSSL::SSL::Socket::Client.new(socket, context)
  end
  Fiber.yield

  server_done.should be_false
  server.last_exception.should_not be_nil
  server.last_exception.not_nil!.message.not_nil!.should contain("alert unknown ca")
end
@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 22, 2018

Regarding sync behaviour, I'm not sure about all the details... (side note: I think this entire API with sync and read_buffering needs to be overhauled to be more clearly understandable).

I suppose adding (2) is fine, but not sure if it is required.

However, I'd rather add it in SSL::Server#accept and #accept? instead of SSL::Socket::Server#initialize. The server is the place where an incoming connection needs to be setup. The SSL socket should not have to worry about that.

socket.accept?
rescue e
::puts "#{socket.class}#accept? raised but it shouldn't"
::puts e

This comment has been minimized.

@RX14

RX14 Aug 22, 2018

Member

e.inspect_with_backtrace(STDOUT).

This comment has been minimized.

@Sija

Sija Aug 22, 2018

Contributor

STDOUT -> STDERR

This comment has been minimized.

@RX14

RX14 Aug 22, 2018

Member

then the other line needs to be STDERR.puts

@bcardiff bcardiff force-pushed the bcardiff:fix/6577-ssl-crash branch 2 times, most recently from ac5ef13 to 5008b48 Aug 23, 2018

@bcardiff

This comment has been minimized.

Member

bcardiff commented Aug 23, 2018

I applied the suggestions and turn off the buffering in wrapped IO::Buffered IOs for all OpenSSL::SSL::Socket.

It's ready for another round of review.

end
if io
# a non nillable version of the closured io

This comment has been minimized.

@r00ster91

r00ster91 Aug 23, 2018

Contributor

Typo: nillable

This comment has been minimized.

@bcardiff

bcardiff Aug 24, 2018

Member

nillable is fine, it's not a proper word but the concept is clear.

@@ -83,6 +83,14 @@ abstract class OpenSSL::SSL::Socket < IO
unless @ssl
raise OpenSSL::Error.new("SSL_new")
end
# Since OpenSSL::SSL::Socket is buffered there make no
# sense to wrap a IO::Buffered with buffereing activated.

This comment has been minimized.

@r00ster91

r00ster91 Aug 23, 2018

Contributor

Typo: buffereing

context = OpenSSL::SSL::Context::Client.new
socket = TCPSocket.new(address.address, address.port)
expect_raises(OpenSSL::SSL::Error) do

This comment has been minimized.

@straight-shoota

straight-shoota Aug 23, 2018

Contributor

Is there a specific reason why you removed the specific message matcher certificate verify failed?

This comment has been minimized.

@bcardiff

bcardiff Aug 23, 2018

Member

because the spec was failing in linux, but it turns out that the exception is not even raised. Need to double check in a vm...

This comment has been minimized.

@straight-shoota

straight-shoota Aug 23, 2018

Contributor

The spec I posted worked on my ubuntu 16.04 on WSL.

Perhaps try adding a few more Fiber.yield to make sure the error handling code is really executed.

io = begin
socket.accept?
rescue e
self.handle_exception(e)

This comment has been minimized.

@straight-shoota

straight-shoota Aug 23, 2018

Contributor

Why self.?

@@ -83,6 +83,14 @@ abstract class OpenSSL::SSL::Socket < IO
unless @ssl
raise OpenSSL::Error.new("SSL_new")
end
# Since OpenSSL::SSL::Socket is buffered there make no

This comment has been minimized.

@straight-shoota

straight-shoota Aug 23, 2018

Contributor

it makes no sense or there is no sense

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 23, 2018

It seems the spec as in 5008b48 randomly fails on linux in about 30% of runs due to no apparent reason.

@bcardiff bcardiff force-pushed the bcardiff:fix/6577-ssl-crash branch 3 times, most recently from 459c9b3 to 1b7d510 Aug 24, 2018

@bcardiff

This comment has been minimized.

Member

bcardiff commented Aug 24, 2018

The spec was failing because in the openssl 1.0.2 installed in linux the client triggering the socket in the server. I change the spec to test the overall behavior of not taking down the server and keep able to response to requests.

I also rollbacked the change of calling SSL_shutdown if init failed because it was wrong. Ref: openssl/openssl#710

end
3.times do
3.times do

This comment has been minimized.

@j8r

j8r Aug 24, 2018

Contributor

Why 3.times'ing 3.times, HTTP::Client.get can't be included in a 9.times?

This comment has been minimized.

@bcardiff

bcardiff Aug 24, 2018

Member

Because I wanted multiple wrong calls together.

This comment has been minimized.

@jwoertink

jwoertink Aug 24, 2018

Contributor

Maybe add a comment right above it for that? I can see someone in 2 months submitting a PR "cleaning up some spec code" lol

This comment has been minimized.

@bcardiff

bcardiff Aug 24, 2018

Member

done

@bcardiff bcardiff force-pushed the bcardiff:fix/6577-ssl-crash branch from 1b7d510 to e254a5e Aug 24, 2018

@ysbaddaden

This comment has been minimized.

Member

ysbaddaden commented Aug 24, 2018

I believe OpenSSL has its own buffer for SSL sockets, so we should avoid to have one in Crystal neither read/writes on the underlying socket (set before handshake), and only sync=true on the SSL socket wrapper.

@bcardiff

This comment has been minimized.

Member

bcardiff commented Aug 25, 2018

Good catch @ysbaddaden. But that is not a blocker issue and can be deferred. The BIO_set_write_buf_size is not even binded and the ssl were made buffered at 32e4dd5 to fix an issue it seems.

@bcardiff bcardiff merged commit a56dcbe into crystal-lang:master Aug 25, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment