From da3114007cc22d7fc8595beeb21fc230e33e9cc2 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Tue, 14 Apr 2020 11:01:52 +0100 Subject: [PATCH] Address possible memory out-of-bounds accesses --- common/trans.c | 9 ++-- libxrdp/libxrdp.c | 14 ++--- libxrdp/xrdp_iso.c | 129 ++++++++++++++++++++++++++++++--------------- 3 files changed, 94 insertions(+), 58 deletions(-) diff --git a/common/trans.c b/common/trans.c index 883d7cbd..27efa8f8 100644 --- a/common/trans.c +++ b/common/trans.c @@ -452,17 +452,14 @@ trans_force_read_s(struct trans *self, struct stream *in_s, int size) { int rcvd; - if (self->status != TRANS_STATUS_UP) + if (self->status != TRANS_STATUS_UP || + size < 0 || !s_check_rem_out(in_s, size)) { return 1; } + while (size > 0) { - /* make sure stream has room */ - if ((in_s->end + size) > (in_s->data + in_s->size)) - { - return 1; - } rcvd = self->trans_recv(self, in_s->end, size); if (rcvd == -1) { diff --git a/libxrdp/libxrdp.c b/libxrdp/libxrdp.c index 27e2c3b7..0160bcc8 100644 --- a/libxrdp/libxrdp.c +++ b/libxrdp/libxrdp.c @@ -125,24 +125,18 @@ libxrdp_force_read(struct trans* trans) if (trans_force_read(trans, 4) != 0) { - g_writeln("libxrdp_force_read: error"); + g_writeln("libxrdp_force_read: header read error"); return 0; } bytes = libxrdp_get_pdu_bytes(s->data); - if (bytes < 1) + if (bytes < 4 || bytes > s->size) { - g_writeln("libxrdp_force_read: error"); + g_writeln("libxrdp_force_read: bad header length %d", bytes); return 0; } - if (bytes > 32 * 1024) - { - g_writeln("libxrdp_force_read: error"); - return 0; - } - if (trans_force_read(trans, bytes - 4) != 0) { - g_writeln("libxrdp_force_read: error"); + g_writeln("libxrdp_force_read: Can't read PDU"); return 0; } return s; diff --git a/libxrdp/xrdp_iso.c b/libxrdp/xrdp_iso.c index a53af5ce..27d5a57a 100644 --- a/libxrdp/xrdp_iso.c +++ b/libxrdp/xrdp_iso.c @@ -129,6 +129,12 @@ xrdp_iso_process_rdp_neg_req(struct xrdp_iso *self, struct stream *s) int flags; int len; + if (!s_check_rem(s, 7)) + { + LLOGLN(10, ("xrdp_iso_process_rdpNegReq: unexpected end-of-record")); + return 1; + } + in_uint8(s, flags); if (flags != 0x0 && flags != 0x8 && flags != 0x1) { @@ -153,13 +159,22 @@ xrdp_iso_process_rdp_neg_req(struct xrdp_iso *self, struct stream *s) return 0; } -/*****************************************************************************/ -/* returns error */ +/***************************************************************************** + * Reads an X.224 PDU (X.224 section 13) preceded by a T.123 TPKT + * header (T.123 section 8) + * + * On entry, the TPKT header length field will have been inspected and used to + * set up the input stream. + * + * On exit, the TPKT header and the fixed part of the PDU header will have been + * removed from the stream. + * + * Returns error + *****************************************************************************/ static int xrdp_iso_recv_msg(struct xrdp_iso *self, struct stream *s, int *code, int *len) { int ver; - int plen; *code = 0; *len = 0; @@ -169,7 +184,16 @@ xrdp_iso_recv_msg(struct xrdp_iso *self, struct stream *s, int *code, int *len) LLOGLN(10, ("xrdp_iso_recv_msg error logic")); } + /* TPKT header is 4 bytes, then first 2 bytes of the X.224 CR-TPDU */ + if (!s_check_rem(s, 6)) + { + return 1; + } + in_uint8(s, ver); + in_uint8s(s, 3); /* Skip reserved field, plus length */ + in_uint8(s, *len); + in_uint8(s, *code); if (ver != 3) { @@ -178,24 +202,17 @@ xrdp_iso_recv_msg(struct xrdp_iso *self, struct stream *s, int *code, int *len) return 1; } - in_uint8s(s, 1); - in_uint16_be(s, plen); - - if (plen < 4) + if (*len == 255) { + /* X.224 13.2.1 - reserved value */ + LLOGLN(10, ("xrdp_iso_recv_msg: reserved length encountered")); + LHEXDUMP(10, (s->data, 4)); return 1; } - if (!s_check_rem(s, 2)) - { - return 1; - } - - in_uint8(s, *len); - in_uint8(s, *code); - if (*code == ISO_PDU_DT) { + /* Data PDU : X.224 13.7 */ if (!s_check_rem(s, 1)) { return 1; @@ -204,6 +221,7 @@ xrdp_iso_recv_msg(struct xrdp_iso *self, struct stream *s, int *code, int *len) } else { + /* Other supported PDUs : X.224 13.x */ if (!s_check_rem(s, 5)) { return 1; @@ -302,7 +320,20 @@ xrdp_iso_send_cc(struct xrdp_iso *self) free_stream(s); return 0; } -/*****************************************************************************/ +/***************************************************************************** + * Process an X.224 connection request PDU + * + * See MS-RDPCGR v20190923 sections 2.2.1.1 and 3.3.5.3.1. + * + * From the latter, in particular:- + * - The length embedded in the TPKT header MUST be examined for + * consistency with the received data. If there is a discrepancy, the + * connection SHOULD be dropped + * - If the optional routingToken field exists it MUST be ignored. + * - If the optional cookie field is present it MUST be ignored. + * - If both the routingToken and cookie fields are present, the server + * SHOULD continue with the connection. + *****************************************************************************/ /* returns error */ int xrdp_iso_incoming(struct xrdp_iso *self) @@ -310,16 +341,14 @@ xrdp_iso_incoming(struct xrdp_iso *self) int rv = 0; int code; int len; - int cookie_index; int cc_type; - char text[256]; - char *pend; struct stream *s; + int expected_pdu_len; LLOGLN(10, (" in xrdp_iso_incoming")); s = libxrdp_force_read(self->trans); - if (s == 0) + if (s == NULL) { return 1; } @@ -330,15 +359,29 @@ xrdp_iso_incoming(struct xrdp_iso *self) return 1; } - if ((code != ISO_PDU_CR) || (len < 6)) + if (code != ISO_PDU_CR) { return 1; } + /* + * Make sure the length indicator field extracted from the X.224 + * connection request TPDU corresponds to the length in the TPKT header. + * + * We do this by seeing how the indicator field minus the counted + * octets in the TPDU header (6) compares with the space left in + * the stream. + */ + expected_pdu_len = (s->end - s->p) + 6; + if (len != expected_pdu_len) + { + LLOGLN(0, ("xrdp_iso_incoming: X.224 CR-TPDU length exp %d got %d", + expected_pdu_len, len)); + return 1; + } + /* process connection request */ - pend = s->p + (len - 6); - cookie_index = 0; - while (s->p < pend) + while (s_check_rem(s, 1)) { in_uint8(s, cc_type); switch (cc_type) @@ -355,28 +398,30 @@ xrdp_iso_incoming(struct xrdp_iso *self) break; case RDP_CORRELATION_INFO: /* rdpCorrelationInfo 6 */ // TODO + if (!s_check_rem(s, 1 + 2 + 16 + 16)) + { + LLOGLN(0, ("xrdp_iso_incoming: short correlation info")); + return 1; + } + in_uint8s(s, 1 + 2 + 16 + 16); break; - case 'C': /* Cookie routingToken */ - while (s->p < pend) + case 'C': /* Cookie */ + /* The routingToken and cookie fields are both ASCII + * strings starting with the word 'Cookie: ' and + * ending with CR+LF. We ignore both, so we do + * not need to distinguish them */ + while (s_check_rem(s, 1)) { - text[cookie_index] = cc_type; - cookie_index++; - if (cookie_index > 255) - { - cookie_index = 255; - } - if ((s->p[0] == 0x0D) && (s->p[1] == 0x0A)) - { - in_uint8s(s, 2); - text[cookie_index] = 0; - cookie_index = 0; - if (g_strlen(text) > 0) - { - } - break; - } in_uint8(s, cc_type); + if (cc_type == 0x0D && s_check_rem(s, 1)) + { + in_uint8(s, cc_type); + if (cc_type == 0x0A) + { + break; + } + } } break; }