-
Notifications
You must be signed in to change notification settings - Fork 14k
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
Remove OpenSSL from Windows Meterp, packet header changes, and TLV packet encryption #8625
Remove OpenSSL from Windows Meterp, packet header changes, and TLV packet encryption #8625
Conversation
This moves some of the packet-specific stuff to the packet class itself
This means that meterpreter instances that don't support will continue to work.
Both extended and verbose session logging will show which of the sessions has the encryption enabled as it's not yet supported on all sessions.
# in blocking the entire process. | ||
begin | ||
Timeout.timeout(timeout) do | ||
# Renegotiate SSL over this socket |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a simpler way to do this, since the 'reverse_tcp_ssl' payloads do SSL from the start anyway, even the stager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could well be! I think that'd be a good thing to revisit for sure.
I'll fix up payload cached sizes and Metasploit Payloads gem versions when the payloads have been landed. |
lib/rex/post/meterpreter/packet.rb
Outdated
def aes_decrypt(aes_key, iv, data) | ||
# Create the required cipher instance | ||
aes = OpenSSL::Cipher.new('AES-256-CBC') | ||
# Generate a truly random IV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copypaste leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks :)
lib/rex/post/meterpreter/packet.rb
Outdated
xor_key = self.raw[XOR_KEY_OFF, XOR_KEY_SIZE] | ||
data = xor_key + xor_bytes(xor_key, self.raw[SESSION_GUID_OFF, self.raw.length - SESSION_GUID_OFF]) | ||
self.session_guid = data[SESSION_GUID_OFF, SESSION_GUID_SIZE] | ||
encrypted_flag = data[ENCRYPTED_FLAG_OFF, ENCRYPTED_FLAG_SIZE] == "\x01" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good place to mention - is it worth designating the ENCRYPTED_FLAG
field as FLAGS
with the bottom bit being ENCRYPTED_FLAG
and the other 7 bits being reserved? Will 7 be enough for future use? In which case we should use boolean logic to extract the bottom bit rather than just == "\x01"
to pave the way for general flag field use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, no. At the moment it's just a single bit. But it'll actually become an identifier which represents which encryption scheme is in use (such as something cheaper to run on smaller processors). So it'll stay as is for now :) Then later we'll probs change it to something indicative of the encryption scheme. Such as ENCRYPTION_SCHEME
(where 00
will be none, and 01
will be AES256.. etc. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh got it :)
lib/rex/post/meterpreter/packet.rb
Outdated
packet_length = data[PACKET_LENGTH_OFF, PACKET_LENGTH_SIZE] | ||
packet_type = data[PACKET_TYPE_OFF, PACKET_TYPE_SIZE] | ||
self.type = packet_type.unpack('N')[0] | ||
if encrypted_flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it an issue that we're trusting packets to essentially declare whether the session is under encryption? I think it allows a passive MitM to spoof TLV responses. Would "knowing" that a session is under encryption and that all TLV's must arrive encrypted provide a measure of client authentication? Will noodle if it's worth considering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. The flag is an indication of whether incoming packets have been encrypted. If MITM switches the flag, then it doesn't decloak anything. If the endpoints have keys, they encrypt stuff.
We have to trust something in the protocol, this flag is there to 1) give us an idea that we can't just XOR, and 2) (in future) tell is which scheme to use.
So I'm comfortable with this flag being MITM-able, because there's no risk to current or future comms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. But a passive MitM who knows the session GUID can spoof TLV packets that say "is ok, this packet isn't encrypted so you have no way of knowing that it came from the real client, don't bother decrypting it. Here have a fistful of cleartext TLV's maybe one of them is relevant to you right now?" Which would violate the integrity of the session?
We have to trust something in the protocol
Do we? Can't we track whether the session pertaining to a particular GUID is expected to be encrypted?
You know the architecture and the future plans, I'm just throwing ideas out there. I'm guessing there must be a reason why we can't go "Ah this GUID pertains to a session that I expect to get encrypted TLV's for. This packet has !encrypted_flag
chuck it away."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are scenarios, as per the PR description, where plain text TLVs are needed. There may be more.
The whole "trust the protocol" thing falls back into the "you have to trust something" category that I've already discussed. If you can't trust that byte, you can't trust any bytes. you can't trust the session GUID, or the length, or anything. So yes, I think trusting that byte is ok as far as an active 'toggle this byte' MITM is concerned.
I get the concern for having an unencrypted TLV during an encrypted session though, as far as passive replays go. It may be enough for MSF to say "if I have a key, I expect encryption" and go from there.
This will have to be considered more before settling on it though. Good question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I'm thinking, doing away with trusting the encrypted_flag
for sessions that we expect encrypted packets for is a hard problem.
@OJ said:
After migration a session to another process, the negotiation phase needs to be re-run, because the AES key is not part of the configuration data (intentional). As a result, encrypt keys are cleared once migration is done and a new key is generated
This sounds like something a passive MitM can spoof at any stage and means we must not enforce the idea of "encrypted sessions must not play silly buggers with unencrypted packets". I shall noodle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a painful problem!
Is encrypted TLV replay a problem we should consider? |
Great question @justinsteven. Perhaps! But perhaps not. Might be worth having a pow-wow about this in an open forum like IRC or something to see whether this is a concern. If it is, it's also a concern for unencrypted packets, and for any other packet we're sending, ever. We should definitely discuss and consider the negatives. |
Of course
Is it though? e.g. |
@OJ: Working through it, had to convert named pipes to this, found out x64 doesnt like the HANDLE vs SOCKET piece :). Will get back with test results in a bit. One note on active MITM: we can encrypt symmetric keys in stage1 by using rc4 in stage0 such as to prevent acquisition on the wire. If stage0 is delivered out of band, or uses contextual data (hostname, domain name, a low-precision time value) to decrypt stage1, we should be able to significantly increase our posture against this vector. Basically stage0(shellcode which opens socket, recv, decrypt_in_place(via context key), load into memory) -> stage1(bring key and establish symmetric context, renegotiate under that context if needed). Migrations could bring the original key with them i suppose, or in the event of using context data for this, simply pull that ctx again (the time based approach could be a problem here, unless our stage handler keeps incrementing its clock too in order to maintain relative deltas which are acceptable). Manual memory analysis will catch this approach, but i think we can rule out autonomous defenses currently (or in the near future) being able to do so. Far as the "keep a certificate chain" piece, we may be able to use a simplified version of asymetric relationships to check a pair of small tokens instead of going through the rigmarole of a cert chain... |
@OJ: migration from 32->64 is hosed. 32->32 works fine. Attempting to tab-complete compatible sessions results in:
Not sure if this happens upstream, lemme confirm this one before wasting your time and sanity |
Interesting! I'll have to review that. I thought I'd tested that case.
Thanks!
…On Jun 28, 2017 08:36, "RageLtMan" ***@***.***> wrote:
@OJ <https://github.com/oj>: migration from 32->64 is hosed. 32->32 works
fine.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8625 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABw4PXzqsN0wsUAw-w05Ul5zLOscFU5ks5sIYPvgaJpZM4OGRF8>
.
|
@OJ: this is on a win7 x64 box if it matters Also, i broke something in the way named pipes handle things. Need to review that bit. |
@OJ: we might have a problem - https://pastebin.com/gUUDXUDs |
Thanks @sempervictus. Do you have a set of steps that I can follow to repro this please? |
I can confirm that this is indeed a problem, but has nothing to do with this PR. From my testing on master:
Investigating further, but as part of another PR, not this one :) |
I remove my concerns regarding stability of this code. Turns out my payloads repo didn't keep the commented out version of unhooking call from jefftangs PR. That was causing my migration failures. So far with the comments back in place I'm only seeing a ~10% fail rate. Still too high for master/pro, but rational. I'm also cleaning out old named pipes code as I adopt OJs current work from the packet pivot branch, which seems to be stabilizing things more as I go. I need to do proof passes before official approval, but for all purposes of framework, I confirm this works |
I think this is looking OK now to be honest. I haven't seen any issues with this that we didn't have with the current master. @bcook-r7 should I change this PR so that it points to the |
Also @bcook-r7 as far as mettle goes, the main area of pain is the packet flushing code which analyses TLVs on the fly. That's going to have to change in some way given that we'll have encryption in place so we'll need to do something similar to what I'm doing, which is a bit of a hack. Can we tag team the work on that please? |
Hi OJ - master is fine. Tag team engaged. |
Thanks! |
Quick progress report so this doesn't lose momentum. I'm about 50% through with the mettle bits (had an uninterrupted 3 hours on it yesterday, so I think the rest should come this weekend). I started updating the payloads branch a little too, because it has gotten a little convoluted with the merges. Once those bits are complete, we should be good to go. This is going to disable -aggregator for a while as we rethink how it will work. As I was poking, I noticed that the xor patterns are really easy to identify due the large number of zeros in the first packet. I'm contemplating augmenting the xor obfuscation with an rng, using the xor instead as a seed. |
Awesome, thank you sir. All on board with the xor piece, though that'll need to be implemented across the runtimes. Have some thoughts on aggregator and deep pivots, hoping to have a workable concept in the next few days.
|
@bcook-r7 I like that idea. Yes the first packet does indeed stand out given the NULL guid. The only concern would be making it consistent across platforms. We could easily solve that with a custom implementation though, right? It's not like it has to be cryptographically secure; it's just used for obfuscation. |
Yep. Even a simple 32-bit lfsr would be sufficient, can be implemented in a couple of lines of code added to the xor function. |
Got nailed again using Unrelated: I hate (un)packing in Ruby. |
note, bindata is now in tree and works a lot more like C structs than Ruby packing, if you're ever interested in doing some conversions :) mettle PR is up now too: rapid7/mettle#91 |
Oh that's good to know! Is there a good ref for that lying around? Thanks for helping with the mettle side mate. |
https://github.com/dmendel/bindata ruby_smb is done 100% with bindata, and I usually just bug @thelightcosine when I have questions or need to borrow examples. I converted a few modules to use it as well when we added Ruby 2.4 support. |
Takes a moment to get used to it, but a hell of a lot neater than the packing semantics. Both python and ruby effectively make the dev keep a symbol lookup table in their head. Not everyone is wired like hdm and egypt :).
|
This is an abstraction I can get behind for sure. It's easy to mess things up, and I'm tired of seeing the same block of code in multiple spots (I'm a culprit). |
OK, here goes nothing! We can continue stabilizing a bit in-tree as I just made a CYA release to cover any upcoming metasploit gem release needs. |
…hanges, and TLV packet encryption
This is awesome. Thanks so much @busterb !! |
Release NotesTransport-level encryption provided by OpenSSL has been replaced in some Meterpreters with application-level encryption provided per-message instead. This reduces the size of Windows Meterpreter in particular, while providing an encryption mechanism that is easier to implement in other Meterpreter implementations as well. |
Associated Metasploit Payloads PR: rapid7/metasploit-payloads#211
Associated Mettle PR: < coming shortly I promise, I'm not yet finished >
Those payloads will need to be built and tested alongside this if anything is going to work.
This PR supercedes the previous MSF side PR: #8570, which has now been closed.
Description
This all began after some of the powers that be (and me too!) decided to remove the OpenSSL library from Windows Meterpreter. As a result of that discussion, @busterb went ahead and did the initial OpenSSL removal work in rapid7/metasploit-payloads#205. As shown in the PR comments, various community members expressed a perfectly valid concern about now having encryption on
reverse_tcp
transports. While we initially felt that the lack of encryption in the short term was something we were happy to live, we instead took it on ourselves to go ahead and satiate this need before changing anything.This is another large set of changes, and comes with a large PR description. You'll have to fortive me. I'll do my best to break up the boring bits with amusing memes, gifs and useful tidbits of information.
High-level rationale goes like this:
reverse_tcp
transports).reverse_https
) makes use of CSP/Schannel, and so doesn't rely on the OpenSSL lib that comes withmetsrv
.metsrv
DLL by a whopping 80%! A saving that shouldn't be sniffed at.It turned out that we had a few other needs that were in some way related, and given that we don't like to constantly break binary compatibility with old payloads, it was a good idea to be "in for a penny, in for a pound" and break everything at once so we didn't have to do it again any time soon. The primary need was to modify the packet header so that it included the Session GUID that has been added recently. This would mean that we would have the ability to associate a packet with a session handler, regardless of the transport that the packet came from, and for us to handle the routing of this packet to the relevant session handler that contains the per-session encryption key. This opens up a world of fun things that we can do down the track (more to come on that in future PRs, but trust me, it's going to be funzies!).
Given that we were going to make a point of implementing encryption for
reverse_tcp
, it made more sense to enable encryption at the TLV packet level, rather than binding it to the transport. As a result this work contains not only the removal of OpenSSL, but addition of a bunch of code that enables per-session encryption using encryption tha tis specific to the session, with key sharing via RSA. More on this in a moment. XORing the packet with a per-packet XOR key is still needed to help obfuscate the packet headers, and to make it so that the few packets that might fly in plain view are still obscured.For various reasons (including not updating all the Meterpreters, and needing to have "raw" packets to begin with), the packet needed to be modified to indicate whether or not the packet contained encrypted data. Combine that with the session GUID requirement, and we had the need to modify the packet header... resulting in the breaking of binary backwards compatibility.
Given that we were breaking back compat, I felt now was the time to also fix up the obscure XOR byte swizzling thing we were doing as a result of attempting to use UINT values on Windows. At this point, all the XOR stuff is done assuming a straight 4-byte array, and the Meterpreter code has been updated to reflect this as well.
Packet header changes
Prior to this PR, the packet header looked like this:
After this PR, the packet header looks like this:
null
bytes), and a new one is generated and passed to the Meterpreter instance when the session is established. This GUID is now easily accessible outside of the contents of the TLV packets, and as said before this means we can route the packet to the correct handler prior to it being decrypted with the session-specific key.AES256
(ie. the value1
, it will look like this:There is a 16 byte initialisation vector that is used to being the AES256 decryption process. After those 16 bytes is the full encrypted TLV payload.
In short, the packet size has increased from
12
bytes to32
, and hence needs to be handled as follows:32
byte header should be downloaded first.4
bytes should be extracted, and then used to de-XOR the remaining 28 bytes as a single block.length
value indicates how many more bytes should be read from the data stream associated with the transport.5
(again, to avoid packet alignment issues).16
bytes, and then the decryption process should happen, converting the payload back into a set of TLV values.Packet encryption
As mentioned before, packets are now encrypted with AES256 in windows. But not immediately. The session needs to be established, and MSF will negotiate keys with Meterpreter on the fly. This is done for a few reasons:
This then begs the question: how do we exchange keys in a "secure" manner. At the end of this PR I'll post a rationale behind the approach that we're taking, particularly in an effort to address the "OMG THIS CAN BE MITM'd" crowd. The following shows the "handshake" process:
bind
payloads).metsrv
DLL down the wire to the stager.metsrv
begins execution and takes over communications from the stager.2048
bit RSA key pair (both public and private key).pem
format, and the packet is sent to Meterpreter with the commandcore_negotiate_aes
.There are a couple of cases that needed to be considered when it came to packet encryption:
sleep
or transport switch), MSF believes the session to be dead. When the session comes back, MSF again negotiates a new AES key, as it doesn't have any tracking of the previous AES key. This might change down the track if the team decides to track AES across the session (personally I don't see the need -- more keys, more pain for IR!).These cases have been considered and coded for.
Verification
I am sorry. I am so, so sorry for whoever has to do this. But, sometimes work sucks! This is one of those times.
These verification steps should be done for each of the Meterpreters that exist (yeah.. I know, right?), and for each transport:
sleep
ortransport
switching.sessions -x
andsessions -v
and make sure the encryption indicator is accurate.For non-HTTPS payloads on Windows:
ERMAGHERD!! MAN IN THE MIDDLE!!
The most common question that has come up so far, and probably the most common that will come up down the track, is:
I'm glad you asked. At leaset, I think I am.
The Short Answer
Yes! but this is no different to before ripping out OpenSSL.
If you don't believe me, read the next answer.
The Long Answer
There are two types of MITM attacks that should be considered if we're going to be completely thorough: active MITM and passive MITM.
Active MITM
The risk of active MITM is no different to how it was before. Meterpreter over TCP did nothing to validate that the
STARTTLS
was actually going to MSF, it just trusted that the 'thing on the other end' was MSF and did the handshake with it before sending it data. It didn't know for sure that MSF was listening, it could instead be someone malicious that is tapping the content of the communications in plain text. If active MITM is a concern now, it should have been a concern before. The risk is no different.What can we do about it? Actually not a great deal. In order to avoid active MITM threats, we need a way of validation that the "thing on the other end" is what we expected, and validating that in the context of Meterp that was just brought to life via an exploit in an unknown network isn't exactly easy. It's fair to assume that the network is hostile, and that any means of validation we have isn't reliable. In the case of HTTPS connections, certificates are validated against a certificate store that references a hierarchy of authorities that can be verified. We just don't have this capability. The closest thing we have is baking a certificate of some kind into Meterpreter and validating the keys that come from MSF against that cert. This is again moot though, because if we have Active MITM going on, then that certificate can be replaced on the wire with a malicious one, and we're back to where we started.
It's fair to say that if you are currently being actively MITM'd, you have literally zero points of trust. You can choose to trust nothing, and shut yourself down, or you can do what you've always done, and accept the session and carry on with the assessment.
Passive MITM
This is a different beast, as it doesn't muck with the traffic. The concern here is that the packets captured can be analysed and decrypted later on. Assuming that we're not also being Actively MITM'd we're in a position to prevent the pcap from being decrypted, as we've got the equivalent of HTTPS with PFS enabled. The only differences are:
For someone to decrypt the traffic, they need to get hold of the AES keys. In order to get hold of the AES keys, they need to be able to get access to private key that was generated by MSF (again, we're assuming no active MITM where keys can be switched). For an attacker to get access to the private RSA key, the attacker's machine must be compromised. However, given that the attacker's machine does not store the RSA key pair for any longer than the duration of the request, compromsing the attacker's machine would be useless as the key is no longer there to be found. In the case of normal HTTPS, PSF is supported by generating ephemeral keys that are signed by the RSA private key associated with the HTTPS certificate. If the HTTPS certificate is later accessed, the private key can not be used to decrypt the traffic, as it's public key not the one that was used to encrypt it in the first place. We're doing the same thing, just without signing the key.
I honestly feel that the risk of passive MITM resulting in the compromise of data going across the wire is as small as it can be.
Thanks
As always, props to the crew behind the scenes that helped with this (even if it was just the thought process), including @busterb and @sempervictus. Thanks in advance to those people who get involved with the discussion and who help get this over the line.
Let the discussion commence!
Sorry!
I know, I lied about the memes and gifs!
Edit 28th June
Both
sessions -x
andsessions -v
include an indication of whether or not the session is currently utilising packet level encryption. This should be tested as well!