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

Add a "PortableCipher" list that aims to be as compatible as possible to supplement the browser cipher lists #238

Merged
merged 9 commits into from
Feb 25, 2021

Conversation

mzpqnxow
Copy link
Contributor

@mzpqnxow mzpqnxow commented Dec 7, 2020

See #229 and for more detail, see #236

This is a very small and non-invasive patch that adds an additional static list of ciphers that are already implemented. The objective is to provide an easily referenced collection of cipher-suites for applications using zcrypto that is as compatible as possible given the current set of algorithms implemented in zcrypto.

This should have no impact on current users of zcrypto. If this is merged, it can be used by zgrab2 to more easily access a list of ciphers focused on portability via, e.g. --cipher-suite=portable, which would be added to the other existing options in the zgrab2 http module (firefox-only, chrome-only, safari-only, etc.)

The original "default" is kept the same. Also, in this new PortableCiphers list, for the majority of cases, the same cipher will be negotiated as would be when using the default as I started the list with the same elements in the default list, and then extended it by adding the additional ciphers, to minimize the risk of any unexpected issues for those that choose to use this over the default.

Against a specific real-world dataset that I work with regularly, which consists of ~50k HTTPS endpoints, there's a 2% increase in the amount of successful SSL/TLS negotiations compared with the default cipher list. This is relatively modest, but important depending on the use-case.

I also added a single cipher (TLS_RSA_WITH_AES_256_GCM_SHA384) to the ChromeCiphers list in this commit- this may not be appropriate, if it needs to be removed I can remove it- or feel free to do it in-place in the GH PR interface. The ChromeCiphers list is currently out of date anyway, so maybe there's not much harm leaving it in?

Thanks for your consideration!

Copy link
Member

@dadrian dadrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % notes on a few specific ciphers.

Comment on lines 1153 to 1163
TLS_DH_ANON_EXPORT_WITH_DES40_CBC_SHA,
TLS_DH_ANON_EXPORT_WITH_RC4_40_MD5,
TLS_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA,
TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA,
TLS_DHE_DSS_WITH_AES_128_CBC_SHA256,
TLS_DHE_DSS_WITH_AES_128_GCM_SHA256,
TLS_DHE_DSS_WITH_AES_256_CBC_SHA,
TLS_DHE_DSS_WITH_AES_256_CBC_SHA256,
TLS_DHE_DSS_WITH_AES_256_GCM_SHA384,
TLS_DHE_DSS_WITH_DES_CBC_SHA,
TLS_DHE_DSS_WITH_RC4_128_SHA,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the ANON or DSS suites are actually capable of completing a handshake.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've never actually even tried to use an ANON cipher-suite. Same as the comment below, I'll do testing across all of these and see what shakes out before reporting back with some additional commits with VERIFIED completed handshakes for each. Sorry, this was really careless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, can't complete a handshake with DSS certs, upcoming commit will reflect that

Comment on lines 1137 to 1142
TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA,
TLS_ECDH_RSA_WITH_AES_256_CBC_SHA384,
TLS_ECDH_RSA_WITH_AES_128_CBC_SHA256,
TLS_ECDH_RSA_WITH_AES_256_CBC_SHA,
TLS_ECDH_RSA_WITH_AES_128_CBC_SHA,
TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we actually support the non-ephemeral DH and ECDH suites.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm good point, I didn't actually test with endpoints preferring each. I just took from the list of implemented ciphers in that file. I'll rig something quick up and confirm that what you're saying is correct (and probably more important, confirm I didn't specify any others that won't actually work if they get chosen)

Comment on lines 1130 to 1133
TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA384,
TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA256,
TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA,
TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we actually support the non-ephemeral DH and ECDH suites.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, no non-ephemeral DH are supported. Working on the commit with all of the unsupported stuff removed

tls/cipher_suites.go Outdated Show resolved Hide resolved
@zakird
Copy link
Member

zakird commented Dec 8, 2020

This looks good to me once @dadrian's concerns are addressed. I agree that we shouldn't advertise ciphers we're unable to handshake for.

@mzpqnxow
Copy link
Contributor Author

Yeah, apologies for not actually testing to ensure that the handshakes can complete. I jumped to conclusions based on the implemented ciphers list. I'll confirm all of them before coming back :)

@dadrian
Copy link
Member

dadrian commented Dec 11, 2020

@mzpqnxow It's not unreasonable to expect the implemented ciphers list to contain ciphers that are implemented! 😆

@zakird
Copy link
Member

zakird commented Jan 24, 2021

Hey @mzpqnxow, I'd absolutely love to get this merged. Any chance you'd be willing to take a look at that list of ciphers?

@mzpqnxow
Copy link
Contributor Author

mzpqnxow commented Feb 6, 2021

Hey @zakird yes, I’m sorry this fell off of my map (or maybe I fell off of the map) I’d like to get to this in the upcoming week if you’re still interested

@zakird
Copy link
Member

zakird commented Feb 6, 2021 via email

zakird and others added 3 commits February 16, 2021 17:12
…le as well as unsupported suites from existing browser profiles (DSS, Non-anon DH, Non-ephemeral DH) and added annotations only for other unsupported suites.

I only actually removed (commented out) suites that were in browser profiles. Where unsupported suites existed elsewhere, I annotated with comments- not removing because they still may be useful, if only as constants/symbols.

The annotations can easily be removed if they're an eye-sore, just le met know.

I regret now not breaking this up into multiple commits May be worth retrying...
…com/mzpqnxow/zcrypto into feature/portable-cipher-suite-profile
@mzpqnxow
Copy link
Contributor Author

mzpqnxow commented Feb 18, 2021

@zakird @dadrian I just pushed a rather large commit onto the existing PR...

Unfortunately, I did this all as one commit, making it pretty hard for you to review as individual sections. If you'd prefer, I can revert to the old version and include only the removal of the unsupported suites from the PortableCiphers list that I added.

What is in this:

  1. Fixup to the PortableCiphers- commenting out the unsupported suites
  2. Fixups for any of the browser-specific suite lists, commenting out unsupported suites- in one or two cases, there were DSS or non-ephemeral DH ciphers that were listed above a few others. I don't know how you feel about that- if the goal is to look like those browsers over the wire and you're OK with the trade-off of potentially losing a few handshakes then you probably won't want this. Well, you get the idea, let me know if you prefer I uncomment them and leave them as is
  3. Annotations (comments) but not removal in a few of the suite lists that include unsupported suites. This is also easy for me to undo if you don't want the clutter all over the place, just let me know

If you get a chance, just let me know at a high level which (if any) of the three categories of changes made should be removed. Sorry I went a bit overboard but I figured since I was putting the effort in I might as well be as verbose and correct as possible. Not everyone agrees on that though :)

BTW- this came out of this effort... maybe useful in the future (but probably not): https://gist.github.com/mzpqnxow/b863ee8ea4b30c96c10f7ee4d101e887

EDIT: Also, forgot to mention- I spent a good deal of time confirming these. Though it's possible I made a mistake or two, I think it should be good, at least in terms of being accurate for what will complete a handshake successfully with zgrab2 http. Only those that won't work under any circumstances (e.g. even when specifying --cipher-suite <int16>) were commented out from the browser profiles

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

Successfully merging this pull request may close these issues.

4 participants