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

Detect DROWN with NSE script sslv2-drown #365

Closed
wants to merge 12 commits into from

Conversation

bbc2
Copy link

@bbc2 bbc2 commented Apr 14, 2016

This adds the detection of severe SSLv2 vulnerabilities in OpenSSL (CVE-2015-3197, CVE-2016-0703 and CVE-2016-0800, also known as DROWN). The three CVEs are related and the result of the test should give an idea of how vulnerable the tested server is. I've tested the script with real servers from https://test.drownattack.com/ and relevant versions of OpenSSL s_server). Because they can be intrusive, those tests are only performed if the sslv2.extended-test script-arg is given. The default behavior of the script is mostly the same as before.

To deal with more than SSLv2 hello packets, I've had to refine the decoding and encoding of packets and I followed the draft spec for that. For instance, this should make record length computing a lot clearer. The script should also be easier to extend in the future.

I've mentioned this work on the mailing-list some time ago: http://seclists.org/nmap-dev/2016/q1/259 and as said there, I'd be happy to get feedback on this new version.

@dmiller-nmap
Copy link

Thanks for this good work! I like the idea of specific checks for the 3 SSLv2-related vulnerabilities, but I have 2 concerns with including them in the sslv2 script directly:

  1. Users are much less likely to use a feature of a script that requires using a script-arg to activate than if they can just add another term or wildcard to their --script option.
  2. Removing the "safe" category will prevent many users who currently restrict themselves to "safe" scripts from detecting SSLv2 at all.

I would like to see how this PR would look with the following changes:

  • Only simple enhancements to sslv2.nse if you have discovered problems with how it currently works.
  • A new script, sslv2-drown, for checking the forced-cipher and clear-key vulnerabilities. The regular sslv2 script could store discovered ciphers in the host registry, with sslv2-drown using dependencies to indicate that sslv2 should run first if it is selected, in order to avoid repeating the basic handshake if possible.
  • Use the vulns NSE library for reporting vulnerability info.

@bbc2 bbc2 force-pushed the cryptosense-sslv2 branch from f3cadd7 to cb4cf94 Compare May 11, 2016 07:10
@dmiller-nmap
Copy link

@bbc2 Be sure to let us know when you think this is ready for review again.

@bbc2
Copy link
Author

bbc2 commented May 11, 2016

OK! It's not ready yet.

bbc2 and others added 12 commits May 26, 2016 13:45
This fixes the XML output but leaves the human-readable output unchanged.
This is important if more than one SSL record is to be read, which will be the
case later.
This improves sslv2 with new data structures to make it easier to extend.

sslv2-drown is a new script copied from sslv2 and which is aimed at finding
DROWN-related vulnerabilities. See CVE-2015-3197 and [1] for more information
about the one detected with this commit.

[1]: https://www.openssl.org/news/secadv/20160128.txt
If not available, do the sslv2 test anyway.  It's required to check for
CVE-2015-3197.
@bbc2 bbc2 force-pushed the cryptosense-sslv2 branch from cb4cf94 to 2db5907 Compare May 26, 2016 15:44
@bbc2
Copy link
Author

bbc2 commented May 26, 2016

@dmiller-nmap It's ready for review. Thank you for your help!

I've hopefully addressed all your concerns. I don't know what is the best practice for NSE libraries but we could win a lot on code deduplication if the two scripts shared common SSLv2 structures and functions.

@bbc2 bbc2 changed the title NSE sslv2 upgrade with optional CVE tests Detect DROWN with NSE script sslv2-drown May 27, 2016
@dmiller-nmap
Copy link

Working on integrating this. Partially applied changes to sslv2.nse, along with some of my own improvements. Expect to have your DROWN script and possibly a sslv2 library committed in the next few days. Thanks so much for this great work!

@dmiller-nmap
Copy link

@bbc2 You list these ciphers as "weak enough to enable" DROWN:

-- Those ciphers are weak enough to enable a "General DROWN" attack.
local GENERAL_DROWN_CIPHERS = {
  [SSL_CK.RC2_128_CBC_EXPORT40_WITH_MD5] = true,
  [SSL_CK.RC4_128_EXPORT40_WITH_MD5] = true,
  [SSL_CK.DES_64_CBC_WITH_MD5] = true,
}

Why these and not SSL2_RC4_64_WITH_MD5? It also has an encrypted-key length of 8 (64 bits) like SSL2_DES_64_CBC_WITH_MD5.

On the other hand, the DROWN paper only mentions EXPORT ciphers (40 bits) as being vulnerable, so should SSL2_DES_64_CBC_WITH_MD5 be removed from the list?


-- CVE-2016-0703
for _, cipher in pairs(forced_ciphers) do
local result = has_extra_clear_bug(host, port, cipher)

Choose a reason for hiding this comment

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

@bbc2 Do we really need to try this for every discovered cipher? If the bug is there, it should show up no matter what the cipher we choose. I'll test it myself if you don't have further insight.

Copy link
Author

Choose a reason for hiding this comment

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

That's right, if the bug is there, it will stop at the first cipher suite with which it detected it (see the break below). However, it will try all cipher suites until it detects it, which might be unnecessary. I tried to remember why it does it like that but can't. It should be enough to only test the first cipher we get and determine whether the bug is there or not.

@bbc2
Copy link
Author

bbc2 commented Jul 7, 2016

I took the list of weak enough ciphers from an early version of the DROWN paper which listed them explicitly. In the published version it's less obvious but please see the last paragraph of section 4.1 (The SSLv2 export padding oracle):

If the server does not support 40-bit export ciphers, the attack can also be mounted in feasible computation time by choosing DES as the symmetric cipher.

Even though DES requires 64-bit keys, it only uses 56 bits from them, making it a lot weaker against bruteforce than other ciphers with 64-bit keys.

@nmap-bot nmap-bot closed this in b47c55d Jul 7, 2016
@bbc2
Copy link
Author

bbc2 commented Jul 7, 2016

Thanks for the review and the integration, it's awesome!

tremblerz pushed a commit to tremblerz/nmap that referenced this pull request Jul 20, 2016
tremblerz pushed a commit to tremblerz/nmap that referenced this pull request Jul 20, 2016
tremblerz pushed a commit to tremblerz/nmap that referenced this pull request Jul 21, 2016
tremblerz pushed a commit to tremblerz/nmap that referenced this pull request Jul 21, 2016
sergeykhegay pushed a commit to sergeykhegay/nmap that referenced this pull request Jul 27, 2016
batrick pushed a commit to batrick/nmap that referenced this pull request Aug 2, 2016
batrick pushed a commit to batrick/nmap that referenced this pull request Aug 2, 2016
git-svn-id: https://svn.nmap.org/nmap@35965 e0a8ed71-7df4-0310-8962-fdc924857419
@bbc2 bbc2 deleted the cryptosense-sslv2 branch September 1, 2016 10:19
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