From: Marc-André M. <mar...@gm...> - 2010-03-26 15:26:29
|
Actually, the more I look at it, the more I find that the root of what comes in the way of TLS + NLA is the way the rdp_ structures are assuming packets are encapsulated. For instance, some MCS packets are sent unencrypted with legacy RDP encryption, while they are sent over TLS when TLS is in use. Some packets such as for the network level authentication have nothing to do with TPKT, X.224 and MCS but are still over TLS. The current encapsulation assumed by the structures is the following: rdp->sec->mcs->iso->tcp This really is a problem and creates ugly code when trying to use tcp directly from places where "sec->mcs->iso" is assumed but isn't the case. What do you guys think of changing this to: rdp->tcp rdp->sec rdp->iso rdp->mcs Which would make it easier and cleaner to make parts of the protocol not in the assumed encapsulation order. Also, I really think we should rename iso to something else. ISO is the organisation name... not a standards name. Most of the time packets with a TPKT header contain an X.224 TPDU which may in turn contain an MCS payload which would then contain the RDP payload. Maybe we could rename iso to x224, but without explicitely adding anything for TPKT which is mostly just a simple header (we'd have a few function calls with tpkt_ in its name, but not a separate file or layer just for TPKT). Any comments? On Thu, Mar 25, 2010 at 9:43 PM, Mads Kiilerich <ma...@ki...> wrote: > Marc-André Moreau wrote, On 03/26/2010 12:23 AM: > > The current code uses different hacks to figure out where to dispatch the >> packet once received. For instance, a TPKT header starts with a one-byte >> field called "Version". That field is extracted and then used to check if >> it's a valid TPKT header (version should be 3). But then this field is >> stored in a variable "rdpVer" which is used in other parts of the program to >> take other decisions. If it's not a valid TPKT header you also have a piece >> of a code that starts digging further the packet to extract a "length" field >> from an ASN.1 encoded packet... it's not hard to see that some cleaning is >> required in that part of the code, and it is absolutely essential to >> refactor it in order to implement TLS + NLA. >> > > Please see my "More correct parsing of Server Redirection Packet" which > also needed and did some cleanups in this area. > > I tend to think that the current implementation is more layered than the > RDP protocol is, and thus we don't get the usual benefits of structured and > layered code. It is for example odd that rdpver handling lives in secure.c. > Perhaps secure.c should be a utility library and not a layer trying to hide > mcs. > > /Mads > > |