Merge pull request #408 from GoSecure/fileio-improvements

Minor improvements while debugging File IO issues
GoSecure · Sep 13, 2022 · 4f7a942 · 4f7a942
2 parents 4cafde0 + 0def420
commit 4f7a942
Show file tree

Hide file tree

Showing 11 changed files with 129 additions and 36 deletions.
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
@@ -33,7 +33,7 @@ For a detailed view of what has changed, refer to the {uri-repo}/commits/master[
 * Minor CLI improvements
 * Improved type hints
 * Updated instructions to extract the RDP certificate and private key ({uri-issue}345[#345])
-* Documentation updates ({uri-issue}335[#335], {uri-issue}339[#339], {uri-issue}340[#340], {uri-issue}360[#360], {uri-issue}371[#371], {uri-issue}381[#381], {uri-issue}383[#383], {uri-issue}384[#384])
+* Documentation updates ({uri-issue}335[#335], {uri-issue}339[#339], {uri-issue}340[#340], {uri-issue}360[#360], {uri-issue}371[#371], {uri-issue}381[#381], {uri-issue}383[#383], {uri-issue}384[#384], {uri-issue}408[#408])
 * Replaced unmaintained dependency notify2 with py-notifier ({uri-issue}363[#363], {uri-issue}365[#365])
 * Some Python 3.10 compatibility work ({uri-issue}366[#366], {uri-issue}380[#380])
 
@@ -45,6 +45,7 @@ For a detailed view of what has changed, refer to the {uri-repo}/commits/master[
 * Fixed NLA redirection problems if original target and NLA redirection target are the same ({uri-issue}342[#342], {uri-issue}343[#343])
 * Added a missing dependency for the GUI on Ubuntu 20.04 LTS ({uri-issue}348[#348], {uri-issue}351[#351], {uri-issue}355[#355])
 * No longer assuming every connection will have VirtualChannels ({uri-issue}375[#375])
+* Some minor protocol-level fixes ({uri-issue}408[#408])
 
 === Infrastructure
 

diff --git a/docs/debugging-recipes.adoc b/docs/debugging-recipes.adoc
@@ -1,5 +1,7 @@
 = Debugging recipes
 
+Don't Google, search in the master PDF downloadable from here: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/5073f4ed-1e93-45e1-b039-6e30c385867c
+
 == Debugging from the docker container
 
 Build the container (or use an existing tag):
@@ -38,3 +40,76 @@ Run `c` to continue on loading breakpoint:
 
 ipdb> c
 ```
+
+== Protocol-level Debugging
+
+I used that recipe when troubleshooting reproducible Device Redirection issues.
+In the proper parser layer, add print statements of the raw bytes both at the
+parser (`doParse()`) and the writer (`write()`) level. Here's an example patch:
+
+    --- a/pyrdp/parser/rdp/virtual_channel/device_redirection.py
+    +++ b/pyrdp/parser/rdp/virtual_channel/device_redirection.py
+    @@ -108,23 +108,31 @@ class DeviceRedirectionParser(Parser):
+            packetID = DeviceRedirectionPacketID(Uint16LE.unpack(stream))
+
+             if component == DeviceRedirectionComponent.RDPDR_CTYP_CORE and packetID in self.parsers.keys():
+    -            return self.parsers[packetID](stream)
+    +            ret = self.parsers[packetID](stream)
+    +            print(f"Component-specific parser returned\nPDU: {ret}\nRaw bytes: {data.hex()}")
+    +            return ret
+    +            #return self.parsers[packetID](stream)
+             else:
+    -            return DeviceRedirectionPDU(component, packetID, payload=stream.read())
+    +            ret = DeviceRedirectionPDU(component, packetID, payload=stream.read())
+    +            print(f"Generic parser returned\nPDU: {ret}\nRaw bytes: {data.hex()}")
+    +            return ret
+    +            #return DeviceRedirectionPDU(component, packetID, payload=stream.read())
+
+             def write(self, pdu: DeviceRedirectionPDU) -> bytes:
+                 stream = BytesIO()
+                 Uint16LE.pack(pdu.component, stream)
+                 Uint16LE.pack(pdu.packetID, stream)
+
+    +        #import ipdb; ipdb.set_trace()
+             if pdu.component == DeviceRedirectionComponent.RDPDR_CTYP_CORE and pdu.packetID in self.writers.keys():
+                 self.writers[pdu.packetID](pdu, stream)
+    +            print(f"Component-specific writer\nPDU: {pdu}\nRaw bytes: {stream.getvalue().hex()}")
+             else:
+                 stream.write(pdu.payload)
+    +            print(f"Generic writer\nPDU: {pdu}\nRaw bytes: {stream.getvalue().hex()}")
+
+             return stream.getvalue()
+
+
+Then run `pyrdp-mitm.py` with `| tee -a raw-logs.txt` and filter for the raw bytes
+lines and compare one line to the next. I'm doing this inside a ipython notebook
+with the following script:
+
+    # Prepare
+    !grep "Raw bytes" io-raw-bytes.log > io-raw-bytes-only.log
+
+    with open("io-raw-bytes-only.log") as _f:
+        first = _f.readline()
+        while first:
+            second = _f.readline()
+            if first != second:
+                print(first)
+                print(second)
+            first = _f.readline()
+
+Lines that don't match should be investigated because it means that we are not
+sending what we are receiving.
+
+
+== Wireshark
+
+Using the "Decode As" feature and TLS master secrets from the `ssl.log` logfile you can analyze the RDP traffic in Wireshark.
+For "Decode As" the inner protocol should be set to TPKT this will be automatic for port 3389.
+
+Olivier faced problems numerous times with wireshark where encapsulation would stay on TPKT instead of RDP or other RDP-related inner protocols.
+Resetting Wireshark's user configuration fixed the issue.
+Moving `~/.wireshark/` was enough.
+
+Extracting a single VirtualChannel (id 1004 for DeviceRedirection) raw bytes:
+
+    -d tls.port==13389,tpkt -T fields -e rdp.virtualChannelData -2 "t124.channelId == 1004"
diff --git a/docs/devel.adoc b/docs/devel.adoc
@@ -49,3 +49,10 @@ docker images (latest, slim), on linux, on Windows
 * Update version in `setup.py` (+1 bugfix, append '.dev0') and commit
 * commit msg: Begin development on next release
 
+
+== Logging
+
+By default we log to stdout and in JSON format. Please use the recommended
+logging style to best leverage the JSON output:
+
+https://docs.python.org/3/howto/logging-cookbook.html#formatting-styles
diff --git a/pyrdp/enum/rdp.py b/pyrdp/enum/rdp.py
@@ -561,6 +561,7 @@ class ErrorInfo(IntEnum):
     ERRINFO_SERVER_FRESH_CREDENTIALS_REQUIRED = 0x0000000A
     ERRINFO_RPC_INITIATED_DISCONNECT_BYUSER = 0x0000000B
     ERRINFO_LOGOFF_BY_USER = 0x0000000C
+    ERRINFO_SERVER_SHUTDOWN = 0x000000019
     ERRINFO_LICENSE_INTERNAL = 0x00000100
     ERRINFO_LICENSE_NO_LICENSE_SERVER = 0x00000101
     ERRINFO_LICENSE_NO_LICENSE = 0x00000102

diff --git a/pyrdp/layer/layer.py b/pyrdp/layer/layer.py
@@ -1,11 +1,11 @@
 #
 # This file is part of the PyRDP project.
-# Copyright (C) 2018, 2019, 2021 GoSecure Inc.
+# Copyright (C) 2018-2022 GoSecure Inc.
 # Licensed under the GPLv3 or later.
 #
 
 from abc import ABCMeta, abstractmethod
-from typing import Union
+from typing import List, Union
 
 from pyrdp.core import EventEngine, ObservedBy, Observer, Subject
 from pyrdp.exceptions import UnknownPDUTypeError, ParsingError
@@ -162,7 +162,7 @@ def __init__(self):
         self.next: BaseLayer = None
 
     @staticmethod
-    def chain(first: 'LayerChainItem', second: Union['BaseLayer', 'LayerChainItem'], *layers: [Union['BaseLayer', 'LayerChainItem']]):
+    def chain(first: 'LayerChainItem', second: Union['BaseLayer', 'LayerChainItem'], *layers: List[Union['BaseLayer', 'LayerChainItem']]):
         """
         Chain a series of layers together by calling setNext iteratively.
         :param first: first layer in the chain.

diff --git a/pyrdp/mitm/AttackerMITM.py b/pyrdp/mitm/AttackerMITM.py
@@ -6,7 +6,7 @@
 from collections import defaultdict
 from logging import LoggerAdapter
 from pathlib import Path
-from typing import Dict, Optional
+from typing import Dict, List, Optional
 from functools import partial
 
 from pyrdp.enum import FastPathInputType, FastPathOutputType, MouseButton, PlayerPDUType, PointerFlag, ScanCodeTuple
@@ -83,16 +83,16 @@ def onPDUReceived(self, pdu: PlayerPDU):
             self.handlers[pdu.header](pdu)
 
 
-    def sendInputEvents(self, events: [FastPathInputEvent]):
+    def sendInputEvents(self, events: List[FastPathInputEvent]):
         pdu = FastPathPDU(0, events)
         self.recorder.record(pdu, PlayerPDUType.FAST_PATH_INPUT, True)
         self.server.sendPDU(pdu)
 
-    def sendOutputEvents(self, events: [FastPathOutputEvent]):
+    def sendOutputEvents(self, events: List[FastPathOutputEvent]):
         pdu = FastPathPDU(0, events)
         self.client.sendPDU(pdu)
 
-    def sendKeys(self, keys: [ScanCodeTuple]):
+    def sendKeys(self, keys: List[ScanCodeTuple]):
         for released in [False, True]:
             for key in keys:
                 self.handleKeyboard(PlayerKeyboardPDU(0, key.code, released, key.extended))

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

diff --git a/pyrdp/parser/parser.py b/pyrdp/parser/parser.py
@@ -5,8 +5,6 @@
 #
 from io import BytesIO
 
-import typing
-
 from pyrdp.core import FilePositionGuard
 from pyrdp.exceptions import ParsingError
 from pyrdp.pdu import PDU

diff --git a/pyrdp/parser/rdp/virtual_channel/device_redirection.py b/pyrdp/parser/rdp/virtual_channel/device_redirection.py
@@ -1,16 +1,17 @@
 #
 # This file is part of the PyRDP project.
-# Copyright (C) 2018, 2019, 2021 GoSecure Inc.
+# Copyright (C) 2018-2022 GoSecure Inc.
 # Licensed under the GPLv3 or later.
 #
 
 from io import BytesIO
 from typing import Dict, List, Union
 
 from pyrdp.core import decodeUTF16LE, Uint16LE, Uint32LE, Uint64LE, Uint8
-from pyrdp.enum import DeviceRedirectionComponent, DeviceRedirectionPacketID, DeviceType, FileAttributes, \
-    FileCreateDisposition, FileCreateOptions, FileShareAccess, FileSystemInformationClass, GeneralCapabilityVersion, \
-    MajorFunction, MinorFunction, RDPDRCapabilityType
+from pyrdp.enum import DeviceRedirectionComponent, DeviceRedirectionPacketID, \
+    DeviceType, FileAccessMask, FileAttributes, FileCreateDisposition, \
+    FileCreateOptions, FileShareAccess, FileSystemInformationClass, \
+    GeneralCapabilityVersion, MajorFunction, MinorFunction, RDPDRCapabilityType
 from pyrdp.parser import Parser
 from pyrdp.pdu import DeviceAnnounce, DeviceCloseRequestPDU, DeviceCloseResponsePDU, DeviceCreateRequestPDU, \
     DeviceCreateResponsePDU, DeviceDirectoryControlResponsePDU, DeviceIORequestPDU, DeviceIOResponsePDU, \
@@ -318,7 +319,7 @@ def writeDeviceIOResponse(self, pdu: DeviceIOResponsePDU, stream: BytesIO):
 
 
     def parseDeviceCreateRequest(self, deviceID: int, fileID: int, completionID: int, minorFunction: int, stream: BytesIO) -> DeviceCreateRequestPDU:
-        desiredAccess = Uint32LE.unpack(stream)
+        desiredAccess = FileAccessMask(Uint32LE.unpack(stream))
         allocationSize = Uint64LE.unpack(stream)
         fileAttributes = FileAttributes(Uint32LE.unpack(stream))
         sharedAccess = FileShareAccess(Uint32LE.unpack(stream))
@@ -344,7 +345,11 @@ def parseDeviceCreateRequest(self, deviceID: int, fileID: int, completionID: int
         )
 
     def writeDeviceCreateRequest(self, pdu: DeviceCreateRequestPDU, stream: BytesIO):
-        path = (pdu.path + "\x00").encode("utf-16le")
+        # A null path *does not* include the null terminator in its length
+        if len(pdu.path) == 0:
+            path = b''
+        else:
+            path = (pdu.path + "\x00").encode("utf-16le")
 
         Uint32LE.pack(pdu.desiredAccess, stream)
         Uint64LE.pack(pdu.allocationSize, stream)
@@ -374,7 +379,7 @@ def parseDeviceCreateResponse(self, deviceID: int, completionID: int, ioStatus:
 
     def writeDeviceCreateResponse(self, pdu: DeviceCreateResponsePDU, stream: BytesIO):
         Uint32LE.pack(pdu.fileID, stream)
-        Uint8.pack(pdu.information)
+        Uint8.pack(pdu.information, stream)
 
 
 
@@ -442,14 +447,19 @@ def writeDirectoryControlRequest(self, pdu: Union[DeviceIORequestPDU, DeviceQuer
             stream.write(pdu.payload)
         else:
             self.informationClassForParsingResponse[pdu.completionID] = pdu.informationClass
-            path = (pdu.path + "\x00").encode("utf-16le")
 
             Uint32LE.pack(pdu.informationClass, stream)
             Uint8.pack(pdu.initialQuery, stream)
-            Uint32LE.pack(len(path), stream)
-            stream.write(b"\x00" * 23)
-            stream.write(path)
 
+            # A null path *does not* include the null terminator in its length
+            if len(pdu.path) == 0:
+                stream.write(b"\x00" * 4) # 32-bit path length (empty)
+                stream.write(b"\x00" * 23) # protocol required padding
+            else:
+                path = (pdu.path + "\x00").encode("utf-16le")
+                Uint32LE.pack(len(path), stream)
+                stream.write(b"\x00" * 23) # protocol required padding
+                stream.write(path)
 
     def parseDirectoryControlResponse(self, deviceID: int, completionID: int, ioStatus: int, stream: BytesIO) -> DeviceIOResponsePDU:
         minorFunction = self.minorFunctionsForParsingResponse.pop(completionID, None)
@@ -507,7 +517,7 @@ def writeFileInformationList(self, dataList: List[bytes], stream: BytesIO):
 
     def parseFileDirectoryInformation(self, data: bytes) -> List[FileDirectoryInformation]:
         stream = BytesIO(data)
-        information: [FileDirectoryInformation] = []
+        information: List[FileDirectoryInformation] = []
 
         while stream.tell() < len(data):
             nextEntryOffset = Uint32LE.unpack(stream)
@@ -547,7 +557,7 @@ def parseFileDirectoryInformation(self, data: bytes) -> List[FileDirectoryInform
 
 
     def writeFileDirectoryInformation(self, information: List[FileDirectoryInformation], stream: BytesIO):
-        dataList: [bytes] = []
+        dataList: List[bytes] = []
 
         for info in information:
             substream = BytesIO()
@@ -571,7 +581,7 @@ def writeFileDirectoryInformation(self, information: List[FileDirectoryInformati
 
     def parseFileFullDirectoryInformation(self, data: bytes) -> List[FileFullDirectoryInformation]:
         stream = BytesIO(data)
-        information: [FileFullDirectoryInformation] = []
+        information: List[FileFullDirectoryInformation] = []
 
         while stream.tell() < len(data):
             nextEntryOffset = Uint32LE.unpack(stream)
@@ -612,7 +622,7 @@ def parseFileFullDirectoryInformation(self, data: bytes) -> List[FileFullDirecto
 
 
     def writeFileFullDirectoryInformation(self, information: List[FileFullDirectoryInformation], stream: BytesIO):
-        dataList: [bytes] = []
+        dataList: List[bytes] = []
 
         for info in information:
             substream = BytesIO()
@@ -637,7 +647,7 @@ def writeFileFullDirectoryInformation(self, information: List[FileFullDirectoryI
 
     def parseFileBothDirectoryInformation(self, data: bytes) -> List[FileBothDirectoryInformation]:
         stream = BytesIO(data)
-        information: [FileBothDirectoryInformation] = []
+        information: List[FileBothDirectoryInformation] = []
 
         while stream.tell() < len(data):
             nextEntryOffset = Uint32LE.unpack(stream)
@@ -683,7 +693,7 @@ def parseFileBothDirectoryInformation(self, data: bytes) -> List[FileBothDirecto
 
 
     def writeFileBothDirectoryInformation(self, information: List[FileBothDirectoryInformation], stream: BytesIO):
-        dataList: [bytes] = []
+        dataList: List[bytes] = []
 
         for info in information:
             substream = BytesIO()
@@ -712,7 +722,7 @@ def writeFileBothDirectoryInformation(self, information: List[FileBothDirectoryI
 
     def parseFileNamesInformation(self, data: bytes) -> List[FileNamesInformation]:
         stream = BytesIO(data)
-        information: [FileNamesInformation] = []
+        information: List[FileNamesInformation] = []
 
         while stream.tell() < len(data):
             nextEntryOffset = Uint32LE.unpack(stream)
@@ -733,7 +743,7 @@ def parseFileNamesInformation(self, data: bytes) -> List[FileNamesInformation]:
 
 
     def writeFileNamesInformation(self, information: List[FileNamesInformation], stream: BytesIO):
-        dataList: [bytes] = []
+        dataList: List[bytes] = []
 
         for info in information:
             substream = BytesIO()

diff --git a/pyrdp/pdu/rdp/fastpath.py b/pyrdp/pdu/rdp/fastpath.py
@@ -23,7 +23,7 @@ def __init__(self, payload: bytes = b""):
 
 
 class FastPathPDU(SegmentationPDU):
-    def __init__(self, header: int, events: [FastPathEvent]):
+    def __init__(self, header: int, events: List[FastPathEvent]):
         super().__init__(b"")
         self.header = header
         self.events = events

diff --git a/pyrdp/pdu/rdp/virtual_channel/device_redirection.py b/pyrdp/pdu/rdp/virtual_channel/device_redirection.py
@@ -1,14 +1,15 @@
 #
 # This file is part of the PyRDP project.
-# Copyright (C) 2018 GoSecure Inc.
+# Copyright (C) 2018, 2019, 2022 GoSecure Inc.
 # Licensed under the GPLv3 or later.
 #
 
 from typing import Dict, List, Optional
 
-from pyrdp.enum import DeviceRedirectionComponent, DeviceRedirectionPacketID, DeviceType, FileAttributes, \
-    FileCreateDisposition, FileCreateOptions, FileShareAccess, FileSystemInformationClass, MajorFunction, MinorFunction, \
-    RDPDRCapabilityType
+from pyrdp.enum import DeviceRedirectionComponent, DeviceRedirectionPacketID, \
+    DeviceType, FileAccessMask, FileAttributes, FileCreateDisposition, \
+    FileCreateOptions, FileShareAccess, FileSystemInformationClass, \
+    MajorFunction, MinorFunction, RDPDRCapabilityType
 from pyrdp.pdu import PDU
 
 
@@ -57,7 +58,7 @@ class DeviceCreateRequestPDU(DeviceIORequestPDU):
     """
 
     def __init__(self, deviceID: int, fileID: int, completionID: int, minorFunction: int,
-                 desiredAccess: int, allocationSize: int, fileAttributes: FileAttributes, sharedAccess: FileShareAccess,
+                 desiredAccess: FileAccessMask, allocationSize: int, fileAttributes: FileAttributes, sharedAccess: FileShareAccess,
                  createDisposition: FileCreateDisposition, createOptions: FileCreateOptions, path: str):
         super().__init__(deviceID, fileID, completionID, MajorFunction.IRP_MJ_CREATE, minorFunction)
         self.desiredAccess = desiredAccess