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

Fix python meterpreter network method exceptions for OSX #651

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions .github/workflows/python.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
name: Python

# https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions
permissions:
actions: none
checks: none
contents: none
deployments: none
id-token: none
issues: none
discussions: none
packages: none
pages: none
pull-requests: none
repository-projects: none
security-events: none
statuses: none

on:
push:
paths:
- 'python/**'
pull_request:
paths:
- 'python/**'

jobs:
verify:
strategy:
fail-fast: false
matrix:
os:
- macos-11
- windows-2019
- ubuntu-20.04
runtime_version:
- 3.6
- 3.8
- 3.11
include:
# We run older Python versions in Docker - as Github Actions does not support installing these versions on the host
- { os: ubuntu-20.04, runtime_version: 2.7, docker_image: 'python:2.7-alpine' }
- { os: ubuntu-20.04, runtime_version: 3.3, docker_image: 'python:3.3-alpine' }
- { os: ubuntu-20.04, runtime_version: 3.4, docker_image: 'python:3.4-alpine' }
- { os: ubuntu-20.04, runtime_version: 3.5, docker_image: 'python:3.5-alpine' }

timeout-minutes: 40
runs-on: ${{ matrix.os }}
name: Python ${{ matrix.runtime_version }} ${{ matrix.docker_image && 'Docker' || matrix.os }}

steps:
- name: Checkout code
uses: actions/checkout@v3

- name: Run tests in docker
if: ${{ matrix.docker_image }}
env:
DOCKER_IMAGE: ${{ matrix.docker_image }}
run: |
cd python/meterpreter
docker run --rm -w $(pwd) -v $(pwd):$(pwd) ${DOCKER_IMAGE} /bin/sh -c 'ls -lah; pip install mock; python -m unittest discover -v ./tests'

- name: Set up Python on host
if: ${{ !matrix.docker_image }}
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.runtime_version }}

- name: Run tests on host
if: ${{ !matrix.docker_image }}
run: |
cd python/meterpreter
python -m unittest discover -v ./tests
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Verify
name: WindowsMeterpreter

# https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions
permissions:
Expand All @@ -16,10 +16,16 @@ permissions:
security-events: none
statuses: none

on: [push, pull_request]
on:
push:
paths:
- 'c/**'
pull_request:
paths:
- 'c/**'

jobs:
mingw:
verify:
runs-on: ubuntu-latest
timeout-minutes: 40
name: Meterpreter MinGW Docker Build
Expand Down
1 change: 1 addition & 0 deletions python/meterpreter/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
*.pyc
11 changes: 11 additions & 0 deletions python/meterpreter/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
## Python Meterpreter

Running unit tests:

```
# Only required if less than python 3.3
pip install mock
adfoster-r7 marked this conversation as resolved.
Show resolved Hide resolved

# Run the tests
python -m unittest discover -v ./tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer pytest, but I thought only depending on the inbuilt testing library would be easier for contributors

```
18 changes: 10 additions & 8 deletions python/meterpreter/ext_server_stdapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -2188,8 +2188,8 @@ def stdapi_net_config_get_interfaces(request, response):
iface_tlv += tlv_pack(TLV_TYPE_MAC_ADDRESS, iface_info.get('hw_addr', '\x00\x00\x00\x00\x00\x00'))
if 'mtu' in iface_info:
iface_tlv += tlv_pack(TLV_TYPE_INTERFACE_MTU, iface_info['mtu'])
if 'flags' in iface_info:
iface_tlv += tlv_pack(TLV_TYPE_INTERFACE_FLAGS, iface_info['flags'])
if 'flags_str' in iface_info:
iface_tlv += tlv_pack(TLV_TYPE_INTERFACE_FLAGS, iface_info['flags_str'])
Comment on lines +2191 to +2192
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OSX was returning the integer flag value here, whilst linux was returning the string flag names. Now the implementations return both separately

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be introducing new data into the TLV, is there value for both vs just shifting to a more consistent type retuned in flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TLV_TYPE_INTERFACE_FLAGS has always been a string:

TLV_TYPE_INTERFACE_FLAGS    = TLV_META_TYPE_STRING  | 1403

Previously the OSX code path was was extracting out the integer value for flags instead of a string like in the Linux codepath, i.e. in the osx code path attempted to write out 8836 instead of "UP,BROADCAST,SMART,RUNNING,SIMPLEX,MULTICAST" from the below ifconfig output

en5: flags=8863<UP,BROADCAST,SMART,RUNNING,SIMPLEX,MULTICAST> mtu 1500

This is now updated to send the string flag names for both OSX and Linux

Copy link
Contributor

@jmartin-tech jmartin-tech Jun 5, 2023

Choose a reason for hiding this comment

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

Got it, stdapi_net_config_get_interfaces_via_netlink was returning flags as a set of strings names and stdapi_net_config_get_interfaces_via_osx_ifconfig was returning flags as numeric values now

