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 HeartBleed check functionality #3252

Merged
merged 2 commits into from Apr 17, 2014
Merged

Add HeartBleed check functionality #3252

merged 2 commits into from Apr 17, 2014

Conversation

@dchanm
Copy link
Contributor

dchanm commented Apr 13, 2014

This change allows for partial testing of HeartBleed without retrieving server memory. It can lead to false positives unlike the run() code path. The check() code path relies on vulnerable versions of OpenSSL returning a response for non-spec compliant Heartbeats, padding < 16 bytes. The minimum test case would be 6 bytes, 0x18 0x00 0x03 0x01 0x00 0x00

New versions of OpenSSL will discard the message. The actual check code sends a Heartbeat with payload of 5000 0x01 since I had some socket EOFErrors with only a couple bytes.

@nanotechz9l

This comment has been minimized.

Copy link

nanotechz9l commented on 6fafc10 Apr 12, 2014

Nice job. Did you send PR to msf master? I havent seen it committed yet. If so, might be in testing phase/refactoring limbo.

@musalbas

This comment has been minimized.

Copy link

musalbas commented Apr 13, 2014

I believe that increasing the payload length from 5000 to a value greater than 16384 should decrease false positives. At the moment this has a false positive with GnuTLS 3.2.0+ because it doesn't check for padding either, but it does check for the RFC-specified maximum padding length (16384), as seen here: https://gitorious.org/gnutls/gnutls/source/9654d4b90f0b2ebd300d402e615fff3f244593f6:lib/ext/heartbeat.c#L161

@dchanm

This comment has been minimized.

Copy link
Contributor Author

dchanm commented Apr 14, 2014

@musalbas We're currently looking in to better ways to reduce false positives. The linked code appears to be for sending a heartbeat request to a connected peer, but I'll look into it.

We had looked at sending a payload closer to 16KB on initial versions. However the TLS max record length of 16KB prevented us from specifying a payload longer than 16KB - 3 bytes. The basis of the test requires that we send as much data as actually claimed in payload_length but without padding.

@dchanm

This comment has been minimized.

Copy link
Contributor Author

dchanm commented Apr 14, 2014

@musalbas Thanks for the explanation! I wasn't aware of how to properly handle fragmented records. I looked at your code example and came up with a testcase that appears to work for the 3 situations. I updated the code to send a heartbeat with payload length of 16,381 then a heartbeat with payload length of 0.

Vulnerable versions of OpenSSL respond as you said. Fixed versions don't respond, I actually get a EOFError but no response as expected. GnuTLS 3.2.13 returns an ALERT record, since the second heartbeat has length < 4

@musalbas

This comment has been minimized.

Copy link

musalbas commented Apr 14, 2014

@dchan Great! Thanks. Glad to be of help.

(I deleted my original reply as it was confusing and was planning to rewrite it, but for other people's reference, it was discussing a way to send payloads greater than 16KB by splitting the payload into multiple TLS records, using the example at https://github.com/musalbas/heartbleed-masstest/blob/5fa1b7d4e4c7e1f41701cbef7cd3f108eb971da8/ssltest.py#L135, which is our implementation of @dchan's heartbleed test in Python.)

