feat(rdpdr): `DR_CORE_SERVER_CLIENTID_CONFIRM` and `DR_CORE_DEVICELIST_ANNOUNCE` #193

ibeckermayer · 2023-09-13T02:35:30Z

Adds handling for DR_CORE_SERVER_CLIENTID_CONFIRM and DR_CORE_DEVICELIST_ANNOUNCE/DR_CORE_DEVICELIST_ANNOUNCE_REQ, the next steps in the rdpdr initialization sequence.

…need to iterate on how to pass in the desired device_list

…eHeader> Unfortunately these structs rely on taking ownership of the underlying data when it's used. This would work very nicely/efficiently, except under normal circumstances ironrdp-client connects and then reconnects during a session, which means that in the second run (the one that the user actually interacts with) the data is gone, and empty lists are sent for both of these values. In the next commit I will fix this by changing take() to take_clone().

CBenoit

Looking quite good to me.
Small request: can you add your PDU structures to the pdu_decode fuzzing oracle?

crates/ironrdp-rdpdr/src/lib.rs

crates/ironrdp-rdpdr/src/pdu/efs.rs

ibeckermayer · 2023-09-18T18:39:42Z

Looking quite good to me. Small request: can you add your PDU structures to the pdu_decode fuzzing oracle?

b9b2ba9

CBenoit

Thank you, looks good to me!

github-actions · 2023-09-18T18:47:44Z

Coverage Report 🤖 ⚙️

Past:
Total lines: 22400
Covered lines: 15615 (69.71%)

New:
Total lines: 23062
Covered lines: 15626 (67.76%)

Diff: -1.95%

[this comment will be updated automatically]

pacmancoder

LGTM

CBenoit · 2023-09-19T13:46:09Z

Unfortunately these structs rely on taking ownership of the underlying
data when it's used. This would work very nicely/efficiently, except
under normal circumstances ironrdp-client connects and then reconnects
during a session, which means that in the second run (the one that the user
actually interacts with) the data is gone, and empty lists are sent for
both of these values.

This is very surprising by the way. Currently ironrdp-client is reconnecting from scratch and the Rdpdr channel is thus constructed from scratch as well, meaning with_smartcard is called again. I would expect the data to be populated again. Why are we actually ending up with empty lists?

ibeckermayer · 2023-09-20T22:22:49Z

Unfortunately these structs rely on taking ownership of the underlying
data when it's used. This would work very nicely/efficiently, except
under normal circumstances ironrdp-client connects and then reconnects
during a session, which means that in the second run (the one that the user
actually interacts with) the data is gone, and empty lists are sent for
both of these values.

This is very surprising by the way. Currently ironrdp-client is reconnecting from scratch and the Rdpdr channel is thus constructed from scratch as well, meaning with_smartcard is called again. I would expect the data to be populated again. Why are we actually ending up with empty lists?

@CBenoit you're correct, I was misinterpreting the logs. What I'm seeing on that commit is that we're going through the RDPDR initialization sequence twice in a single connection:

