cleanup: Code review changes and flake8 all the things.

GoSecure · Apr 9, 2020 · 5906da4 · 5906da4
1 parent 1818b88
commit 5906da4
Show file tree

Hide file tree

Showing 28 changed files with 200 additions and 110 deletions.
diff --git a/.flake8 b/.flake8
@@ -1,3 +1,3 @@
 [flake8]
 # ignore = 
-max-line-length = 160
+max-line-length = 120
diff --git a/README.md b/README.md
@@ -393,6 +393,9 @@ Tells the MITM to allow clients to use [Graphics Device Interface Acceleration][
 drawing orders instead of raw bitmaps. The advantage of this mode is a significant reduction in required bandwidth
 for high resolution connections.
 
+Note that some GDI drawing orders are currently unimplemented because they appear to be unused.
+If you have a replay which contains any unsupported or untested order, do not hesitate to share it with the project maintainers so that support can be added as required. (Make sure that the trace does not contain sensitive information)
+
 [gdi]: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegdi/745f2eee-d110-464c-8aca-06fc1814f6ad
 
 ### Using the PyRDP Player

diff --git a/bin/pyrdp-player.py b/bin/pyrdp-player.py
@@ -8,21 +8,21 @@
 
 # asyncio needs to be imported first to ensure that the reactor is
 # installed properly. Do not re-order.
-import asyncio
+import asyncio   # noqa
 from twisted.internet import asyncioreactor
 asyncioreactor.install(asyncio.get_event_loop())
 
-from pyrdp.core import settings
-from pyrdp.logging import LOGGER_NAMES, NotifyHandler, configure as configureLoggers
-from pyrdp.player import HAS_GUI
-from pyrdp.player.config import DEFAULTS
+from pyrdp.core import settings  # noqa
+from pyrdp.logging import LOGGER_NAMES, NotifyHandler, configure as configureLoggers  # noqa
+from pyrdp.player import HAS_GUI  # noqa
+from pyrdp.player.config import DEFAULTS  # noqa
 
-from pathlib import Path
-import argparse
-import logging
-import logging.handlers
-import sys
-import os
+from pathlib import Path  # noqa
+import argparse  # noqa
+import logging  # noqa
+import logging.handlers  # noqa
+import sys  # noqa
+import os  # noqa
 
 if HAS_GUI:
     from pyrdp.player import MainWindow
@@ -34,7 +34,7 @@ def enableNotifications(logger):
     # https://docs.python.org/3/library/os.html
     if os.name != "nt":
         notifyHandler = NotifyHandler()
-        notifyHandler.setFormatter(logging.Formatter("[{asctime}] - {message}", style = "{"))
+        notifyHandler.setFormatter(logging.Formatter("[{asctime}] - {message}", style="{"))
 
         uiLogger = logging.getLogger(LOGGER_NAMES.PLAYER_UI)
         uiLogger.addHandler(notifyHandler)
@@ -52,8 +52,10 @@ def main():
     parser.add_argument("-b", "--bind", help="Bind address (default: 127.0.0.1)", default="127.0.0.1")
     parser.add_argument("-p", "--port", help="Bind port (default: 3000)", default=3000)
     parser.add_argument("-o", "--output", help="Output folder", default="pyrdp_output")
-    parser.add_argument("-L", "--log-level", help="Log level", default=None, choices=["INFO", "DEBUG", "WARNING", "ERROR", "CRITICAL"], nargs="?")
-    parser.add_argument("-F", "--log-filter", help="Only show logs from this logger name (accepts '*' wildcards)", default=None)
+    parser.add_argument("-L", "--log-level", help="Log level", default=None,
+                        choices=["INFO", "DEBUG", "WARNING", "ERROR", "CRITICAL"], nargs="?")
+    parser.add_argument("-F", "--log-filter",
+                        help="Only show logs from this logger name (accepts '*' wildcards)", default=None)
     parser.add_argument("--headless", help="Parse a replay without rendering the user interface.", action="store_true")
     args = parser.parse_args()
 
@@ -77,7 +79,8 @@ def main():
         enableNotifications(logger)
 
     if not HAS_GUI and not args.headless:
-        logger.error('Headless mode is not specified and PySide2 is not installed. Install PySide2 to use the graphical user interface.')
+        logger.error('Headless mode is not specified and PySide2 is not installed.'
+                     ' Install PySide2 to use the graphical user interface.')
         sys.exit(127)
 
     if not args.headless:

diff --git a/pyrdp/enum/__init__.py b/pyrdp/enum/__init__.py
@@ -1,8 +1,9 @@
 #
 # This file is part of the PyRDP project.
-# Copyright (C) 2018 GoSecure Inc.
+# Copyright (C) 2018-2020 GoSecure Inc.
 # Licensed under the GPLv3 or later.
 #
+# flake8: noqa
 
 from pyrdp.enum.core import ParserMode
 from pyrdp.enum.gcc import GCCPDUType

diff --git a/pyrdp/enum/orders.py b/pyrdp/enum/orders.py
@@ -1,6 +1,6 @@
 #
 # This file is part of the PyRDP project.
-# Copyright (C) 2019 GoSecure Inc.
+# Copyright (C) 2020 GoSecure Inc.
 # Licensed under the GPLv3 or later.
 #
 

diff --git a/pyrdp/enum/rdp.py b/pyrdp/enum/rdp.py
@@ -1,9 +1,12 @@
 #
 # This file is part of the PyRDP project.
-# Copyright (C) 2018, 2019 GoSecure Inc.
+# Copyright (C) 2018-2020 GoSecure Inc.
 # Licensed under the GPLv3 or later.
 #
 
+# Disable line-too-long lints.
+# flake8: noqa
+
 from enum import IntEnum, IntFlag
 
 
@@ -60,10 +63,12 @@ class SecurityHeaderType(IntEnum):
     # Header type used for Client Info and Licensing PDUs if no encryption is used
     DEFAULT = 1
 
+
 class FastPathSecurityFlags:
     FASTPATH_OUTPUT_SECURE_CHECKSUM = 0x40
     FASTPATH_OUTPUT_ENCRYPTED = 0x80
 
+
 class FastPathInputType(IntEnum):
     FASTPATH_INPUT_EVENT_SCANCODE = 0
     FASTPATH_INPUT_EVENT_MOUSE = 1
@@ -182,6 +187,7 @@ class SlowPathPDUType(IntEnum):
     DATA_PDU = 0x7
     SERVER_REDIR_PKT_PDU = 0xA
 
+
 class SlowPathUpdateType(IntEnum):
     """
     https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/bd3c4df4-87b9-43dd-88cb-ce5b24698e19
@@ -191,6 +197,7 @@ class SlowPathUpdateType(IntEnum):
     SLOWPATH_UPDATETYPE_PALETTE = 2
     SLOWPATH_UPDATETYPE_SYNCHRONIZE = 3
 
+
 class SlowPathDataType(IntEnum):
     """
     Slow-path data PDU types.
@@ -384,6 +391,7 @@ class NegotiationFailureCode(IntEnum):
     HYBRID_REQUIRED_BY_SERVER = 0x00000005
     SSL_WITH_USER_AUTH_REQUIRED_BY_SERVER = 0x00000006
 
+
     @staticmethod
     def getMessage(code: "NegotiationFailureCode"):
         MESSAGES = {
@@ -819,8 +827,6 @@ class ChannelOption(IntFlag):
     REMOTE_CONTROL_PERSISTENT = 0x00100000
 
 
-
-
 class BitmapFlags(IntEnum):
     """
     https://msdn.microsoft.com/en-us/library/cc240612.aspx

diff --git a/pyrdp/mitm/SlowPathMITM.py b/pyrdp/mitm/SlowPathMITM.py
@@ -1,16 +1,17 @@
 #
 # This file is part of the PyRDP project.
-# Copyright (C) 2019 GoSecure Inc.
+# Copyright (C) 2019-2020 GoSecure Inc.
 # Licensed under the GPLv3 or later.
 #
 
-from pyrdp.enum import CapabilityType, KeyboardFlag, OrderFlag, VirtualChannelCompressionFlag
+from pyrdp.enum import CapabilityType, KeyboardFlag, OrderFlag, VirtualChannelCompressionFlag, Order
 from pyrdp.layer import SlowPathLayer, SlowPathObserver
 from pyrdp.logging.StatCounter import StatCounter, STAT
 from pyrdp.mitm.state import RDPMITMState
 from pyrdp.pdu import Capability, ConfirmActivePDU, DemandActivePDU, InputPDU, KeyboardEvent, SlowPathPDU
 from pyrdp.mitm.BasePathMITM import BasePathMITM
 
+
 class SlowPathMITM(BasePathMITM):
     """
     MITM component for the slow-path layer.
@@ -24,8 +25,8 @@ def __init__(self, client: SlowPathLayer, server: SlowPathLayer, state: RDPMITMS
         super().__init__(state, client, server, statCounter)
 
         self.clientObserver = self.client.createObserver(
-            onPDUReceived = self.onClientPDUReceived,
-            onConfirmActive = self.onConfirmActive
+            onPDUReceived=self.onClientPDUReceived,
+            onConfirmActive=self.onConfirmActive
         )
 
         self.serverObserver = self.server.createObserver(
@@ -44,7 +45,8 @@ def onClientPDUReceived(self, pdu: SlowPathPDU):
             if isinstance(pdu, InputPDU):
                 for event in pdu.events:
                     if isinstance(event, KeyboardEvent):
-                        self.onScanCode(event.keyCode, event.flags & KeyboardFlag.KBDFLAGS_DOWN == 0, event.flags & KeyboardFlag.KBDFLAGS_EXTENDED != 0)
+                        self.onScanCode(event.keyCode, event.flags & KeyboardFlag.KBDFLAGS_DOWN == 0,
+                                        event.flags & KeyboardFlag.KBDFLAGS_EXTENDED != 0)
 
     def onServerPDUReceived(self, pdu: SlowPathPDU):
         self.statCounter.increment(STAT.IO_OUTPUT_SLOWPATH)
@@ -68,26 +70,29 @@ def onConfirmActive(self, pdu: ConfirmActivePDU):
             # Disable GDI if not explicitly requested.
             if not self.state.config.useGdi:
                 # Force RDP server to send bitmap events instead of order events.
-                pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_ORDER].orderFlags = OrderFlag.NEGOTIATEORDERSUPPORT | OrderFlag.ZEROBOUNDSDELTASSUPPORT
+                pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_ORDER].orderFlags = \
+                    OrderFlag.NEGOTIATEORDERSUPPORT | OrderFlag.ZEROBOUNDSDELTASSUPPORT
                 pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_ORDER].orderSupport = b"\x00" * 32
 
                 # Override the bitmap cache capability set with null values.
                 if CapabilityType.CAPSTYPE_BITMAPCACHE in pdu.parsedCapabilitySets:
-                    pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_BITMAPCACHE] = Capability(CapabilityType.CAPSTYPE_BITMAPCACHE, b"\x00" * 36)
+                    pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_BITMAPCACHE] = Capability(
+                        CapabilityType.CAPSTYPE_BITMAPCACHE, b"\x00" * 36)
             else:
                 # Disable NineGrid support (Not implemented in Player)
                 if CapabilityType.CAPSTYPE_ORDER in pdu.parsedCapabilitySets:
                     orders = pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_ORDER]
                     supported = bytearray(orders.orderSupport)