Now both return flags as numeric values and flags_str as string names and only flags_str is being placed in the response as a TLV_TYPE_INTERFACE_FLAGS packet.

iface_tlv += tlv_pack(TLV_TYPE_INTERFACE_INDEX, iface_info['index'])
for address in iface_info.get('addrs', []):
iface_tlv += tlv_pack(TLV_TYPE_IP, address[1])
Expand Down Expand Up @@ -2224,7 +2224,8 @@ def stdapi_net_config_get_interfaces_via_netlink():
for flag in iface_flags_sorted:
if (iface.flags & flag):
flags.append(iface_flags[flag])
iface_info['flags'] = ' '.join(flags)
iface_info['flags'] = iface.flags
iface_info['flags_str'] = ' '.join(flags)
cursor = ctypes.sizeof(IFINFOMSG)
while cursor < len(res_data):
attribute = ctstruct_unpack(RTATTR, res_data[cursor:])
Expand Down Expand Up @@ -2271,19 +2272,20 @@ def stdapi_net_config_get_interfaces_via_osx_ifconfig():
proc_h = subprocess.Popen('/sbin/ifconfig', stdout=subprocess.PIPE, stderr=subprocess.PIPE)
if proc_h.wait():
raise Exception('ifconfig exited with non-zero status')
output = proc_h.stdout.read()
output = str(proc_h.stdout.read())
Copy link
Contributor Author

@adfoster-r7 adfoster-r7 Jun 5, 2023

Choose a reason for hiding this comment

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

Required in newer versions of Python - when the later code attempts to split this result with for line in output.split('\n') the following exception is raised:

[-] method stdapi_net_config_get_interfaces resulted in an error
Traceback (most recent call last):
  File "<string>", line 1725, in create_response
  File "ext_server_stdapi.py", line 2191, in stdapi_net_config_get_interfaces
    interfaces = stdapi_net_config_get_interfaces_via_osx_ifconfig()
  File "ext_server_stdapi.py", line 2293, in stdapi_net_config_get_interfaces_via_osx_ifconfig
    for line in output.split('\n'):
TypeError: a bytes-like object is required, not 'str'


