From 2be175cec4fbc1c7e23321f1e9ac3401986d0fab Mon Sep 17 00:00:00 2001 From: Zavadovsky Yan Date: Sun, 22 Feb 2015 00:07:25 +0300 Subject: [PATCH 1/2] libfreerdp-core: fix dead-end state in transport_read_pdu() Situation: we have fragmented TPKT PDU without two last bytes (or one last byte - for fast-path) in network stack. First call to transport_read_pdu() works normally, read available bytes and exit with status 0 - no whole PDU readed. Before second call this missed bytes arrive. Optionally with next PDU. In second call header parsing code unconditionally read this two bytes(one byte) despite this is not header bytes. And increase stream position, so stream now contains whole PDU. This cause (pduLength - Stream_GetPosition(s)) calculation to be 0. So transport_read_layer_bytes()-->transport_read_layer() return 0 and transport_read_pdu() exits with "not enough data is available" status. If next PDU isn't available next calls to transport_read_pdu() give same result. If next PDU arrive - (pduLength - Stream_GetPosition(s)) will be less than 0. Stream position will grow, grow and grow on each call. And transport_read_pdu() never signals that PDU is readed. Caught on Android FreeRDP client with high RDP traffic (several MBytes/s). --- libfreerdp/core/transport.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/libfreerdp/core/transport.c b/libfreerdp/core/transport.c index 2189b0b8c..f7ab8267c 100644 --- a/libfreerdp/core/transport.c +++ b/libfreerdp/core/transport.c @@ -422,6 +422,7 @@ int transport_read_pdu(rdpTransport* transport, wStream* s) return status; } + position = Stream_GetPosition(s); header = Stream_Buffer(s); if (transport->NlaMode) @@ -439,7 +440,8 @@ int transport_read_pdu(rdpTransport* transport, wStream* s) { if ((header[1] & ~(0x80)) == 1) { - if ((status = transport_read_layer_bytes(transport, s, 1)) != 1) + if (position < (2 + 1) + && (status = transport_read_layer_bytes(transport, s, 2 + 1 - position)) != 1) return status; pduLength = header[2]; @@ -447,7 +449,8 @@ int transport_read_pdu(rdpTransport* transport, wStream* s) } else if ((header[1] & ~(0x80)) == 2) { - if ((status = transport_read_layer_bytes(transport, s, 2)) != 1) + if (position < (2 + 2) + && (status = transport_read_layer_bytes(transport, s, 2 + 2 - position)) != 1) return status; pduLength = (header[2] << 8) | header[3]; @@ -471,7 +474,8 @@ int transport_read_pdu(rdpTransport* transport, wStream* s) if (header[0] == 0x03) { /* TPKT header */ - if ((status = transport_read_layer_bytes(transport, s, 2)) != 1) + if (position < (2 + 2) + && (status = transport_read_layer_bytes(transport, s, 2 + 2 - position)) != 1) return status; pduLength = (header[2] << 8) | header[3]; @@ -488,7 +492,8 @@ int transport_read_pdu(rdpTransport* transport, wStream* s) /* Fast-Path Header */ if (header[1] & 0x80) { - if ((status = transport_read_layer_bytes(transport, s, 1)) != 1) + if (position < (2 + 1) + && (status = transport_read_layer_bytes(transport, s, 2 + 1 - position)) != 1) return status; pduLength = ((header[1] & 0x7F) << 8) | header[2]; From ab8aedd801e164fb4fedb41f3e9d360f0e809c1c Mon Sep 17 00:00:00 2001 From: Zavadovsky Yan Date: Mon, 23 Feb 2015 13:57:46 +0300 Subject: [PATCH 2/2] libfreerdp-core: add comments for previous fix --- libfreerdp/core/transport.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/libfreerdp/core/transport.c b/libfreerdp/core/transport.c index f7ab8267c..696fedeed 100644 --- a/libfreerdp/core/transport.c +++ b/libfreerdp/core/transport.c @@ -422,6 +422,7 @@ int transport_read_pdu(rdpTransport* transport, wStream* s) return status; } + /* update position value for further checks */ position = Stream_GetPosition(s); header = Stream_Buffer(s); @@ -440,8 +441,9 @@ int transport_read_pdu(rdpTransport* transport, wStream* s) { if ((header[1] & ~(0x80)) == 1) { - if (position < (2 + 1) - && (status = transport_read_layer_bytes(transport, s, 2 + 1 - position)) != 1) + /* check for header bytes already was readed in previous calls */ + if (position < 3 + && (status = transport_read_layer_bytes(transport, s, 3 - position)) != 1) return status; pduLength = header[2]; @@ -449,8 +451,9 @@ int transport_read_pdu(rdpTransport* transport, wStream* s) } else if ((header[1] & ~(0x80)) == 2) { - if (position < (2 + 2) - && (status = transport_read_layer_bytes(transport, s, 2 + 2 - position)) != 1) + /* check for header bytes already was readed in previous calls */ + if (position < 4 + && (status = transport_read_layer_bytes(transport, s, 4 - position)) != 1) return status; pduLength = (header[2] << 8) | header[3]; @@ -474,8 +477,9 @@ int transport_read_pdu(rdpTransport* transport, wStream* s) if (header[0] == 0x03) { /* TPKT header */ - if (position < (2 + 2) - && (status = transport_read_layer_bytes(transport, s, 2 + 2 - position)) != 1) + /* check for header bytes already was readed in previous calls */ + if (position < 4 + && (status = transport_read_layer_bytes(transport, s, 4 - position)) != 1) return status; pduLength = (header[2] << 8) | header[3]; @@ -492,8 +496,9 @@ int transport_read_pdu(rdpTransport* transport, wStream* s) /* Fast-Path Header */ if (header[1] & 0x80) { - if (position < (2 + 1) - && (status = transport_read_layer_bytes(transport, s, 2 + 1 - position)) != 1) + /* check for header bytes already was readed in previous calls */ + if (position < 3 + && (status = transport_read_layer_bytes(transport, s, 3 - position)) != 1) return status; pduLength = ((header[1] & 0x7F) << 8) | header[2];