Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#8189 defect closed fixed (fixed)

Test failures with OpenSSL 1.0.2f

Reported by: Tristan Seligmann Owned by: Tristan Seligmann
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/openssl-f-8189-2
branch-diff, diff-cov, branch-cov, buildbot
Author: mithrandi

Description (last modified by Tristan Seligmann)

I have very little clue what's going on here yet, but here are the failures (which do not happen with OpenSSL 1.0.2e):

===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/home/mithrandi/code/Twisted/twisted/test/test_sslverify.py", line 1820, in test_surpriseFromInfoCallback
    buggyInfoCallback=True,
  File "/home/mithrandi/code/Twisted/twisted/test/test_sslverify.py", line 1579, in serviceIdentitySetup
    lambda: clientFactory.buildProtocol(None),
  File "/home/mithrandi/code/Twisted/twisted/test/iosim.py", line 429, in connectedServerAndClient
    return c, s, connect(s, sio, c, cio, debug)
  File "/home/mithrandi/code/Twisted/twisted/test/iosim.py", line 382, in connect
    clientProtocol.makeConnection(clientTransport)
  File "/home/mithrandi/code/Twisted/twisted/protocols/tls.py", line 322, in makeConnection
    self._tlsConnection.do_handshake()
  File "/home/mithrandi/deployment/virtualenvs/Twisted/local/lib/python2.7/site-packages/OpenSSL/SSL.py", line 1442, in do_handshake
    self._raise_ssl_error(self._ssl, result)
  File "/home/mithrandi/deployment/virtualenvs/Twisted/local/lib/python2.7/site-packages/OpenSSL/SSL.py", line 1187, in _raise_ssl_error
    _raise_current_error()
  File "/home/mithrandi/deployment/virtualenvs/Twisted/local/lib/python2.7/site-packages/OpenSSL/_util.py", line 48, in exception_from_error_queue
    raise exception_type(errors)
OpenSSL.SSL.Error: [('SSL routines', 'ssl_undefined_function', 'called a function you should not call')]

twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/home/mithrandi/code/Twisted/twisted/internet/_sslverify.py", line 1154, in infoCallback
    return wrapped(connection, where, ret)
  File "/home/mithrandi/code/Twisted/twisted/test/test_sslverify.py", line 1528, in broken
    1 / 0
exceptions.ZeroDivisionError: division by zero

(There are another bunch of tests that fail due to timeouts, probably from the same underlying cause)

Change History (18)

comment:1 Changed 4 years ago by Cory Benfield

Just a quick note: this reproduces on OS X using OpenSSL 1.0.2f. Presumably something nasty slipped into OpenSSL's releases.

I should also note that if I leave it long enough the interpreter eventually segfaults:

twisted.test.test_sslverify
  ServiceIdentityTests
    test_surpriseFromInfoCallback ...                                   [ERROR]
                                  [ERROR]
                                  [ERROR]
                                  [ERROR]
                                  [ERROR]
                                  [ERROR]

===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/Users/cory/Documents/Python/Twisted/twisted/test/test_sslverify.py", line 1820, in test_surpriseFromInfoCallback
    buggyInfoCallback=True,
  File "/Users/cory/Documents/Python/Twisted/twisted/test/test_sslverify.py", line 1579, in serviceIdentitySetup
    lambda: clientFactory.buildProtocol(None),
  File "/Users/cory/Documents/Python/Twisted/twisted/test/iosim.py", line 429, in connectedServerAndClient
    return c, s, connect(s, sio, c, cio, debug)
  File "/Users/cory/Documents/Python/Twisted/twisted/test/iosim.py", line 382, in connect
    clientProtocol.makeConnection(clientTransport)
  File "/Users/cory/Documents/Python/Twisted/twisted/protocols/tls.py", line 322, in makeConnection
    self._tlsConnection.do_handshake()
  File "/Users/cory/Documents/Python/Twisted/build/lib/python2.7/site-packages/OpenSSL/SSL.py", line 1442, in do_handshake
    self._raise_ssl_error(self._ssl, result)
  File "/Users/cory/Documents/Python/Twisted/build/lib/python2.7/site-packages/OpenSSL/SSL.py", line 1187, in _raise_ssl_error
    _raise_current_error()
  File "/Users/cory/Documents/Python/Twisted/build/lib/python2.7/site-packages/OpenSSL/_util.py", line 48, in exception_from_error_queue
    raise exception_type(errors)
OpenSSL.SSL.Error: [('SSL routines', 'ssl_undefined_function', 'called a function you should not call')]

twisted.test.test_sslverify.ServiceIdentityTests.test_surpriseFromInfoCallback
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/Users/cory/Documents/Python/Twisted/twisted/internet/_sslverify.py", line 1154, in infoCallback
    return wrapped(connection, where, ret)
  File "/Users/cory/Documents/Python/Twisted/twisted/test/test_sslverify.py", line 1528, in broken
    1 / 0
exceptions.ZeroDivisionError: division by zero

<not in test>
<not in test>
<not in test>
<not in test>
<not in test>
-------------------------------------------------------------------------------
Ran 1 tests in 0.160s

FAILED (errors=6)
[1]    22684 segmentation fault  ./bin/trial 

comment:2 Changed 4 years ago by Tristan Seligmann

Description: modified (diff)

comment:3 Changed 4 years ago by Tristan Seligmann

Description: modified (diff)
Summary: Test failures with Debian / OpenSSL 1.0.2f-2Test failures with OpenSSL 1.0.2f

comment:4 Changed 4 years ago by Glyph

This caused some test-failure annoyances on #7413

comment:5 Changed 4 years ago by Glyph

Also #5642; I guess this is going to keep failing on every new ticket?

comment:6 Changed 4 years ago by Tristan Seligmann

Any builder that upgrades to OpenSSL 1.0.2f or installs cryptography 1.2.2+ on a platform where it is statically linked against OpenSSL (ie. OS X and Windows) will be affected. We could downgrade the OS X builder temporarily, I guess; on the other hand, actually fixing this would be nice (but I still have no clue what's going on).

comment:7 Changed 4 years ago by Tristan Seligmann

I bisected OpenSSL, and it seems the patch that breaks this test is: https://github.com/openssl/openssl/commit/f73c737c7ac908c5d6407c419769123392a3b0a9

Somewhat baffling, since I'm not sure why that should produce ssl_undefined_function rather than, say, shutdown while in init. Actually, I get OpenSSL.SSL.Error: [] with my self-built LD_PRELOADed OpenSSL, which is even more weird.

comment:8 Changed 4 years ago by Tristan Seligmann

I should note that this problem occurs (on Linux, anyway) with all combinations of cryptography {master,1.2.2,1.1.2} and cffi {1.5.0,1.3.1}, so I don't think it's related to the static callbacks mess either.

comment:9 Changed 4 years ago by Tristan Seligmann

Okay, I think I understand the problem now. The info callback can be called during init, before the handshake has completed, and that OpenSSL patch means it is no longer safe to call shutdown at that point (previously, the conditional would just make the shutdown call do nothing, I think?). The normal hostname verification callback only tries to shut down the connection if the state is SSL_CB_HANDSHAKE_DONE, but the buggy one always blows up, and _tolerateErrors always tries to abort the connection.

comment:10 Changed 4 years ago by Tristan Seligmann

In fact, the problem is not that it isn't safe to call shutdown, but rather that it isn't safe to try to handshake after calling shutdown; patch incoming that seems to deal with this.

comment:11 Changed 4 years ago by Tristan Seligmann

Author: mithrandi
Branch: branches/openssl-f-8189

(In [46737]) Branching to openssl-f-8189.

comment:12 Changed 4 years ago by Tristan Seligmann

Keywords: review added

It's done... sort of. Builders are mostly green: https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/openssl-f-8189

The Windows builders have an unrelated problem, and the OS X builder crashes at the end, probably due to https://bitbucket.org/cffi/cffi/issues/246/crash-during-interpreter-shutdown-when.

You'll notice that I had to change the tests to write before calling loseConnection; it seems that if you try to shut down before you've ever written to the connection, the handshake is *never* done, and thus you just wait forever for shutdown to complete cleanly. I couldn't figure out any way to trigger the shutdown without writing first.

An alternative I considered was making loseConnection call abortConnection if the handshake wasn't complete yet, however loseConnection isn't supposed to lose data the way abortConnection does. Still, this might be better?

comment:13 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Tristan Seligmann

Thanks for fixing this very vexing bug, mithrandi.

  1. It would be nice if you could merge forward and make some new builds to pick up the cffi fix into the branch as well, to eliminate a potential failure distraction. (trunk is all green right now)
  2. There's a twistedchecker failure.
  3. I'm a bit concerned about the need to add the write-an-empty-string call. Aren't we skipping covering the case where we don't, now?

I think making loseConnection call abortConnection in that particular case is a fine compromise, since TLS actually mandates some stuff (which is impossible to prove about an implementation, but w/e) about dropping all buffers when you send a close_alert. In the case where the handshake isn't even done yet, you really shouldn't even have called write yet because you don't have an authenticated peer at that point.

Please address the above points, especially point 3, and resubmit for review; I want to be sure that we're not just dropping test coverage. (Also, be sure to file a ticket on the buildbots to upgrade cryptography again on one of them to ensure that this version is actually covered in the future; for the purposes of this ticket the reviewer should run the tests locally to be sure, since the buildbots are presently pinning at an earlier version due to these failures.)

comment:14 Changed 4 years ago by Tristan Seligmann

Branch: branches/openssl-f-8189branches/openssl-f-8189-2

(In [46827]) Branching to openssl-f-8189-2.

comment:15 Changed 4 years ago by Tristan Seligmann

Keywords: review added
Owner: Tristan Seligmann deleted

I believe all three points are addressed now, and builds are running:

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/openssl-f-8189-2

I also filed https://github.com/twisted-infra/braid/issues/188 to track unpinning Cryptography again.

comment:16 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Tristan Seligmann

Basically looks good.

But abortConnection is not quite the same thing as loseConnection, and we're playing a little fast and loose with interface definitions in the test fakes here; can you land, but file a followup to make a distinction between abortConnection and loseConnection in the test fakes in the future?

comment:17 Changed 4 years ago by Tristan Seligmann

Resolution: fixed
Status: newclosed

(In [46833]) Merge openssl-f-8189-2: Fix compatibility with OpenSSL 1.0.2f

Author: mithrandi Reviewer: glyph Fixes: #8189

OpenSSL 1.0.2f handles shutdown slightly differently to earlier versions; loseConnection() now calls abortConnection() in cases where the connection cannot be cleanly shut down.

comment:18 in reply to:  16 Changed 4 years ago by Tristan Seligmann

Replying to glyph:

But abortConnection is not quite the same thing as loseConnection, and we're playing a little fast and loose with interface definitions in the test fakes here; can you land, but file a followup to make a distinction between abortConnection and loseConnection in the test fakes in the future?

Neither of the test fakes that I modified buffer any data, so I'm not sure what distinction could exist between abortConnection and loseConnection?

Note: See TracTickets for help on using tickets.