Server sample & mstsc related connection fixes #198

elmarco · 2023-09-27T14:23:33Z

This is WIP branch to add a simple server, so we can further debug issues more easily.

In particular mstsc fails to connect succesfully, and I am having trouble to debug the issue. Notably, not being able to setup wireshark to decode the protocol...

Help welcome!

awakecoding · 2023-09-27T14:30:29Z

@elmarco luckily, I have just the right guide and tooling for you to properly decode mstsc traffic in Wireshark! This should help you diagnose the issue quickly: https://github.com/awakecoding/wireshark-rdp

elmarco · 2023-09-27T20:14:30Z

@elmarco luckily, I have just the right guide and tooling for you to properly decode mstsc traffic in Wireshark! This should help you diagnose the issue quickly: https://github.com/awakecoding/wireshark-rdp

thanks, unfortunately that didn't help me much :( I am trying to dump ironrdp, on server side (and sfreerdp-server). SSLKEYLOGFILE= doesnt seem to be taken into account there, I'll need to investigate why. I tried to set the RSA key list myself, but that didnt help either.. I am bit clueless, I would appreciate your help! :)

crates/ironrdp/examples/server.rs

awakecoding · 2023-09-27T20:55:33Z

@elmarco luckily, I have just the right guide and tooling for you to properly decode mstsc traffic in Wireshark! This should help you diagnose the issue quickly: https://github.com/awakecoding/wireshark-rdp

thanks, unfortunately that didn't help me much :( I am trying to dump ironrdp, on server side (and sfreerdp-server). SSLKEYLOGFILE= doesnt seem to be taken into account there, I'll need to investigate why. I tried to set the RSA key list myself, but that didnt help either.. I am bit clueless, I would appreciate your help! :)

is there a particular reason why the technique where you'd try to dump the SChannel pre-master secrets out of the Windows client machine running mstsc would not be suitable in this case? It's either that, or SSLKEYLOGFILE in the IronRDP server, but as my colleague Benoit pointed out there's a small code modification to be made there, I don't think it's enabled in the server right now.

CBenoit · 2023-09-29T04:02:54Z

(Converted the PR to an draft explicitly, given the current title)

elmarco · 2023-10-03T12:05:44Z

@elmarco luckily, I have just the right guide and tooling for you to properly decode mstsc traffic in Wireshark! This should help you diagnose the issue quickly: https://github.com/awakecoding/wireshark-rdp

I can get the decrypted TLS now. However, it doesn't interpret it as RDP. Any idea?

awakecoding · 2023-10-03T12:54:34Z

@elmarco hum... that's odd, normally the RDP dissector kicks in automatically when TLS gets decrypted. This doesn't look like the Windows version of Wireshark, maybe it's an older release on another OS? In any case, if you look at the last packets exchanged, can you spot anything interesting?

It may be easier to discuss some of this in the FreeRDP chat, which we use for more than just FreeRDP these days: https://www.freerdp.com/ (see the matrix.org channel, I hang in there from time to time).

I cloned your branch, and I'm unsure how to really get started running the sample server. Here's what I did so far:

cargo run --example=server --features=server -- --host 127.0.0.1 --port 4489

I see there's a parameter for a server certificate which I didn't pass, but I don't see how I'd be supposed to pass server username+password that the server would use to authenticate the client. If I can reproduce the issue on my side, I could probably get the Wireshark part right

elmarco · 2023-10-03T13:07:55Z

@elmarco hum... that's odd, normally the RDP dissector kicks in automatically when TLS gets decrypted. This doesn't look like the Windows version of Wireshark, maybe it's an older release on another OS? In any case, if you look at the last packets exchanged, can you spot anything interesting?

Wireshark 4.0.8 (Git commit 81696bb74857) (wireshark-4.0.8-2.fc38.x86_64)

It may be easier to discuss some of this in the FreeRDP chat, which we use for more than just FreeRDP these days: https://www.freerdp.com/ (see the matrix.org channel, I hang in there from time to time).

I am there too, ok.

I cloned your branch, and I'm unsure how to really get started running the sample server. Here's what I did so far:

cargo run --example=server --features=server -- --host 127.0.0.1 --port 4489

I see there's a parameter for a server certificate which I didn't pass, but I don't see how I'd be supposed to pass server username+password that the server would use to authenticate the client. If I can reproduce the issue on my side, I could probably get the Wireshark part right

