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

define ecdhe/ecdsa aes 128/256 cbc/sha mode as per rfc 4492 #96

Closed
wants to merge 1 commit into from

Conversation

jbremer
Copy link

@jbremer jbremer commented Feb 26, 2016

As per commit message it would be great to include this patch and preferably even a new alpha release. As I'm using tlslite-ng as part of httpreplay which in turn is used in Cuckoo Sandbox - where we see quite some analyses hitting this exact issue - it would be great if we can start using a new version of tlslite-ng without having to monkey patch our tlslite-ng==0.6.0a3 setup.

Thanks,
Jurriaan


This change is Reviewable

jbremer added a commit to hatching/httpreplay that referenced this pull request Feb 27, 2016
Until our pull request [1] comes through and a new alpha version of
tlslite is released.

tlsfuzzer/tlslite-ng#96
@tomato42
Copy link
Member

tomato42 commented Mar 4, 2016

Sorry for the long delay, DROWN kept me busy.

In general I have no problem with adding them, some minor issues:

  • I think they should be clearly marked as not supported (and a reason for the lack of support)
  • I don't see why not add in similar way the rest of ECDHE-ECDSA ciphers, especially the AES-128-GCM-SHA256 ones, especially if decrypting communication with google services is the goal
  • Generally the ciphers in the lists are added in the strength order, so they definitely should be put before the NULL or anon ciphers :)
  • I have workarounded the python 2.6 issue, so please rebase on current master

This doesn't actually add support for these TLS ciphersuites to tlslite,
but it does allow 3rd party libraries (e.g., httpreplay [1]) to decrypt
TLS streams using TLS master secrets which use these particular ciphers.
In practice we see for example https://encrypted.google.com/ using these
two ciphers.

Thanks to Maximilian Hils for pointing out that merely adding these
ciphersuites as being in existence is enough to actually decrypt them.

[1]: https://github.com/jbremer/httpreplay
@jbremer
Copy link
Author

jbremer commented Mar 4, 2016

Hi, thanks for the elaborate answer, and no need to worry about delays :)
I rebased the PR for you. I'm all for adding some more constants as well, the following reference could help in that matter https://tls.mbed.org/supported-ssl-ciphersuites.
Regarding documentation I'm not sure what the best way to do that would be, so I'll leave that up to you for now ;-)
Fair enough @ null/anon cipher suites..

@tomato42
Copy link
Member

tomato42 commented Mar 4, 2016

The official list is here: https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml

regarding documentation: a simple

# not implemented, ECDSA certificate support missing

would be sufficient above the definition of every cipher missing full support (just the name and ietfName entry, the arrays are good as is)

@tomato42
Copy link
Member

any chance for update?

@jbremer
Copy link
Author

jbremer commented Jun 30, 2016

Unfortunately I didn't put any more effort into this issue since the PR, provided it works as expected on my side :-)

@tomato42
Copy link
Member

if you want to manage out-of-tree patches, I won't stop you ;)

@jbremer
Copy link
Author

jbremer commented Jun 30, 2016

You make it sound like the most evil thing, but you're right :-(
Maybe in a couple of weeks from now - quite swamped as well. Sorry for that :)

@tomato42
Copy link
Member

ok, I'll remind you later if nobody picks up the ecdsa in general

@tomato42
Copy link
Member

In the end I needed it myself, take a look at #151.

Closing as #151 is a superset of this PR.

@tomato42 tomato42 closed this Jan 26, 2017
@jbremer
Copy link
Author

jbremer commented Jan 26, 2017

Looks good! Thanks :-) Your PR might also explain another issue to me, namely one with decrypting RC4 related TLS streams (namely that, as you stated, those ciphersuites are not supported). Will have to look into that at some point as well..

@tomato42
Copy link
Member

RC4 is supported, just not by default. But yes, at least one RC4 ciphersuite that should work was missing...

@jbremer
Copy link
Author

jbremer commented Jan 28, 2017

Ah, right. I'm sorry to ask this here, but what's required to be done to support RC4? With some quick grep's I see there's OpenSSL-based RC4 support as well as a Python version (which is probably slower but I'm fine with that). Thanks!

@tomato42
Copy link
Member

it's caused by this configuration: https://github.com/tomato42/tlslite-ng/blob/master/tlslite/handshakesettings.py#L20

so to change it, you need to add "rc4" to the HandshakeSettings.cipherNames array

@jbremer
Copy link
Author

jbremer commented Jan 29, 2017

Ah, I see, thanks for the information!

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.

2 participants