2023-09-20T21:49:23.076710Z TRACE ironrdp_rdpdr: received RdpdrPdu(VersionAndIdPdu { version_major: 1, version_minor: 13, client_id: 3, kind: ServerAnnounceRequest })
2023-09-20T21:49:23.076720Z TRACE ironrdp_rdpdr: sending RdpdrPdu(VersionAndIdPdu { version_major: 1, version_minor: 12, client_id: 3, kind: ClientAnnounceReply })
2023-09-20T21:49:23.076727Z TRACE ironrdp_rdpdr: sending RdpdrPdu(Unicode("IronRDP"))
2023-09-20T21:49:23.308538Z TRACE ironrdp_rdpdr: received RdpdrPdu(CoreCapability { padding: 0, capabilities: [CapabilityMessage { header: CapabilityHeader { cap_type: General, length: 44, version: 2 }, capability_data: General(GeneralCapabilitySet { os_type: 2, os_version: 0, protocol_major_version: 1, protocol_minor_version: 13, io_code_1: IoCode1(RDPDR_IRP_MJ_CREATE | RDPDR_IRP_MJ_CLEANUP | RDPDR_IRP_MJ_CLOSE | RDPDR_IRP_MJ_READ | RDPDR_IRP_MJ_WRITE | RDPDR_IRP_MJ_FLUSH_BUFFERS | RDPDR_IRP_MJ_SHUTDOWN | RDPDR_IRP_MJ_DEVICE_CONTROL | RDPDR_IRP_MJ_QUERY_VOLUME_INFORMATION | RDPDR_IRP_MJ_SET_VOLUME_INFORMATION | RDPDR_IRP_MJ_QUERY_INFORMATION | RDPDR_IRP_MJ_SET_INFORMATION | RDPDR_IRP_MJ_DIRECTORY_CONTROL | RDPDR_IRP_MJ_LOCK_CONTROL | RDPDR_IRP_MJ_QUERY_SECURITY | RDPDR_IRP_MJ_SET_SECURITY), io_code_2: 0, extended_pdu: ExtendedPdu(RDPDR_DEVICE_REMOVE_PDUS | RDPDR_CLIENT_DISPLAY_NAME_PDU | RDPDR_USER_LOGGEDON_PDU), extra_flags_1: ExtraFlags1(0x0), extra_flags_2: 0, special_type_device_cap: 2 }) }, CapabilityMessage { header: CapabilityHeader { cap_type: Printer, length: 8, version: 1 }, capability_data: Printer }, CapabilityMessage { header: CapabilityHeader { cap_type: Port, length: 8, version: 1 }, capability_data: Port }, CapabilityMessage { header: CapabilityHeader { cap_type: Drive, length: 8, version: 2 }, capability_data: Drive }, CapabilityMessage { header: CapabilityHeader { cap_type: Smartcard, length: 8, version: 1 }, capability_data: Smartcard }], kind: ServerCoreCapabilityRequest })
2023-09-20T21:49:23.308548Z TRACE ironrdp_rdpdr: sending RdpdrPdu(CoreCapability { padding: 0, capabilities: [CapabilityMessage { header: CapabilityHeader { cap_type: General, length: 44, version: 2 }, capability_data: General(GeneralCapabilitySet { os_type: 0, os_version: 0, protocol_major_version: 1, protocol_minor_version: 12, io_code_1: IoCode1(RDPDR_IRP_MJ_CREATE | RDPDR_IRP_MJ_CLEANUP | RDPDR_IRP_MJ_CLOSE | RDPDR_IRP_MJ_READ | RDPDR_IRP_MJ_WRITE | RDPDR_IRP_MJ_FLUSH_BUFFERS | RDPDR_IRP_MJ_SHUTDOWN | RDPDR_IRP_MJ_DEVICE_CONTROL | RDPDR_IRP_MJ_QUERY_VOLUME_INFORMATION | RDPDR_IRP_MJ_SET_VOLUME_INFORMATION | RDPDR_IRP_MJ_QUERY_INFORMATION | RDPDR_IRP_MJ_SET_INFORMATION | RDPDR_IRP_MJ_DIRECTORY_CONTROL | RDPDR_IRP_MJ_LOCK_CONTROL), io_code_2: 0, extended_pdu: ExtendedPdu(RDPDR_DEVICE_REMOVE_PDUS | RDPDR_CLIENT_DISPLAY_NAME_PDU), extra_flags_1: ExtraFlags1(0x0), extra_flags_2: 0, special_type_device_cap: 1 }) }, CapabilityMessage { header: CapabilityHeader { cap_type: Smartcard, length: 8, version: 1 }, capability_data: Smartcard }], kind: ClientCoreCapabilityResponse })
2023-09-20T21:49:23.308569Z TRACE ironrdp_rdpdr: received RdpdrPdu(VersionAndIdPdu { version_major: 1, version_minor: 13, client_id: 3, kind: ServerClientIdConfirm })
2023-09-20T21:49:23.308572Z TRACE ironrdp_rdpdr: sending RdpdrPdu(ClientDeviceListAnnounce { device_list: [DeviceAnnounceHeader { device_type: Smartcard, device_id: 0, preferred_dos_name: PreferredDosName("SCARD"), device_data: [] }] })
2023-09-20T21:49:23.411110Z TRACE ironrdp_rdpdr: received RdpdrPdu::Unimplemented
2023-09-20T21:49:23.411115Z  WARN ironrdp_rdpdr: received unimplemented packet: RdpdrPdu::Unimplemented