musalbas added a commit to musalbas/heartbleed-masstest that referenced this pull request Apr 15, 2014
…ere the TLS version was only embedded to the first heartbeat. 2) Update heartbeat request to reduce false positives with GnuTLS (see rapid7/metasploit-framework#3252 (comment)).
@musalbas

This comment has been minimized.

Copy link

musalbas commented Apr 15, 2014

I've implemented your new test here: https://github.com/musalbas/heartbleed-masstest/blob/8ab4ca7f43810b99cfb3ad82cb30a40b8435d365/ssltest.py#L146.

You may also be interested in the way it handles the TLS version. This supports TLS 1.2 as well was 1.1, because it sends a hello message specifying TLS 1.2, and sends a heartbeat with whatever TLS version the server supports if not TLS 1.2, resulting in less false negatives when testing for Heartbleed. See: musalbas/heartbleed-masstest#25.

(I'll look into submitting a pull request for that when this pull request gets merged.)

@todb-r7 todb-r7 added the heartbleed label Apr 17, 2014
vprint_error("#{peer} - Unexpected Heartbeat response")
vprint_status("#{peer} - Sending Heartbeat...")
if safe
sock.put(heartbeat(max_record_length - 3, safe) << heartbeat(0, safe))

This comment has been minimized.

Copy link
@jlee-r7

jlee-r7 Apr 17, 2014

Contributor

Please don't use String#<< in expressions, it gets confusing. + makes more sense here.

Also, a comment above this line explaining why this is 3 would be useful.

This comment has been minimized.

Copy link
@dchanm

dchanm Apr 17, 2014

Author Contributor

Sorry for the magic number. The -3 is to account for the 3 byte heartbeat header. OpenSSL complains if the total record length is > 16,384 unless the IE big buffer workaround is enabled.

@todb-r7 todb-r7 merged commit 1a73206 into rapid7:master Apr 17, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
todb-r7 added a commit that referenced this pull request Apr 17, 2014
@todb-r7

This comment has been minimized.

Copy link
Contributor

todb-r7 commented Apr 17, 2014

Works for me!

msf auxiliary(openssl_heartbleed) > check

[*] x.y.z.164:443 - Sending Client Hello...
[*] x.y.z.164:443 - Sending Heartbeat...
[*] x.y.z.164:443 - Heartbeat response, checking if there is data leaked...
[*] x.y.z.164:443 - The target appears to be vulnerable.
[*] Checked 1 of 1 hosts (100% complete)
msf auxiliary(openssl_heartbleed) > set RHOSTS x.y.z.204
RHOSTS => x.y.z.204
msf auxiliary(openssl_heartbleed) > run

[*] x.y.z.204:443 - Sending Client Hello...
[*] x.y.z.204:443 - Sending Heartbeat...
[-] x.y.z.204:443 - No Heartbeat response...
[-] x.y.z.204:443 - Looks like there isn't leaked information...
[*] Scanned 1 of 1 hosts (100% complete)
[*] Auxiliary module execution completed
msf auxiliary(openssl_heartbleed) > 
@FireFart

This comment has been minimized.

Copy link
Contributor

FireFart commented Apr 17, 2014

Uff this merge will mess up with #3274 looks like we need to rebase again :)

@todb-r7

This comment has been minimized.

Copy link
Contributor

todb-r7 commented Apr 17, 2014

Sorry about that. I can untangle, shouldn't be too hard.

todb-r7 added a commit to todb-r7/metasploit-framework that referenced this pull request Apr 17, 2014
undo rapid7#3252 completely. This means a reimplementation of @dchan's work,
but his intent was simply to implement a check_host() that doesn't
actually pull memory, so that should be pretty straight forward with the
new structure of the module.
@dchanm

This comment has been minimized.

Copy link
Contributor Author

dchanm commented Apr 17, 2014

I'll pull once #3274 lands and refactor my code for that

@todb-r7

This comment has been minimized.

Copy link
Contributor

todb-r7 commented Apr 21, 2014

@dchan here's a reference script that the core ruby language people endorse. It looks significantly more simple than what you had (lots of constants):

https://github.com/emboss/heartbeat

We can't use it whole due to the reliance on standard Ruby sockets vs Rex::Socket, but maybe it'd be easier for a check to use this kind of simplified test? It'd be more atomic and would avoid having to track the fast-moving OpenSSL custom implementations (at the cost of some un-DRY code).

What do you think?

@dchanm

This comment has been minimized.

Copy link
Contributor Author

dchanm commented Apr 21, 2014

@todb-r7
PAYLOAD = "\x18\x03\x01\x00\x03\x01\x40\x00"
is the shortest TLS record required to support checking for heartbleed. The current scanner code in metasploit can already do this by passing in 16,384 (0x40 0x00) for heartbeat_length. I would say the primary difference between my test and emboss' / other heartbleed tests is that mine is safe to use in a webapp since no server memory is returned. The downside of mine is that it results in some false positives with non-OpenSSL libraries.

If the goal is to simply have a heartbleed check, then you already implemented emboss' check when untangling this pull with pull #3274 . I realize that metasploit isn't designed as a vulnerability scanner, so I defer to you as to what is best for the project.

@todb

This comment has been minimized.

Copy link
Contributor

todb commented Apr 21, 2014

Since you're using CheckCode::Appears, rather than Vulnerable, you're
afforded some wiggle room on the false positives. I'd rather attack
something that might be vulnerable rather than skip an opportunity based on
a miss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.