interfaces = []
iface = {}
for line in output.split('\n'):
match = re.match(r'^([a-z0-9]+): flags=(\d+)<[A-Z,]*> mtu (\d+)\s*$', line)
match = re.match(r'^([a-z0-9]+): flags=(\d+)<([A-Z,]*)> mtu (\d+)\s*$', line)
if match is not None:
if iface:
interfaces.append(iface)
iface = {}
iface['name'] = match.group(1)
iface['flags'] = int(match.group(2))
iface['mtu'] = int(match.group(3))
iface['flags_str'] = match.group(3)
iface['mtu'] = int(match.group(4))
iface['index'] = len(interfaces)
continue
match = re.match(r'^\s+ether (([a-f0-9]{2}:){5}[a-f0-9]{2})\s*$', line)
Expand Down Expand Up @@ -2487,7 +2489,7 @@ def stdapi_net_config_get_routes_via_osx_netstat():
proc_h = subprocess.Popen(['/usr/sbin/netstat', '-rn'], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like there's a file descriptor leak here, will put up a separate PR for this

$ python3 -m unittest discover -v ./tests
test_stdapi_net_config_get_interfaces (test_ext_server_stdapi.TestExtServerStdApi.test_stdapi_net_config_get_interfaces) ... <string>:2183: ResourceWarning: unclosed file <_io.BufferedReader name=3>
Object allocated at (most recent call last):
  File "/usr/local/Cellar/python@3.11/3.11.2_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/subprocess.py", lineno 1014
    self.stdout = io.open(c2pread, 'rb', bufsize)
<string>:2183: ResourceWarning: unclosed file <_io.BufferedReader name=5>
Object allocated at (most recent call last):
  File "/usr/local/Cellar/python@3.11/3.11.2_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/subprocess.py", lineno 1019
    self.stderr = io.open(errread, 'rb', bufsize)

if proc_h.wait():
raise Exception('netstat exited with non-zero status')
output = proc_h.stdout.read()
output = str(proc_h.stdout.read())

routes = []
state = None
Expand Down Expand Up @@ -2524,7 +2526,7 @@ def stdapi_net_config_get_routes_via_osx_netstat():
continue
if destination == 'default':
destination = all_nets
if re.match('link#\d+', gateway) or re.match('([0-9a-f]{1,2}:){5}[0-9a-f]{1,2}', gateway):
if re.match('link#\\d+', gateway) or re.match('([0-9a-f]{1,2}:){5}[0-9a-f]{1,2}', gateway):
Copy link
Contributor Author

@adfoster-r7 adfoster-r7 Jun 5, 2023

Choose a reason for hiding this comment

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

Fixes:

test_stdapi_net_config_get_interfaces_via_osx_ifconfig (test_ext_server_stdapi.TestExtServerStdApi.test_stdapi_net_config_get_interfaces_via_osx_ifconfig) ... :2529: DeprecationWarning: invalid escape sequence '\d'

gateway = all_nets[:-2]
if '/' in destination:
destination, netmask_bits = destination.rsplit('/', 1)
Expand Down
168 changes: 168 additions & 0 deletions python/meterpreter/tests/test_ext_server_stdapi.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
# coding=utf-8

import unittest
import code
import sys
import socket

if sys.version_info >= (3, 3):
from unittest import mock
else:
import mock

ERROR_SUCCESS = 0


def create_meterpreter_context():
with open("meterpreter.py", "rb") as file:
# Read and patch out the Meterpreter socket connection logic side effect onwards
source = file.read()
source_without_socket_connection = source[
0 : source.index(b"# PATCH-SETUP-ENCRYPTION #")
]

context = {}
exec(source_without_socket_connection, context, context)
return context


def create_ext_server_stdapi_context(meterpreter, meterpreter_context):
with open("ext_server_stdapi.py", "rb") as file:
extension_content = file.read()

context = {}
context.update(meterpreter_context["EXPORTED_SYMBOLS"])
context["meterpreter"] = meterpreter
exec(extension_content, context, context)
return context


class MockMeterpreter:
def __init__(self):
self.extension_functions = {}

def register_extension(self, extension_name):
pass

def register_function(self, func):
self.extension_functions[func.__name__] = func
return func


class TestExtServerStdApi(unittest.TestCase):
def setUp(self):
self.ext_server_stdapi = create_ext_server_stdapi_context(
MockMeterpreter(), create_meterpreter_context()
)

@mock.patch("subprocess.Popen")
def test_stdapi_net_config_get_routes_via_osx_netstat(self, mock_popen):
popen_read_value = b"""
Routing tables

Internet:
Destination Gateway Flags Netif Expire
default 10.79.0.1 UGScg en0

Internet6:
Destination Gateway Flags Netif Expire
default fe80::%utun0 UGcIg utun0
""".lstrip()

process_mock = mock.Mock()
attrs = {
"stdout.read.return_value": popen_read_value,
"wait.return_value": ERROR_SUCCESS,
}
process_mock.configure_mock(**attrs)
mock_popen.return_value = process_mock

result = self.ext_server_stdapi[
"stdapi_net_config_get_routes_via_osx_netstat"
]()

expected = [
{
"gateway": b"\nO\x00\x01",
"iface": "en0",
"metric": 0,
"netmask": b"\x00\x00\x00\x00",
"subnet": b"\x00\x00\x00\x00",
},
{
"gateway": b"\xfe\x80\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",
"iface": "utun0",
"metric": 0,
"netmask": b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",
"subnet": b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",
},
]

self.assertEqual(result, expected)

@mock.patch("subprocess.Popen")
def test_stdapi_net_config_get_interfaces_via_osx_ifconfig(self, mock_popen):
popen_read_value = b"""
en0: flags=8863<UP,BROADCAST,SMART,RUNNING,SIMPLEX,MULTICAST> mtu 1500
options=400<CHANNEL_IO>
ether 11:22:33:44:55:66
inet 192.168.1.166 netmask 0xffffff00 broadcast 192.168.1.255
media: autoselect
status: active
""".lstrip()

process_mock = mock.Mock()
attrs = {
"stdout.read.return_value": popen_read_value,
"wait.return_value": ERROR_SUCCESS,
}
process_mock.configure_mock(**attrs)
mock_popen.return_value = process_mock

result = self.ext_server_stdapi[
"stdapi_net_config_get_interfaces_via_osx_ifconfig"
]()

expected = [
{
"addrs": [
(
socket.AF_INET,
b"\xc0\xa8\x01\xa6",
b"\xff\xff\xff\x00",
)
],
"flags": 8863,
"flags_str": "UP,BROADCAST,SMART,RUNNING,SIMPLEX,MULTICAST",
"hw_addr": '\x11"3DUf',
"index": 0,
"mtu": 1500,
"name": "en0",
}
]

self.assertEqual(result, expected)

def test_stdapi_net_config_get_interfaces(self):
request = bytes()
response = bytes()
self.assertErrorSuccess("stdapi_net_config_get_interfaces", request, response)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll skip verifying the exact bytes for now, this was just an MVP "make sure it doesn't explode" test for now


def test_stdapi_net_config_get_routes(self):
request = bytes()
response = bytes()
self.assertErrorSuccess("stdapi_net_config_get_routes", request, response)

def assertErrorSuccess(self, method_name, request, response):
request = bytes()
response = bytes()
result = self.ext_server_stdapi[method_name](request, response)

self.assertEqual(result[0], ERROR_SUCCESS)
self.assertIsInstance(result[1], bytes)

return result


if __name__ == "__main__":
unittest.main()