----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

2023-09-20T21:49:24.045425Z TRACE ironrdp_rdpdr: received RdpdrPdu(VersionAndIdPdu { version_major: 1, version_minor: 13, client_id: 2, kind: ServerAnnounceRequest })
2023-09-20T21:49:24.045439Z TRACE ironrdp_rdpdr: sending RdpdrPdu(VersionAndIdPdu { version_major: 1, version_minor: 12, client_id: 2, kind: ClientAnnounceReply })
2023-09-20T21:49:24.045447Z TRACE ironrdp_rdpdr: sending RdpdrPdu(Unicode("IronRDP"))
2023-09-20T21:49:24.133821Z TRACE ironrdp_rdpdr: received RdpdrPdu(CoreCapability { padding: 0, capabilities: [CapabilityMessage { header: CapabilityHeader { cap_type: General, length: 44, version: 2 }, capability_data: General(GeneralCapabilitySet { os_type: 2, os_version: 0, protocol_major_version: 1, protocol_minor_version: 13, io_code_1: IoCode1(RDPDR_IRP_MJ_CREATE | RDPDR_IRP_MJ_CLEANUP | RDPDR_IRP_MJ_CLOSE | RDPDR_IRP_MJ_READ | RDPDR_IRP_MJ_WRITE | RDPDR_IRP_MJ_FLUSH_BUFFERS | RDPDR_IRP_MJ_SHUTDOWN | RDPDR_IRP_MJ_DEVICE_CONTROL | RDPDR_IRP_MJ_QUERY_VOLUME_INFORMATION | RDPDR_IRP_MJ_SET_VOLUME_INFORMATION | RDPDR_IRP_MJ_QUERY_INFORMATION | RDPDR_IRP_MJ_SET_INFORMATION | RDPDR_IRP_MJ_DIRECTORY_CONTROL | RDPDR_IRP_MJ_LOCK_CONTROL | RDPDR_IRP_MJ_QUERY_SECURITY | RDPDR_IRP_MJ_SET_SECURITY), io_code_2: 0, extended_pdu: ExtendedPdu(RDPDR_DEVICE_REMOVE_PDUS | RDPDR_CLIENT_DISPLAY_NAME_PDU | RDPDR_USER_LOGGEDON_PDU), extra_flags_1: ExtraFlags1(0x0), extra_flags_2: 0, special_type_device_cap: 2 }) }, CapabilityMessage { header: CapabilityHeader { cap_type: Printer, length: 8, version: 1 }, capability_data: Printer }, CapabilityMessage { header: CapabilityHeader { cap_type: Port, length: 8, version: 1 }, capability_data: Port }, CapabilityMessage { header: CapabilityHeader { cap_type: Drive, length: 8, version: 2 }, capability_data: Drive }, CapabilityMessage { header: CapabilityHeader { cap_type: Smartcard, length: 8, version: 1 }, capability_data: Smartcard }], kind: ServerCoreCapabilityRequest })
2023-09-20T21:49:24.133851Z TRACE ironrdp_rdpdr: sending RdpdrPdu(CoreCapability { padding: 0, capabilities: [], kind: ClientCoreCapabilityResponse })
2023-09-20T21:49:24.133991Z TRACE ironrdp_rdpdr: received RdpdrPdu(VersionAndIdPdu { version_major: 1, version_minor: 13, client_id: 2, kind: ServerClientIdConfirm })
2023-09-20T21:49:24.133999Z TRACE ironrdp_rdpdr: sending RdpdrPdu(ClientDeviceListAnnounce { device_list: [] })