There is no user/pass handling atm in ironrdp-server (I suppose it accepts all users): https://github.com/Devolutions/IronRDP/blob/master/crates/ironrdp-acceptor/src/connection.rs#L329

You need to pass cert/keys (I generated mine with something like openssl req -new -newkey rsa:4096 -x509 -sha256 -days 365 -nodes -out MyCertificate.crt -keyout MyKey.key)

awakecoding · 2023-10-03T13:09:52Z

For reference, if I launch it without a certificate, it'll fallback in pre-TLS mode which we're not really supposed to support (although it partially connected, which is surprising?). mstsc shows this warning which is indicative of a pre-TLS connection:

I then generated a self-signed RDP server certificate:

openssl genpkey -algorithm RSA -out rdp-server.key
openssl req -new -x509 -key rdp-server.key -out rdp-server.pem -days 365 -subj "/CN=rdp-server.local"
openssl x509 -in rdp-server.pem -text -noout

Then launched the sample RDP server again:

cargo run --example=server --features=server -- --host 172.22.121.189 --port 4489 --cert rdp-server.pem --key rdp-server.key

This time, I got the TLS certificate warning (expected, but confirming we're doing TLS):

I hit a protocol error in mstsc, and the IronRDP server shows the following errors:

2023-10-03T13:05:23.299307Z ERROR ironrdp_server::server: Connection error: [read frame by hint] custom error
2023-10-03T13:05:35.507127Z ERROR ironrdp_server::server: Connection error: [invalid payload] PDU error

I'll work on getting the traffic decrypted now. I'm still curious about where the credentials come into play, have you been working with RDP NLA, or TLS without NLA, which delegates the credentials later during the connection sequence?

elmarco · 2023-10-03T13:27:38Z

I hit a protocol error in mstsc, and the IronRDP server shows the following errors:

Yep, same error here. nice! :-)

awakecoding · 2023-10-03T13:30:16Z

I got the traffic decrypted in Wireshark, and see two fishy things:

A malformed packet sent by the server at the beginning, containing the GCC conference create response:

Later down the connection sequence, channel id 1003 is weirdly undissected, before causing a protocol error in the client:

With FreeRDP, I don't have a traffic capture, but I have a bunch of warnings:

 type=PDU_TYPE_DATA[0x00000007], tpktLength=22, remainingLength=16
 recv Synchronize Data PDU (0x1F), length: 8
 [CONNECTION_STATE_FINALIZATION_CLIENT_SYNC] received flag FINALIZE_SC_SYNCHRONIZE_PDU [0x00000001]
 CONNECTION_STATE_FINALIZATION_CLIENT_SYNC --> CONNECTION_STATE_FINALIZATION_CLIENT_COOPERATE
 x=38, y=59, w=1024, h=768
 x=1926, y=27, w=1024, h=768
 type=PDU_TYPE_DATA[0x00000007], tpktLength=26, remainingLength=20
 recv Control Data PDU (0x14), length: 12
 [CONNECTION_STATE_FINALIZATION_CLIENT_COOPERATE] received flag FINALIZE_SC_CONTROL_COOPERATE_PDU [0x00000002]
 CONNECTION_STATE_FINALIZATION_CLIENT_COOPERATE --> CONNECTION_STATE_FINALIZATION_CLIENT_GRANTED_CONTROL
 type=PDU_TYPE_DATA[0x00000007], tpktLength=26, remainingLength=20
 recv Control Data PDU (0x14), length: 12
 [CONNECTION_STATE_FINALIZATION_CLIENT_GRANTED_CONTROL] received flag FINALIZE_SC_CONTROL_GRANTED_PDU [0x00000004]
 CONNECTION_STATE_FINALIZATION_CLIENT_GRANTED_CONTROL --> CONNECTION_STATE_FINALIZATION_CLIENT_FONT_MAP
 type=PDU_TYPE_DATA[0x00000007], tpktLength=26, remainingLength=20
 recv Font Map Data PDU (0x28), length: 12
 [MS-RDPBCGR] 2.2.1.22.1 Font Map PDU Data (TS_FONT_MAP_PDU)::numberEntries != 0 [1]
 [MS-RDPBCGR] 2.2.1.22.1 Font Map PDU Data (TS_FONT_MAP_PDU)::totalNumEntries != 0 [1]
 [MS-RDPBCGR] 2.2.1.22.1 Font Map PDU Data (TS_FONT_MAP_PDU)::mapFlags != 0x0003 (FONTLIST_FIRST | FONTLIST_LAST) [0x0000]
 [MS-RDPBCGR] 2.2.1.22.1 Font Map PDU Data (TS_FONT_MAP_PDU)::entrySize != 4 [0]
 [CONNECTION_STATE_FINALIZATION_CLIENT_FONT_MAP] received flag FINALIZE_SC_FONT_MAP_PDU [0x00000008]
 CONNECTION_STATE_FINALIZATION_CLIENT_FONT_MAP --> CONNECTION_STATE_ACTIVE

