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

SSL v3 is vulnerable #487

Closed
kevinburke opened this issue Oct 15, 2014 · 16 comments
Closed

SSL v3 is vulnerable #487

kevinburke opened this issue Oct 15, 2014 · 16 comments

Comments

@kevinburke
Copy link
Contributor

kevinburke commented Oct 15, 2014

I'm trying to figure out whether this means that urllib3 is vulnerable.

  • In browsers, failed connections are retried with a different, weaker protocol, which may be SSL v3, which is why Google is freaking out a lot about this. I don't think urllib3 has this problem.
  • I am confused by the ssl_wrap_socket method in pyopenssl.py because it takes ssl_version=None as an argument. However the first line of the method calls
OpenSSL.SSL.Context(_openssl_versions[ssl_version])

But _openssl_versions[None] will raise a KeyError.

  • When it's run, CPython 2.7.8 and Pypy (2.7.6) use TLS v1.0 to secure the connection, at least according to howsmyssl.com. The only place I can see a default ssl version being set is in resolve_ssl_version which sets the version as PROTOCOL_SSLv23 if it's not explicitly set. I can't figure out where in the urllib3 code that TLS v1 gets set.
  • This issue suggests a possible path to disabling: SSLv2 should be disallowed #471 Though the code path suggests this would only work for environments with an SSLContext.

I'm still trying to work through all of this.

@kevinburke
Copy link
Contributor Author

I think this patch fixes things but I'm not very confident in it because I don't understand the underlying issues well enough, and I'm not sure what the effect will be on connections to servers that don't support TLS v1.

kevinburke/urllib3@shazow:master...no-ssl-v23

@Lukasa
Copy link
Contributor

Lukasa commented Oct 15, 2014

@kevinburke Your patch is a troubling one. Some problems:

  1. Sufficiently old versions of OpenSSL may not recognise the OP_NO_SSL* flags. I haven't checked, but you need to see what was installed with Python in the oldest supported Python version. It'll be at most 0.9.8h, and will probably be older.
  2. This breaks connections to servers that do not support anything higher than SSLv3, with no ability to fix them without repatching the entire method.
  3. Does PyOpenSSL really not make those two constants available?

@kevinburke
Copy link
Contributor Author

I was right not to be very confident :)

Re: 3, was just following the example of OP_NO_COMPRESSION right above it.

@shazow
Copy link
Member

shazow commented Oct 15, 2014

Does anyone know what the vulnerability implications are of a library client running into the exploit?

@kevinburke
Copy link
Contributor Author

According to @t-8ch the attacker would need to control some data sent by the client.

If the attacker can do that and the connection is ssl v3 then a middleman can read the message contents.

Another attack vector is dropping enough of an attempted tls v1 connection so that the connection is downgraded to ssl v3. I am not sure whether this behavior is supported in urllib3/openssl.

@sigmavirus24
Copy link
Contributor

Can we please have this discussion in exactly one place?

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@Lukasa
Copy link
Contributor

Lukasa commented Oct 15, 2014

I think there are two discussions. One is whether this should be done (yes), in urllib3. The other is how requests exposes it (user friendly).

@shazow
Copy link
Member

shazow commented Oct 15, 2014

Sorry I was not aware there is another discussion. :/

@sigmavirus24
Copy link
Contributor

So given our lowest Python version is 2.6, the best we can do is specify a strict version of TLSv1.0 (https://docs.python.org/2.6/library/ssl.html?highlight=ssl#ssl.PROTOCOL_TLSv1). 2.6 cannot handle anything newer than that. Further, we (according to Python's own docs) cannot rely on the TLSv1_{1,2} constants being there (or working) unless the user is on OpenSSL 1.0.1, i.e., we can't steal the constant value and try to make it work on 0.9.8 (which is what 2.6/2.7 will likely be stuck on). Do we want to force the ssl version to be TLSv1.0 then?

@Lukasa
Copy link
Contributor

Lukasa commented Oct 16, 2014

No. The ssl.PROTOCOL_SSLv23 constant is incredibly poorly named, but it actually means 'negotiate the highest possible version'. If there are versions we don't want we have to use the OP_NO_SSLvx constants to do it.

@sigmavirus24
Copy link
Contributor

@Lukasa so we tell users on 2.6 and 2.7.x (for x < 9) that they have to explicitly remember to not use PROTOCOL_SSLv23 because it could negotiate down to SSLv3? There's no way to use the OP_NO_SSLv{y} constants on those versions (that we're all still supporting). I also don't think we can arbitrarily (reasonably) say that we no longer support 2.7.7 or 2.7.8 (when 2.7.9 finally arrives).