On the second time I'm sending back the empty ClientDeviceListAnnounce because it was already taken.

I'm not seeing this same behavior in Teleport. It may be being caused by the fact that we announce the smartcard capability but then currently don't handle one of the smartcard commands the server sends to us. I will continue development and then revisit whether this solves itself.

CBenoit · 2023-09-20T22:28:34Z

Oh okay, I see. Sounds good to me!

Isaiah Becker-Mayer added 4 commits August 29, 2023 17:27

Adds ClientDeviceListAnnounce. The API is not quite yet finalized, I …

d1111fb

…need to iterate on how to pass in the desired device_list

Adds take_clone so that reconnects work properly

5959e5e

Renames take_clone to clone_inner

8e23d79

ibeckermayer requested review from pacmancoder and CBenoit and removed request for pacmancoder September 13, 2023 02:35

CBenoit self-assigned this Sep 13, 2023

CBenoit reviewed Sep 13, 2023

View reviewed changes

crates/ironrdp-rdpdr/src/lib.rs Outdated Show resolved Hide resolved

crates/ironrdp-rdpdr/src/pdu/efs.rs Outdated Show resolved Hide resolved

Isaiah Becker-Mayer added 3 commits September 18, 2023 11:35

removes unneeded Default

4660c38

vec\![] -> Vec::new()

d429a94

Adds RdpdrPdu decode to fuzzing oracle

b9b2ba9

ibeckermayer requested a review from CBenoit September 18, 2023 18:39

CBenoit approved these changes Sep 18, 2023

View reviewed changes

pacmancoder approved these changes Sep 19, 2023

View reviewed changes

ibeckermayer mentioned this pull request Sep 20, 2023

Check whether RDPDR initialization sequence occurs twice #195

Open

CBenoit merged commit fe1567c into master Sep 20, 2023
6 checks passed

CBenoit deleted the feat/next-rdpdr-init-sequence branch September 20, 2023 22:29

CBenoit added the A-virtual-channel Area: Static or dynamic virtual channel label Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rdpdr): `DR_CORE_SERVER_CLIENTID_CONFIRM` and `DR_CORE_DEVICELIST_ANNOUNCE` #193

feat(rdpdr): `DR_CORE_SERVER_CLIENTID_CONFIRM` and `DR_CORE_DEVICELIST_ANNOUNCE` #193

ibeckermayer commented Sep 13, 2023

CBenoit left a comment

ibeckermayer commented Sep 18, 2023

CBenoit left a comment

github-actions bot commented Sep 18, 2023

pacmancoder left a comment

CBenoit commented Sep 19, 2023 •

edited

Loading

ibeckermayer commented Sep 20, 2023

CBenoit commented Sep 20, 2023

feat(rdpdr): DR_CORE_SERVER_CLIENTID_CONFIRM and DR_CORE_DEVICELIST_ANNOUNCE #193

feat(rdpdr): DR_CORE_SERVER_CLIENTID_CONFIRM and DR_CORE_DEVICELIST_ANNOUNCE #193

Conversation

ibeckermayer commented Sep 13, 2023

CBenoit left a comment

Choose a reason for hiding this comment

ibeckermayer commented Sep 18, 2023

CBenoit left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2023

Coverage Report 🤖 ⚙️

pacmancoder left a comment

Choose a reason for hiding this comment

CBenoit commented Sep 19, 2023 • edited Loading

ibeckermayer commented Sep 20, 2023

CBenoit commented Sep 20, 2023

feat(rdpdr): `DR_CORE_SERVER_CLIENTID_CONFIRM` and `DR_CORE_DEVICELIST_ANNOUNCE` #193

feat(rdpdr): `DR_CORE_SERVER_CLIENTID_CONFIRM` and `DR_CORE_DEVICELIST_ANNOUNCE` #193

CBenoit commented Sep 19, 2023 •

edited

Loading