-                    supported[0x7] = 0  # Spoof disable NineGrid support.
+                    supported[Order.TS_NEG_DRAWNINEGRID_INDEX] = 0
                     orders.orderSupport = supported
 
                 if CapabilityType.CAPSTYPE_DRAWNINEGRIDCACHE in pdu.parsedCapabilitySets:
                     pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_DRAWNINEGRIDCACHE].rawData = b"\x00"*8
 
         # Disable virtual channel compression
         if CapabilityType.CAPSTYPE_VIRTUALCHANNEL in pdu.parsedCapabilitySets:
-            pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_VIRTUALCHANNEL].flags = VirtualChannelCompressionFlag.VCCAPS_NO_COMPR
+            pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_VIRTUALCHANNEL].flags = \
+                VirtualChannelCompressionFlag.VCCAPS_NO_COMPR
 
     def onDemandActive(self, pdu: DemandActivePDU):
         """
@@ -98,7 +103,8 @@ def onDemandActive(self, pdu: DemandActivePDU):
         if CapabilityType.CAPSTYPE_ORDER in pdu.parsedCapabilitySets:
             orders = pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_ORDER]
             supported = bytearray(orders.orderSupport)
-            supported[0x7] = 0  # DRAWNINEGRID = False
+            supported[Order.TS_NEG_DRAWNINEGRID_INDEX] = 0
             orders.orderSupport = supported
 
-        pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_VIRTUALCHANNEL].flags = VirtualChannelCompressionFlag.VCCAPS_NO_COMPR
+        pdu.parsedCapabilitySets[CapabilityType.CAPSTYPE_VIRTUALCHANNEL].flags = \
+            VirtualChannelCompressionFlag.VCCAPS_NO_COMPR
diff --git a/pyrdp/mitm/cli.py b/pyrdp/mitm/cli.py
@@ -114,43 +114,72 @@ def generateCertificate(keyPath: str, certificatePath: str) -> bool:
     else:
         nullDevicePath = "NUL"
 
-    result = os.system("openssl req -newkey rsa:2048 -nodes -keyout %s -x509 -days 365 -out %s -subj \"/CN=www.example.com/O=PYRDP/C=US\" 2>%s" % (keyPath, certificatePath, nullDevicePath))
+    result = os.system("openssl req -newkey rsa:2048 -nodes -keyout %s -x509"
+                       " -days 365 -out %s -subj \"/CN=www.example.com/O=PYRDP/C=US\" 2>%s" %
+                       (keyPath, certificatePath, nullDevicePath))
     return result == 0
 
 
 def showConfiguration(config: MITMConfig):
-    logging.getLogger(LOGGER_NAMES.MITM).info("Target: %(target)s:%(port)d", {"target": config.targetHost, "port": config.targetPort})
-    logging.getLogger(LOGGER_NAMES.MITM).info("Output directory: %(outputDirectory)s", {"outputDirectory": config.outDir.absolute()})
+    logging.getLogger(LOGGER_NAMES.MITM).info("Target: %(target)s:%(port)d", {
+        "target": config.targetHost, "port": config.targetPort})
+    logging.getLogger(LOGGER_NAMES.MITM).info("Output directory: %(outputDirectory)s",
+                                              {"outputDirectory": config.outDir.absolute()})
 
 
 def buildArgParser():
     parser = argparse.ArgumentParser()
     parser.add_argument("target", help="IP:port of the target RDP machine (ex: 192.168.1.10:3390)")
     parser.add_argument("-l", "--listen", help="Port number to listen on (default: 3389)", default=3389)
     parser.add_argument("-o", "--output", help="Output folder", default="pyrdp_output")
-    parser.add_argument("-i", "--destination-ip", help="Destination IP address of the PyRDP player.If not specified, RDP events are not sent over the network.")
-    parser.add_argument("-d", "--destination-port", help="Listening port of the PyRDP player (default: 3000).", default=3000)
+    parser.add_argument("-i", "--destination-ip",
+                        help="Destination IP address of the PyRDP player.If not specified, RDP events are"
+                        " not sent over the network.")
+    parser.add_argument("-d", "--destination-port",
+                        help="Listening port of the PyRDP player (default: 3000).", default=3000)
     parser.add_argument("-k", "--private-key", help="Path to private key (for SSL)")
     parser.add_argument("-c", "--certificate", help="Path to certificate (for SSL)")
     parser.add_argument("-u", "--username", help="Username that will replace the client's username", default=None)
     parser.add_argument("-p", "--password", help="Password that will replace the client's password", default=None)
-    parser.add_argument("-L", "--log-level", help="Console logging level. Logs saved to file are always verbose.", default="INFO", choices=["INFO", "DEBUG", "WARNING", "ERROR", "CRITICAL"])
-    parser.add_argument("-F", "--log-filter", help="Only show logs from this logger name (accepts '*' wildcards)", default="")
-    parser.add_argument("-s", "--sensor-id", help="Sensor ID (to differentiate multiple instances of the MITM where logs are aggregated at one place)", default="PyRDP")
+    parser.add_argument("-L", "--log-level", help="Console logging level. Logs saved to file are always verbose.",
+                        default="INFO", choices=["INFO", "DEBUG", "WARNING", "ERROR", "CRITICAL"])
+    parser.add_argument("-F", "--log-filter",
+                        help="Only show logs from this logger name (accepts '*' wildcards)", default="")
+    parser.add_argument(
+        "-s", "--sensor-id", help="Sensor ID (to differentiate multiple instances of the MITM"
+        " where logs are aggregated at one place)", default="PyRDP")
     parser.add_argument("--payload", help="Command to run automatically upon connection", default=None)
-    parser.add_argument("--payload-powershell", help="PowerShell command to run automatically upon connection", default=None)
-    parser.add_argument("--payload-powershell-file", help="PowerShell script to run automatically upon connection (as -EncodedCommand)", default=None)
-    parser.add_argument("--payload-delay", help="Time to wait after a new connection before sending the payload, in milliseconds", default=None)
-    parser.add_argument("--payload-duration", help="Amount of time for which input / output should be dropped, in milliseconds. This can be used to hide the payload screen.", default=None)
-    parser.add_argument("--disable-active-clipboard", help="Disables the active clipboard stealing to request clipboard content upon connection.", action="store_true")
+    parser.add_argument("--payload-powershell",
+                        help="PowerShell command to run automatically upon connection", default=None)
+    parser.add_argument("--payload-powershell-file",
+                        help="PowerShell script to run automatically upon connection (as -EncodedCommand)",
+                        default=None)
+    parser.add_argument(
+        "--payload-delay", help="Time to wait after a new connection before sending the payload, in milliseconds",
+        default=None)
+    parser.add_argument(
+        "--payload-duration", help="Amount of time for which input / output should be dropped, in milliseconds."
+        " This can be used to hide the payload screen.", default=None)
+    parser.add_argument("--disable-active-clipboard",
+                        help="Disables the active clipboard stealing to request clipboard content upon connection.",
+                        action="store_true")
     parser.add_argument("--crawl", help="Enable automatic shared drive scraping", action="store_true")
-    parser.add_argument("--crawler-match-file", help="File to be used by the crawler to chose what to download when scraping the client shared drives.", default=None)
-    parser.add_argument("--crawler-ignore-file", help="File to be used by the crawler to chose what folders to avoid when scraping the client shared drives.", default=None)
+    parser.add_argument("--crawler-match-file",
+                        help="File to be used by the crawler to chose what to download when scraping the client shared"
+                        " drives.", default=None)
+    parser.add_argument("--crawler-ignore-file",
+                        help="File to be used by the crawler to chose what folders to avoid when scraping the client"
+                        " shared drives.", default=None)
     parser.add_argument("--no-replay", help="Disable replay recording", action="store_true")
-    parser.add_argument("--no-downgrade", help="Disables downgrading of unsupported extensions. This makes PyRDP harder to fingerprint but might impact the player's ability to replay captured traffic.", action="store_true")
-    parser.add_argument("--no-files", help="Do not extract files transferred between the client and server.", action="store_true")
-    parser.add_argument("--transparent", help="Spoof source IP for connections to the server (See README)", action="store_true")
-    parser.add_argument("--gdi", help="Accept accelerated graphics pipeline (MS-RDPEGDI) extension", action="store_true")
+    parser.add_argument("--no-downgrade", help="Disables downgrading of unsupported extensions. This makes PyRDP harder"
+                        " to fingerprint but might impact the player's ability to replay captured traffic.",
+                        action="store_true")
+    parser.add_argument(
+        "--no-files", help="Do not extract files transferred between the client and server.", action="store_true")
+    parser.add_argument(
+        "--transparent", help="Spoof source IP for connections to the server (See README)", action="store_true")
+    parser.add_argument("--gdi", help="Accept accelerated graphics pipeline (MS-RDPEGDI) extension",
+                        action="store_true")
 
     return parser
 
@@ -207,7 +236,11 @@ def configure(cmdline=None) -> MITMConfig:
     payload = None
     powershell = None
 
-    if int(args.payload is not None) + int(args.payload_powershell is not None) + int(args.payload_powershell_file is not None) > 1:
+    npayloads = int(args.payload is not None) + \
+        int(args.payload_powershell is not None) + \
+        int(args.payload_powershell_file is not None)
+
+    if npayloads > 1:
         logger.error("Only one of --payload, --payload-powershell and --payload-powershell-file may be supplied.")
         sys.exit(1)
 
@@ -254,7 +287,9 @@ def configure(cmdline=None) -> MITMConfig:
             sys.exit(1)
 
         if config.payloadDelay < 1000:
-            logger.warning("You have provided a payload delay of less than 1 second. We recommend you use a slightly longer delay to make sure it runs properly.")
+            logger.warning(
+                "You have provided a payload delay of less than 1 second."
+                " We recommend you use a slightly longer delay to make sure it runs properly.")
 
         try:
             config.payloadDuration = int(args.payload_duration)