CBenoit · 2023-10-05T15:16:07Z

Hi @elmarco!
You should rebase on top of master and fix the conflicts when you get a chance

CBenoit · 2023-10-11T18:21:29Z

crates/ironrdp-pdu/src/rdp/headers.rs

@@ -262,6 +262,7 @@ pub enum ShareDataPdu {
    FrameAcknowledge(FrameAcknowledgePdu),
    ServerSetErrorInfo(ServerSetErrorInfoPdu),
    Input(InputEventPdu),
+    ShutdownRequest,


Nice! You don’t have to address the client-side as part of this PR, but it’s also moving forward this other issue: #115

crates/ironrdp-pdu/src/rdp/capability_sets/general.rs

CBenoit · 2023-10-11T18:32:45Z

crates/ironrdp-pdu/src/gcc/conference_create.rs

-            gcc_blocks_buffer_length as u16 + CONFERENCE_RESPONSE_CONNECT_PDU_SIZE,
+            gcc_blocks_buffer_length as u16 + CONFERENCE_RESPONSE_CONNECT_PDU_SIZE + 1,


Good catch. I think this fix will break some tests. FYI a lot of tests in ironrdp-pdu are very old and I wouldn’t be surprised if some of them are snapshots generated using IronRDP itself (i.e.: not official samples from the documentation or extracted from an actual RDP session)

I am not sure why WireShark complains about this field though.. I tried to read their code, but it's not obvious :) anyway the field is marked as being ignored in RDP spec, so I guess we can just add +1 to make WS happy.

crates/ironrdp-server/src/capabilities.rs

crates/ironrdp-pdu/src/gcc/core_data/client.rs

crates/ironrdp-pdu/src/rdp/capability_sets/general.rs

CBenoit · 2023-10-16T15:32:41Z

FYI, the coverage workflow is now "fixed" for forks on master: a4fee87
CI should be green next time you rebase

Even when the server doesn't advertize its support, msctc sends this. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

To ease debugging scenario. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Support for other channel messages is currently lacking. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Used by the next patch during server negotiation. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Only accept channels we have attached. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Those flags are set both by client and server. Even if the server doesn't send those commands yet, this should be supported by most clients. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

It's needed for bitmap & surface commands etc. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

mstsc does not display bitmap update, for some reason. Using SetSurfaceBits instead solves it, and is required for more advanced codecs. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

CBenoit

LGTM! No blocker, I’ll merge and help with the follow up in a subsequent PR.
Thank you a lot for moving forward this!!

CBenoit · 2023-12-05T15:36:55Z

crates/ironrdp-pdu/src/rdp/refresh_rectangle.rs

+    }
+
+    fn to_buffer(&self, mut stream: impl io::Write) -> Result<(), Self::Error> {
+        stream.write_u8(self.areas_to_refresh.len() as u8)?;


Issue (non-blocking): cast_length! macro could be used to gracefully handle self.areas_to_refresh.len() being too big to be converted into a u8

I was wondering. I think there are a couple of other places that could also use it then.

CBenoit · 2023-12-05T15:39:28Z

crates/ironrdp-server/src/capabilities.rs

@@ -74,6 +75,8 @@ fn virtual_channel_capabilities() -> capability_sets::VirtualChannel {

 fn multifragment_update() -> capability_sets::MultifragmentUpdate {
    capability_sets::MultifragmentUpdate {
-        max_request_size: u32::MAX,
+        // FIXME: use an acceptable value for msctc.


Nitpick: add the issue number for easier cross-referencing

Suggested change

// FIXME: use an acceptable value for msctc.

// FIXME(#318): use an acceptable value for msctc.

CBenoit · 2023-12-05T16:01:05Z

crates/ironrdp-server/src/server.rs

-        let mut buffer = vec![0u8; 4096];
-        let mut encoder = UpdateEncoder::new();
+        debug!("Starting client loop");
+        self.io_channel_id = Some(result.io_channel_id);


Issue (non-blocking): we never handle more than one client at a time, so there is no actual problem, but I feel mixed about this approach of updating a global state (as far as the server is concerned) for something which is only relevant for the current client. This could lead to confusion, especially since this state isn't reset after client disconnection, leaving a redundant "dummy" value.

Thought: that being said, we know it’s technically always the same value, so the "dummy" value will always be the "good" value.

Suggestion: two avenues:

Remove the io_channel_id from RdpServer: it’s possible to just keep the value in the function of the body and to give it to handle_x224

Go the "constant" route completely. I directed otherwise in a previous comment, but on second thought it was not bad all things considered. The only request I have is to keep things consistent, i.e., use the constant in the connector itself too (previously the approach was mixed).

CBenoit · 2023-12-05T16:13:43Z

crates/ironrdp-server/src/server.rs

+                if Some(data.channel_id) == self.io_channel_id && self.handle_io_channel_data(data).await? {
+                    return Ok(true);
                }


Suggestion: a more idiomatic approach would be to call handle_io_channel_data inside the if body and avoid nesting control flow inside the condition expression itself.

Suggested change

if Some(data.channel_id) == self.io_channel_id && self.handle_io_channel_data(data).await? {

return Ok(true);

}

if Some(data.channel_id) == self.io_channel_id {

return self.handle_io_channel_data(data).await;

}

CBenoit · 2023-12-05T16:27:48Z

crates/ironrdp/examples/server.rs

+}
+
+#[derive(Debug)]
+struct InnerHandler {}


Issue (non-blocking): this appears to be dead code. Strangely enough, the lint is not fired.

elmarco · 2023-12-05T18:42:42Z

LGTM! No blocker, I’ll merge and help with the follow up in a subsequent PR. Thank you a lot for moving forward this!!

thanks a lot,

btw are you on #ironrdp on matrix?

CBenoit · 2023-12-05T19:16:47Z

btw are you on #ironrdp on matrix?

Just joined the room!

CBenoit added the A-server Area: RDP server label Sep 27, 2023

CBenoit reviewed Sep 27, 2023

View reviewed changes

crates/ironrdp/examples/server.rs Outdated Show resolved Hide resolved

CBenoit marked this pull request as draft September 29, 2023 04:02

CBenoit self-assigned this Sep 29, 2023

elmarco force-pushed the server-sample branch from e74858b to 7a4aa8b Compare October 3, 2023 12:56

elmarco force-pushed the server-sample branch from 7a4aa8b to 175d7b7 Compare October 11, 2023 13:26

CBenoit reviewed Oct 11, 2023

View reviewed changes

crates/ironrdp-pdu/src/rdp/capability_sets/general.rs Outdated Show resolved Hide resolved

CBenoit reviewed Oct 11, 2023

View reviewed changes

crates/ironrdp-server/src/capabilities.rs Show resolved Hide resolved

CBenoit reviewed Oct 11, 2023

View reviewed changes

crates/ironrdp-pdu/src/gcc/core_data/client.rs Show resolved Hide resolved

elmarco force-pushed the server-sample branch from 175d7b7 to 8ff6bc6 Compare October 16, 2023 11:23

CBenoit reviewed Oct 16, 2023

View reviewed changes

crates/ironrdp-pdu/src/rdp/capability_sets/general.rs Show resolved Hide resolved

elmarco force-pushed the server-sample branch from 8ff6bc6 to 21b8fa2 Compare October 17, 2023 07:12

elmarco changed the title ~~Draft: Server sample~~ Server sample & mstsc related connection fixes Oct 17, 2023

elmarco force-pushed the server-sample branch 3 times, most recently from fe4ace9 to ca4e3d3 Compare October 18, 2023 12:59

elmarco added 5 commits December 5, 2023 15:37

feat(pdu): teach ShareDataPdu about SuppressOutput

d5e7cd6

Even when the server doesn't advertize its support, msctc sends this. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

feat(pdu): teach ShareDataPdu about RefreshRectangle

be21f35

Even when the server doesn't advertize its support, msctc sends this. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

misc(pdu): log pdu_type on error

de590a9

To ease debugging scenario. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

misc(session): simplify a bit the code

bd864ad

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

feat(acceptor): add {io,user}_channel_id in AcceptorResult

7cb0b83

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

elmarco force-pushed the server-sample branch from 39e279a to d851dd5 Compare December 5, 2023 11:41

elmarco added 8 commits December 5, 2023 15:51

fix(server): handle only IO data at this point

6a25199

Support for other channel messages is currently lacking. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

feat(svc): add StaticChannelSet::get_by_channel_name()

f37081f

Used by the next patch during server negotiation. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

feat(acceptor): add basic static channel handling

9d2c524

Only accept channels we have attached. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

feat(server): advertize support for all surface commands

c3624fe

Those flags are set both by client and server. Even if the server doesn't send those commands yet, this should be supported by most clients. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

feat(server): check the client supports fastpath

6c01c68

It's needed for bitmap & surface commands etc. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

feat(server): pass the surface flags to the UpdateEncoder

bf89522

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

feat(server): use SetSurfaceBits if possible

f86b7ee

mstsc does not display bitmap update, for some reason. Using SetSurfaceBits instead solves it, and is required for more advanced codecs. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

feat: add simple server example

cbc682e

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

elmarco force-pushed the server-sample branch from d851dd5 to cbc682e Compare December 5, 2023 11:52

CBenoit approved these changes Dec 5, 2023

View reviewed changes

CBenoit merged commit b611cda into Devolutions:master Dec 5, 2023

CBenoit added a commit that referenced this pull request Dec 5, 2023

refactor: follow-up for PR #198

c5f56b2

CBenoit added a commit that referenced this pull request Dec 5, 2023

refactor: follow-up for PR #198

421f7fc

CBenoit mentioned this pull request Dec 5, 2023

refactor: follow-up for PR #198 #320

Merged

CBenoit added a commit that referenced this pull request Dec 5, 2023

refactor: follow-up for PR #198

31f3e4a

CBenoit added a commit that referenced this pull request Dec 5, 2023

refactor: follow-up for PR #198

9903d48

CBenoit added a commit that referenced this pull request Dec 5, 2023

refactor: follow-up for PR #198

526b17c

CBenoit added a commit that referenced this pull request Dec 5, 2023

refactor: follow-up for PR #198 (#320)

9355ae5

irvingoujAtDevolution mentioned this pull request Mar 25, 2024

feat(ffi): initial C# bindings (connector only) #423

Merged

irvingoujAtDevolution mentioned this pull request Apr 8, 2024

feat(ffi): screenshot example using C# bindings #437

Merged

irvingoujAtDevolution mentioned this pull request Apr 22, 2024

feat(ffi): add a client example using Avalonia #443

Merged

irvingoujAtDevolution mentioned this pull request May 1, 2024

chore(ffi): add avalonia example to top level .sln #453

Merged

		gcc_blocks_buffer_length as u16 + CONFERENCE_RESPONSE_CONNECT_PDU_SIZE,
		gcc_blocks_buffer_length as u16 + CONFERENCE_RESPONSE_CONNECT_PDU_SIZE + 1,

	// FIXME: use an acceptable value for msctc.
	// FIXME(#318): use an acceptable value for msctc.

Server sample & mstsc related connection fixes #198

Server sample & mstsc related connection fixes #198

Uh oh!

Conversation

elmarco commented Sep 27, 2023

Uh oh!

awakecoding commented Sep 27, 2023

Uh oh!

elmarco commented Sep 27, 2023

Uh oh!

Uh oh!

awakecoding commented Sep 27, 2023

Uh oh!

CBenoit commented Sep 29, 2023

Uh oh!

elmarco commented Oct 3, 2023

Uh oh!

awakecoding commented Oct 3, 2023

Uh oh!

elmarco commented Oct 3, 2023

Uh oh!

awakecoding commented Oct 3, 2023

Uh oh!

elmarco commented Oct 3, 2023

Uh oh!

awakecoding commented Oct 3, 2023

Uh oh!

CBenoit commented Oct 5, 2023

Uh oh!

CBenoit Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CBenoit commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CBenoit left a comment

Choose a reason for hiding this comment

Uh oh!

CBenoit Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elmarco commented Dec 5, 2023

Uh oh!

CBenoit commented Dec 5, 2023

Uh oh!

Uh oh!

CBenoit Oct 11, 2023 •

edited

Loading

CBenoit commented Oct 16, 2023 •

edited

Loading

CBenoit Dec 5, 2023 •

edited

Loading