-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Use Better Default Ciphers for the SSL Module #65194
Comments
As of right now the default cipher list for the ssl module is DEFAULT:!aNULL:!eNULL:!LOW:!EXPORT:!SSLv2, additionally on Python 3.4 when you use create_default_context() then you also additionally get HIGH:!aNULL:!RC4:!DSS. I think we should change this to the cipher string: ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:DH+AES:ECDH+3DES:DH+3DES:RSA+AESGCM:RSA+AES:RSA+3DES:!aNULL:!MD5:!DSS This will:
This cipher string is taken from urllib3 where it was compiled through the resources of:
The compatibility of this is pretty good. The only time this should cause a connection to *fail* is if a server is using an insecure cipher and in that case you can re-enable it by simply passing the original cipher list through the ssl.wrap_socket ciphers function. |
I really don't think hardcoding specific ciphers is a good idea. |
On 20.03.2014 15:11, Donald Stufft wrote:
Depends on who "you" is :-) Most of the time this will be the user of I think we should leave this decision to the OpenSSL lib vendors |
create_default_context is about best practices, though, so it seems to me it wouldn't be crazy to do it there. |
Agreed, but the real problem here is maintenance. Hardcoding a list of I'd prefer an open-ended cipher string. Here is a proposal: It prioritizes Diffie-Hellman key exchange (for perfect forward BTW, apparently removing RC4 prevents ECDHE in SSv23 mode: $ ./python -c 'import ssl, socket; ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23); ctx.set_ciphers("EECDH:EDH:AESGCM:HIGH:!eNULL:!aNULL"); s = ctx.wrap_socket(socket.socket()); s.connect(("linuxfr.org", 443)); print(s.cipher()); s.close()'
('ECDHE-RSA-RC4-SHA', 'TLSv1/SSLv3', 128)
$ ./python -c 'import ssl, socket; ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23); ctx.set_ciphers("EECDH:EDH:AESGCM:HIGH:!eNULL:!aNULL:!RC4"); s = ctx.wrap_socket(socket.socket()); s.connect(("linuxfr.org", 443)); print(s.cipher()); s.close()'
('DHE-RSA-AES256-SHA', 'TLSv1/SSLv3', 256) |
That's because of the set of ciphersuites offered by the server (see https://www.ssllabs.com/ssltest/analyze.html?d=linuxfr.org), it's not an inevitable property of TLS. For example jenkins.cryptography.io (see https://www.ssllabs.com/ssltest/analyze.html?d=jenkins.cryptography.io) offers ECDHE suites without any RC4 at all. |
Yea I noticed that, so I was doing some more testing, here's what I think we should be using (It Adds back in RC4): ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:DH+AES:ECDH+3DES:DH+3DES:RSA+AESGCM:RSA+AES:RSA+3DES:ECDH+RC4:DH+RC4:RSA+RC4!aNULL:!MD5:!DSS This gives us everything that DEFAULT:!aNULL:!eNULL:!LOW:!EXPORT:!SSLv2 does except for the ciphers list here https://gist.github.com/dstufft/251dbeb8962e2182e668 on my OpenSSL 1.0.1f install. Antoine, your cipher string priortizes ECDHE RC4 over DHE AES or even just plain AES. The string I'm proposing has been carefully crafted in order to get the ciphers in a very particular order. That order is basically - 1) Security of the cipher itself 2) PFS 3) Performance while also maintaining compatibility both forwards and backwards. RC4 is in a precarious condition and it's use should be heavily discouraged. It is still required in some cases which is why my revised default cipher suggestion includes it, but at the end as a last fall back. At that point if RC4 gets selected it's the servers fault and the client did everything it could except refuse. I still do believe that this should be the default ciphers while my original string should be the "restricted" ciphers that create_default_context() uses. |
In terms of following closely, I'd be willing to encourage Red Hat's SRT to |
Another bit of maintenance here: If a new cipher suite is added to OpenSSL it won' be generally available for a long while so very few if any services are going to be willing to depend on *only* it. For the very rare and unlikely case that somebody does setup a service that requires some brand new cipher they can override this list easily. Using the default or the "wide" open strings are inherently more dangerous because of the wide range of OpenSSL's that are in production use. It's hard without auditing every version of OpenSSL to figure out what ciphers will be available in what circumstances. It also means that if OpenSSL adds a new cipher that ends up being insecure that it will be picked up automatically. Therefore the strings I've posted take the opinion that a whitelist is more secure than a blacklist and whitelist the cipher suites to a very specific set that happen to be best practices at this current time. The only *required* maintenance would be if one of the selected ciphers are found to be insecure. However that was already a required maintenance because (again) of the wide range of OpenSSL versions available and the fact that these strings don't *add* any new ciphers, only remove some and create an explicit priority. |
It's also worth noting that users appear to be FAR more likely to have an up to date Python than they are an up to date OpenSSL, meaning that if a change needs to be made, we're much better situated to get that disseminated to actual users than OpenSSL is |
I still think the ciphers list should be open-ended, i.e. have "HIGH" somewhere at the end. |
Why? At best users will get yet another secure algorithm and at worst they'll get an insecure algorithm. |
By the way:
This doesn't parse. If the system OpenSSL isn't maintained properly, it's not Python's job to workaround that. And we certainly don't have the required knowledge and dedication anyway. |
Please let's not have a repeat of https://bugs.ruby-lang.org/issues/9424, Python is in a better place to workaround that than anyone else. |
Please stop the FUD. I proposed an alternative, how is it insecure |
Oh and don't confuse me that I think Python's current situation is as bad as Ruby's was, but that attitude is dangerous and wrong :/ |
I'm still looking into what "HIGH" entails across all the various OpenSSLs that are in production that I can access. That "FUD" was responding to the attitude that it's not Python's job to do this. Python is exposing a security sensitive API, it is it's job. |
On 20.03.2014 23:36, Donald Stufft wrote:
I disagree. Python only provides an interface to OpenSSL, so the OpenSSL Maintaining system security is an easier and more scalable approach than By restricting the set of allowed ciphers you can also create the We shouldn't do this in Python for the same reason we're not including |
On 20.03.2014 21:52, Alex Gaynor wrote:
This depends a lot on the type of users you're looking at. Corporate |
Python is already changing the OpenSSL defaults, also you're advocating that
Again, Python is already forcing a set of ciphers. I don't know what sort of
Of course, any restriction does that, that's not reason to also allow aNULL
The difference here is that there are properly maintained alternatives to Python exposes this API, it's Python's job to properly secure it. |
I wonder why RedHat doesn't bother changing the defaults. |
I don't know why. Probably because the OpenSSL defaults are not intended to |
Ok Antoine I've looked around. Using a string like this: ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:DH+AES:ECDH+3DES:DH+3DES:RSA+AESGCM:RSA+AES:RSA+3DES:ECDH+RC4:DH+RC4:RSA+RC4:ECDH+HIGH:DH+HIGH:RSA+HIGH:!aNULL:!eNULL:!MD5:!DSS The only *additional* ciphers that get added from the use of HIGH are various Camellia ciphers. These ciphers are not known to be insecure at this point in time so as of right now this is not an insecure cipher string. However I still content that using HIGH in the cipherstring actually adds additional maintenance burden. In order to know if that cipherstring is still safe you must run it against every target OpenSSL you want to make secure to ensure that it doesn't allow a new cipher that doesn't meet the security strength that was attempted to be had with that cipherstring. If you use an explicit cipher string then you know exactly which cipher suites Python will use no matter what the OpenSSL claims is HIGH or not. This means that instead of having to monitor all the various OpenSSL versions for new ciphers you only have to periodically check that the suites that Python selected are still secure. Remember the "failure" mode for not having a cipher in the list is that a different cipher is selected unless there are no other ciphers. A New cipher being added to OpenSSL is not going to be the only cipher available in any meaningful timeframe. The "failure" mode for having a bad cipher in the list is possibly making the users of Python insecure. That's why an explicit approach is preferred over an open ended approach. Because you don't have to audit a moving target. |
Oh, additionally OpenSSL makes no promises what the meaning of "HIGH" will be in the future. So you can only look at what it means now and what it means in the past. OpenSSL is not a good library and it's unfortunate that they don't attempt to make people secure by default. |
Oh, Additionally Marc: Even if some system administrator or some system out there does patch their OpenSSL to actually be safe by default Python changing it's cipher string only adds to the potential security (or at worst does nothing). If even one system (of which there are legion) does not do that patch then Python changing it's ciphers will protect that user. The failure mode for a bad cipher is silent insecurity, the failure mode for not having a needed cipher is an obvious error. |
I think that is a bit reverse. The main configuration point for ciphers Besides, the ssl module doesn't promise a specific "security strength". |
The Python ssl module is used for servers and clients. Ideally servers will
These are not things that affect only paranoid people and expecting someone |
We should have specific defaults for servers in
Again: the point is maintenance later, not breakage now. |
Ok, well I don't agree that it's more maintenance later to be explicit and not include HIGH, but whatever it's not insecure at the moment so. Attached is a patch against 3.5 for folks to review. |
Yup :) Just being explicit in that! |
Ok, so I think the latest patch is mostly good but I don't understand why the "restricted ciphers" (again, misnomer) would ban RC4 (and DSS?). These are the ciphers used by higher-level client libs, and connection failures will confuse the hell out of people. |
Note: The RC4 and DSS exclusion existed previously on the restricted ciphers so we'd have to ask Christian why he did that. For me personally the restricted ciphers are intended to be best practice ciphers and that means no RC4. DSS here I'm kind of meh about the same way I was for the default ciphers. DSA has historically had problems with weak RNGs and as far as I'm aware no CA's actually issue DSS certificates. But I mostly left !DSS in the restricted set because Christian had it in originally. This might be a case where to really do "best practices" we need to diverge between client and server. For a server I definitely think putting RC4 in the cipher string is a bad thing. For clients it is not the greatest thing but it more closely matches what browsers do because there are a few services here and there which only expose RC4. |
Forgot to add! If you think splitting between "restricted" server and client ciphers I can split them like that and upload a new patch. |
I was about to open a separate issue for the server side. How about |
Not sure what you mean by client issue. Do you mean to keep RC4? |
Which "client issue"? Sorry, I've lost track :-) |
Er, I typed issue and meant usage. Right now the only difference between restricted ciphers and the default ciphers is restricted ciphers have no RC4 and no DSS. You wanted this issue limited to client changes and I'm not sure how to do that without enabling RC4/DSS for servers (which is a regression in the security of the restricted ciphers). I think if we want to make restricted ciphers apply only for servers that's OK but as this ticket doesn't change the restrictions (other than omitting SRP/PSK and SEED/IDEA) that there's no changes to be made here, it should be accepted and then another ticket for restricting the restricted ciphers to servers only? Or what did you have in mind? |
Hmm, fair enough, let's change them all at once here. Also, since I'll still open another issue for server-specific configuration: not the |
Shall we commit the new string for 3.5 for the time being? I'm currently working on a PEP to help define a policy for dealing with network security related issues/enhancements in maintenance branches, so I don't think we should touch those until we have that discussion on python-dev and get an explicit direction from Guido. |
(for the record and for the sake of comparison, Postfix's "high" security setting is "ALL:!EXPORT:!LOW:!MEDIUM:+RC4:@strength") |
The patch will also need updating the "Cipher selection" paragraph in ssl.rst, I think. |
I can add that. |
Hmm, I'm not sure what needs updated. The docs only say that ssl module disabled certain weak ciphers by default which is still the case. Was there some specific place or wording you were looking for? |
Well, the doc currently says: """Starting from Python 3.2.3, the context = ssl.SSLContext(ssl.PROTOCOL_TLSv1)
context.set_ciphers('HIGH:!aNULL:!eNULL')""" But after your changes, calling set_ciphers('HIGH:!aNULL:!eNULL') will actually weaken security, so this example should simply be removed (IMHO). |
Ah yes, I skipped over that looking for a place where we were detailing what ciphers were picked. Ok Thanks! |
Added the docs changes |
New changeset e9749a7aa958 by Donald Stufft in branch '3.4': |
New changeset 60f696488c4f by Donald Stufft in branch 'default': |
New changeset 869277faf3dc by Antoine Pitrou in branch '3.4': New changeset 3b81d1b3f9d1 by Antoine Pitrou in branch 'default': |
The buildbot failures should have been fixed by bpo-21015, should we close this one? |
Yes |
The cipher strings rely too much on AES for my taste. Imagine that ChaCha20Poly1305 or any other strong cipher suite is introduced to OpenSSL in the future. Enabling using general, and demoting using narrow terms, seems IMHO a better approach. For example:
|
The cipher string includes HIGH, so if ChaCha20Poly1305 or another cipher suite is added to OpenSSL it'll get included in the cipher string by default. So the major difference of what you suggest would be no longer prioritizing ciphers. However I would argue that would be bad. The priority exists so that we get the best possible cipher is as many situations as we possibly can. It doesn't mean that we'll get the best possible cipher in *every* single situation, but generally we will. To this ends it prioritizes: So if OpenSSL added ChaCha20Poly1305 it would fit into the priority after AES-GCM and AES-CBC. For any device that has hardware support for AES (AES-NI) AES-GCM is hands down a better choice of cipher. It is secure, has no issues in the spec itself, and it is *fast*, like 900MB/s for AES-128-GCM on a Sandy Bridge Xeon w/ AES-NI (ChaCha20Poly1305 got 500MB/s on the same hardware, however it is a 256bit cipher will AES-128-GCM is a 128 bit cipher). Using ChaCha20 on those devices would be a worse choice than AES-GCM. However on lower powered devices, such as smart phones, especially those without hardware support for AES, ChaCha20 really shines. A Galaxy Nexus can do AES-256-GCM at 20MB/s whereas it can do ChaCha20Poly1305 at 92MB/s (same phone). So in an ideal world, assuming ChaCha20 was implemented in OpenSSL, we'd adjust the default cipher string based on the hardware they are running on. However since we don't have the ability to do that then preferring AES (which we know on some systems will be much faster) over an unknown future cipher (which we have no knowledge of if it will be faster or not) is a much more reasonable choice. If at some point in the future OpenSSL gains ChaCha20Poly1305 support then these strings should probably change to put ChaCha20Poly1305 in between AES-GCM and AES-CBC because on any given the system the likelyhood that you want AES-GCM is still higher than ChaCha20, but the likelyhood you want ChaCha20 over AES-CBC is greater. It's also important to note that the server in any TLS communication is the end that picks exactly which cipher we select. Ideally all servers will be configured to have the strongest cipher first, and to prefer their own cipher order. In that case for the *client* side of a TLS connection the order of the ciphers doesn't matter and thus your string vs the implemented string has no difference in behavior. However if the server doesn't enforce their own preference for ciphers, then the difference will be that an undefined cipher will be selected (could be AESGCM, AESCBC, ChaCha20, or Camellia). On the server side of this, if you're using Python to terminate your TLS on the server side, the likelyhood that a server is running on a low powered device where the benefits of ChaCha20Poly1305 are the highest are pretty low and preferring AES-GCM is an even safer idea. |
I think performance isn't really relevant, except perhaps on very busy |
Well, if you factor out performance then ChaCha20Poly1305 and AES-GCM are more In general I agree that the performance of all of these are "good enough" that Hopefully what I was trying to achieve was provide some more context for markk so he'd hopefully be able to better understand why the string cipher calls out AES specifically before falling back to HIGH. |
Thanks for the detailed insight, Donald! And I certainly love the progress these changes here bring. :-) Perhaps limiting the scope to ChaCha20Poly1305 (»CCP«) has been a wrong approach of mine to explain my concerns: We should not refer to any particular cipher in those lists, and by that avoid to revisit the defaults at any point in the future.
But we know from experience with already established ciphers if and when to demote them. That said I don't insist on any changes. |
It would be great if we could rely on OpenSSL's ordering. It would be seriously fantastic. OpenSSL is best positioned to be able to do the right things, it's updated at the right times. It should be where we do this. Unfortunately the OpenSSL maintainers have utterly abdicated any responsibility for helping secure users, and has gone with poor defaults, obligating us to fill the hole. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: