fix: make license parsing and protocol more resilient

[ibeckermayer]

A user encountered

ConnectorError(Error { context: "decode SERVER_PLATFORM_CHALLENGE", kind: Pdu(Error { context: "<ironrdp_pdu::rdp::server_license::BlobHeader as ironrdp_pdu::PduDecode>::decode::{{c
losure}}", kind: InvalidMessage { field: "blobType", reason: "invalid blob type" }, source: None }), source: None })

This appears to be a bug in the parsing code for SERVER_PLATFORM_CHALLENGE which contains a Licensing Binary BLOB<12> with the following footnote:

The wBlobType field is unused in EncryptedPlatformChallenge (part of SERVER_PLATFORM_CHALLENGE PDU), which is sent by the server to the client.

suggesting that the blob type may not be one of the enumerated expected values.

This PR re-introduces resilient parsing for this field.

Putting this in draft mode pending an in-field test by the user.

Update:

After attempting to connect over xrdp, I discovered a larger refactor should be done to improve the resiliency of the licensing protocol. This has also now been tested against the standard windows rdp server.

[CBenoit]

Thank you! Looks good to me! Let’s merge once the user confirms everything is good.

[github-actions]

Coverage Report 🤖 ⚙️

Past:
Total lines: 29646
Covered lines: 17248 (58.18%)

New:
Total lines: 29661
Covered lines: 17335 (58.44%)

Diff: +0.26%

[this comment will be updated automatically]

[ibeckermayer]

Substantial changes have been made since approval see the Update section in the description.

@CBenoit @pacmancoder

[pacmancoder]

@ibeckermayer Benoit is on vacation, I'll take a look at it today👍

[pacmancoder]

I have only a few minor comments to resolve before we merge, but overall a great job 👍

[pacmancoder]

I believe FrameAcknowledge should be moved above "other" comment as we already support/parse it

[ibeckermayer]

My understanding is that the groupings here are

• mandatory
• optional
• other: from specs besides [MS-RDPBCGR]

[pacmancoder]

Lets separate "unparsed" variants with the comment (// Not implemented) as in CapabilitySet

Thought: (for follow-up refactoring at some time in future?) I don't like the idea of mixing named variants with parsed and unparsed data. If we implement new variant which was previously "unparsed", then we break the API each time a new feature is added. Maybe we need to refactor ShareDataPduType to be a struct with const's like BlobType, and then we could refactor ShareDataPdu to make is non_exhaustive and add a single Other variant for unparsed data like so:

Other {
    kind: ShareDataPduType,
    data: Vec<u8>
}

The same thing for the Capability set and similar types which can't be implemented in one go or could be extended in RDP standards.

cc @CBenoit

[CBenoit]

I mostly agree! Here is an alternative:

struct ShareDataPduRaw<'a> {
    kind: ShareDataPduType,
    data: Cow<'a, [u8]>,
}

impl ShareDataPdu<'_> {
     fn parse(&self) -> Result<ShareDataPdu, …> { … }
}

With ShareDataPdu only including supported PDUs, and allowing graceful handling both known and unknown PDU types.

That being said, if the plan is to support all the different types soon enough, it’s probably fine to just add the new variants now instead. I don’t think the protocol will change that much by adding a new Share Data PDU type, and if it does, it will likely be controlled by compatibly flags during the handshake.

[ibeckermayer]

I'm going to punt on this one for now.

[CBenoit]

Thank you for this @ibeckermayer
I adjusted the name of the PR (for when it will be squashed). Otherwise I leave the rest to @pacmancoder 🙌

[pacmancoder]

Looks good to me!