@Lukasa
Copy link
Contributor

Lukasa commented Oct 16, 2014

We could try. I don't like the solution either. =(

@kevinburke
Copy link
Contributor Author

Could we raise a warning if the connection ends up using ssl v3? Reading
the code yesterday made me think that urllib3 is not actually privy to that
information

Kevin Burke
phone: 925.271.7005 | twentymilliseconds.com

On Thu, Oct 16, 2014 at 12:01 PM, Cory Benfield notifications@github.com
wrote:

We could try. I don't like the solution either. =(


Reply to this email directly or view it on GitHub
#487 (comment).

@Lukasa
Copy link
Contributor

Lukasa commented Oct 16, 2014

I don't believe the standard library makes it possible to determine that. PyOpenSSL does though.

@eriol
Copy link

eriol commented Nov 20, 2014

Hello,
a side note: Debian disabled SSL version 3 protocol (in 2.7.8-12[¹]), so to make urllib3.contrib.pyopenssl not break I added this patch in the Debian package:

--- a/urllib3/contrib/pyopenssl.py
+++ b/urllib3/contrib/pyopenssl.py
@@ -70,9 +70,14 @@
 # Map from urllib3 to PyOpenSSL compatible parameter-values.
 _openssl_versions = {
     ssl.PROTOCOL_SSLv23: OpenSSL.SSL.SSLv23_METHOD,
-    ssl.PROTOCOL_SSLv3: OpenSSL.SSL.SSLv3_METHOD,
     ssl.PROTOCOL_TLSv1: OpenSSL.SSL.TLSv1_METHOD,
 }
+
+try:
+    _openssl_versions.update({ssl.PROTOCOL_SSLv3: OpenSSL.SSL.SSLv3_METHOD})
+except AttributeError:
+    pass
+
 _openssl_verify = {
     ssl.CERT_NONE: OpenSSL.SSL.VERIFY_NONE,
     ssl.CERT_OPTIONAL: OpenSSL.SSL.VERIFY_PEER,

[¹] https://packages.qa.debian.org/p/python2.7/news/20141118T160603Z.html

@shazow
Copy link
Member

shazow commented Nov 20, 2014

@EriolV Thank you, looks good. We're aiming to remove support for it natively also.

rbrito pushed a commit to coursera-dl/coursera-dl that referenced this issue Mar 29, 2015
After a considerable amount of RTFM, I try to summarize here what I
discovered, namely, that the problem is actually a conjunction of many
things:

* Python 2.6 and 2.7.x (with x <= 8) don't define the constants
  `ssl.PROTOCOL_TLSv1_1` and `ssl.PROTOCOL_TLSv1_2`. [They were introduced
  with Python 2.7.9][0]

* Having TLS v1.1 and TLS v1.2 seem to only be available with sufficiently
  new versions of OpenSSL. Again, this is reading from [the Python docs][0].

* urllib3, as a result, doesn't seem to support explicitly specifying which
  which version of TLS to use, as far as I can see it (both by reading the
  reading the code and by reading
  urllib3/urllib3#487 (comment)).

[0]: https://docs.python.org/2/library/ssl.html?highlight=ssl#ssl.PROTOCOL_TLSv1

Therefore, I guess that the best that we can do so far is to only request
`ssl.PROTOCOL_TLSv1` to be used. While it seems that TLS v1.0 has issues, I
hope that the packages we depend on manage to find a way to negotiate higher
versions of TLS, so that we can change that accordingly in the future.

A not so good alternative to that would be to decide dynamically (and
monkeypatch) urllib3 to see which TLS protocols we have and what we can try
to convice it to be used.

Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants