Merge pull request #1624 from metalefty/v0.9-CVE-2020-4044

Release v0.9.13.1

NEWS.md

@@ -1,3 +1,17 @@
+# Release notes for xrdp v0.9.13.1 (2020/06/30)
+
+This is a security fix release that includes fixes for the following local buffer overflow vulnerability.
+
+* [CVE-2022-4044: Local users can perform a buffer overflow attack against the xrdp-sesman service and then impersonate it](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-4044)
+
+This update is recommended for all xrdp users.
+
+## Special thanks
+
+Thanks to [Ashley Newson](https://github.com/ashleynewson) reporting the vulnerability and reviewing fix.
+
+-----------------------
+
 # Release notes for xrdp v0.9.13 (2020/03/11)
 
 This release is an intermediate bugfix release. The previous version v0.9.12 has some regressions on drive redirection.
@@ -111,7 +125,7 @@ Thank you for matt335672 contributing to lots of improvements in drive redirecti
 
 -----------------------
 
-## Release notes for xrdp v0.9.9 (2018/12/25)
+# Release notes for xrdp v0.9.9 (2018/12/25)
 
 ## Release cycle
 From the next release, release cycle will be changed from quarterly to every

README.md

@@ -2,7 +2,7 @@
 [![Gitter](https://badges.gitter.im/Join%20Chat.svg)](https://gitter.im/neutrinolabs/xrdp-questions)
 ![Apache-License](https://img.shields.io/badge/License-Apache%202.0-blue.svg)
 
-*Current Version:* 0.9.13
+*Current Version:* 0.9.13.1
 
 # xrdp - an open source RDP server
 

configure.ac

@@ -1,7 +1,7 @@
 # Process this file with autoconf to produce a configure script
 
 AC_PREREQ(2.65)
-AC_INIT([xrdp], [0.9.13], [xrdp-devel@googlegroups.com])
+AC_INIT([xrdp], [0.9.13.1], [xrdp-devel@googlegroups.com])
 AC_CONFIG_HEADERS(config_ac.h:config_ac-h.in)
 AM_INIT_AUTOMAKE([1.7.2 foreign])
 AC_CONFIG_MACRO_DIR([m4])

sesman/libscp/libscp_types.h

@@ -59,6 +59,10 @@
 
 #include "libscp_types_mng.h"
 
+/* Max server incoming and outgoing message size, used to stop memory
+   exhaustion attempts (CVE-2020-4044) */
+#define SCP_MAX_MESSAGE_SIZE 8192
+
 struct SCP_CONNECTION
 {
   int in_sck;

sesman/libscp/libscp_v0.c

@@ -34,13 +34,72 @@
 
 extern struct log_config *s_log;
 
+/** Maximum length of a string (two bytes + len), excluding the terminator
+ *
+ * Practially this is limited by [MS-RDPBCGR] TS_INFO_PACKET
+ * */
+#define STRING16_MAX_LEN 512
+
+/**
+ * Reads a big-endian uint16 followed by a string into a buffer
+ *
+ * Buffer is null-terminated on success
+ *
+ * @param s Input stream
+ * @param [out] Output buffer (must be >= (STRING16_MAX_LEN+1) chars)
+ * @param param Parameter we're reading
+ * @param line Line number reference
+ * @return != 0 if string read OK
+ */
+static
+int in_string16(struct stream *s, char str[], const char *param, int line)
+{
+    int result;
+
+    if (!s_check_rem(s, 2))
+    {
+        log_message(LOG_LEVEL_WARNING,
+                    "[v0:%d] connection aborted: %s len missing",
+                    line, param);
+        result = 0;
+    }
+    else
+    {
+        unsigned int sz;
+
+        in_uint16_be(s, sz);
+        if (sz > STRING16_MAX_LEN)
+        {
+            log_message(LOG_LEVEL_WARNING,
+                        "[v0:%d] connection aborted: %s too long (%u chars)",
+                        line, param, sz);
+            result = 0;
+        }
+        else
+        {
+            result = s_check_rem(s, sz);
+            if (!result)
+            {
+                log_message(LOG_LEVEL_WARNING,
+                            "[v0:%d] connection aborted: %s data missing",
+                            line, param);
+            }
+            else
+            {
+                in_uint8a(s, str, sz);
+                str[sz] = '\0';
+            }
+        }
+    }
+    return result;
+}
 /* client API */
 /******************************************************************************/
 enum SCP_CLIENT_STATES_E
 scp_v0c_connect(struct SCP_CONNECTION *c, struct SCP_SESSION *s)
 {
     tui32 version;
-    tui32 size;
+    int size;
     tui16 sz;
 
     init_stream(c->in_s, c->in_s->size);
@@ -71,10 +130,24 @@ scp_v0c_connect(struct SCP_CONNECTION *c, struct SCP_SESSION *s)
     }
 
     sz = g_strlen(s->username);
+    if (sz > STRING16_MAX_LEN)
+    {
+        log_message(LOG_LEVEL_WARNING,
+                    "[v0:%d] connection aborted: username too long",
+                    __LINE__);
+        return SCP_CLIENT_STATE_SIZE_ERR;
+    }
     out_uint16_be(c->out_s, sz);
     out_uint8a(c->out_s, s->username, sz);
 
     sz = g_strlen(s->password);
+    if (sz > STRING16_MAX_LEN)
+    {
+        log_message(LOG_LEVEL_WARNING,
+                    "[v0:%d] connection aborted: password too long",
+                    __LINE__);
+        return SCP_CLIENT_STATE_SIZE_ERR;
+    }
     out_uint16_be(c->out_s, sz);
     out_uint8a(c->out_s, s->password, sz);
     out_uint16_be(c->out_s, s->width);
@@ -111,14 +184,16 @@ scp_v0c_connect(struct SCP_CONNECTION *c, struct SCP_SESSION *s)
 
     in_uint32_be(c->in_s, size);
 
-    if (size < 14)
+    if (size < (8 + 2 + 2 + 2) || size > SCP_MAX_MESSAGE_SIZE)
     {
-        log_message(LOG_LEVEL_WARNING, "[v0:%d] connection aborted: packet size error", __LINE__);
+        log_message(LOG_LEVEL_WARNING,
+                    "[v0:%d] connection aborted: msg size = %d",
+                    __LINE__, size);
         return SCP_CLIENT_STATE_SIZE_ERR;
     }
 
     /* getting payload */
-    init_stream(c->in_s, c->in_s->size);
+    init_stream(c->in_s, size - 8);
 
     if (0 != scp_tcp_force_recv(c->in_sck, c->in_s->data, size - 8))
     {
@@ -126,6 +201,8 @@ scp_v0c_connect(struct SCP_CONNECTION *c, struct SCP_SESSION *s)
         return SCP_CLIENT_STATE_NETWORK_ERR;
     }
 
+    c->in_s->end = c->in_s->data + (size - 8);
+
     /* check code */
     in_uint16_be(c->in_s, sz);
 
@@ -151,43 +228,38 @@ scp_v0c_connect(struct SCP_CONNECTION *c, struct SCP_SESSION *s)
     return SCP_CLIENT_STATE_END;
 }
 
-/* server API */
+/**
-/******************************************************************************/
+ * Initialises a V0 session object
-enum SCP_SERVER_STATES_E
+ *
-scp_v0s_accept(struct SCP_CONNECTION *c, struct SCP_SESSION **s, int skipVchk)
+ * At the time of the call, the version has been read from the connection
+ *
+ * @param c Connection
+ * @param [out] session pre-allocated session object
+ * @return SCP_SERVER_STATE_OK for success
+ */
+static enum SCP_SERVER_STATES_E
+scp_v0s_init_session(struct SCP_CONNECTION *c, struct SCP_SESSION *session)
 {
-    tui32 version = 0;
+    int size;
-    tui32 size;
+    tui16 height;
-    struct SCP_SESSION *session = 0;
+    tui16 width;
-    tui16 sz;
+    tui16 bpp;
     tui32 code = 0;
-    char *buf = 0;
+    char buf[STRING16_MAX_LEN + 1];
 
-    if (!skipVchk)
+    scp_session_set_version(session, 0);
-    {
-        LOG_DBG("[v0:%d] starting connection", __LINE__);
-
-        if (0 == scp_tcp_force_recv(c->in_sck, c->in_s->data, 8))
-        {
-            c->in_s->end = c->in_s->data + 8;
-            in_uint32_be(c->in_s, version);
-
-            if (version != 0)
-            {
-                log_message(LOG_LEVEL_WARNING, "[v0:%d] connection aborted: version error", __LINE__);
-                return SCP_SERVER_STATE_VERSION_ERR;
-            }
-        }
-        else
-        {
-            log_message(LOG_LEVEL_WARNING, "[v0:%d] connection aborted: network error", __LINE__);
-            return SCP_SERVER_STATE_NETWORK_ERR;
-        }
-    }
 
+    /* Check for a header and a code value in the length */
     in_uint32_be(c->in_s, size);
+    if (size < (8 + 2) || size > SCP_MAX_MESSAGE_SIZE)
+    {
+        log_message(LOG_LEVEL_WARNING,
+                    "[v0:%d] connection aborted: msg size = %d",
+                    __LINE__, size);
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
 
-    init_stream(c->in_s, 8196);
+    init_stream(c->in_s, size - 8);
 
     if (0 != scp_tcp_force_recv(c->in_sck, c->in_s->data, size - 8))
     {
@@ -201,16 +273,6 @@ scp_v0s_accept(struct SCP_CONNECTION *c, struct SCP_SESSION **s, int skipVchk)
 
     if (code == 0 || code == 10 || code == 20)
     {
-        session = scp_session_create();
-
-        if (0 == session)
-        {
-            log_message(LOG_LEVEL_WARNING, "[v0:%d] connection aborted: network error", __LINE__);
-            return SCP_SERVER_STATE_INTERNAL_ERR;
-        }
-
-        scp_session_set_version(session, version);
-
         if (code == 0)
         {
             scp_session_set_type(session, SCP_SESSION_TYPE_XVNC);
@@ -225,154 +287,130 @@ scp_v0s_accept(struct SCP_CONNECTION *c, struct SCP_SESSION **s, int skipVchk)
         }
 
         /* reading username */
-        in_uint16_be(c->in_s, sz);
+        if (!in_string16(c->in_s, buf, "username", __LINE__))
-        buf = g_new0(char, sz + 1);
+        {
-        in_uint8a(c->in_s, buf, sz);
+            return SCP_SERVER_STATE_SIZE_ERR;
-        buf[sz] = '\0';
+        }
         if (0 != scp_session_set_username(session, buf))
         {
-            scp_session_destroy(session);
             log_message(LOG_LEVEL_WARNING, "[v0:%d] connection aborted: error setting username", __LINE__);
-            g_free(buf);
             return SCP_SERVER_STATE_INTERNAL_ERR;
         }
-        g_free(buf);
 
         /* reading password */
-        in_uint16_be(c->in_s, sz);
+        if (!in_string16(c->in_s, buf, "passwd", __LINE__))
-        buf = g_new0(char, sz + 1);
+        {
-        in_uint8a(c->in_s, buf, sz);
+            return SCP_SERVER_STATE_SIZE_ERR;
-        buf[sz] = '\0';
+        }
         if (0 != scp_session_set_password(session, buf))
         {
-            scp_session_destroy(session);
             log_message(LOG_LEVEL_WARNING, "[v0:%d] connection aborted: error setting password", __LINE__);
-            g_free(buf);
             return SCP_SERVER_STATE_INTERNAL_ERR;
         }
-        g_free(buf);
 
-        /* width */
+        /* width  + height + bpp */
-        in_uint16_be(c->in_s, sz);
+        if (!s_check_rem(c->in_s, 2 + 2 + 2))
-        scp_session_set_width(session, sz);
+        {
-        /* height */
+            log_message(LOG_LEVEL_WARNING,
-        in_uint16_be(c->in_s, sz);
+                        "[v0:%d] connection aborted: width+height+bpp missing",
-        scp_session_set_height(session, sz);
+                        __LINE__);
-        /* bpp */
+            return SCP_SERVER_STATE_SIZE_ERR;
-        in_uint16_be(c->in_s, sz);
+        }
-        if (0 != scp_session_set_bpp(session, (tui8)sz))
+        in_uint16_be(c->in_s, width);
+        scp_session_set_width(session, width);
+        in_uint16_be(c->in_s, height);
+        scp_session_set_height(session, height);
+        in_uint16_be(c->in_s, bpp);
+        if (0 != scp_session_set_bpp(session, (tui8)bpp))
         {
-            scp_session_destroy(session);
             log_message(LOG_LEVEL_WARNING,
                         "[v0:%d] connection aborted: unsupported bpp: %d",
-                        __LINE__, (tui8)sz);
+                        __LINE__, (tui8)bpp);
             return SCP_SERVER_STATE_INTERNAL_ERR;
         }
 
         if (s_check_rem(c->in_s, 2))
         {
             /* reading domain */
-            in_uint16_be(c->in_s, sz);
+            if (!in_string16(c->in_s, buf, "domain", __LINE__))
-
+            {
-            if (sz > 0)
+                return SCP_SERVER_STATE_SIZE_ERR;
+            }
+            if (buf[0] != '\0')
             {
-                buf = g_new0(char, sz + 1);
-                in_uint8a(c->in_s, buf, sz);
-                buf[sz] = '\0';
                 scp_session_set_domain(session, buf);
-                g_free(buf);
             }
         }
 
         if (s_check_rem(c->in_s, 2))
         {
             /* reading program */
-            in_uint16_be(c->in_s, sz);
+            if (!in_string16(c->in_s, buf, "program", __LINE__))
-
+            {
-            if (sz > 0)
+                return SCP_SERVER_STATE_SIZE_ERR;
+            }
+
+            if (buf[0] != '\0')
             {
-                buf = g_new0(char, sz + 1);
-                in_uint8a(c->in_s, buf, sz);
-                buf[sz] = '\0';
                 scp_session_set_program(session, buf);
-                g_free(buf);
             }
         }
 
         if (s_check_rem(c->in_s, 2))
         {
             /* reading directory */
-            in_uint16_be(c->in_s, sz);
+            if (!in_string16(c->in_s, buf, "directory", __LINE__))
-
+            {
-            if (sz > 0)
+                return SCP_SERVER_STATE_SIZE_ERR;
+            }
+
+            if (buf[0] != '\0')
             {
-                buf = g_new0(char, sz + 1);
-                in_uint8a(c->in_s, buf, sz);
-                buf[sz] = '\0';
                 scp_session_set_directory(session, buf);
-                g_free(buf);
             }
         }
 
         if (s_check_rem(c->in_s, 2))
         {
             /* reading client IP address */
-            in_uint16_be(c->in_s, sz);
+            if (!in_string16(c->in_s, buf, "client IP", __LINE__))
-
+            {
-            if (sz > 0)
+                return SCP_SERVER_STATE_SIZE_ERR;
+            }
+            if (buf[0] != '\0')
             {
-                buf = g_new0(char, sz + 1);
-                in_uint8a(c->in_s, buf, sz);
-                buf[sz] = '\0';
                 scp_session_set_client_ip(session, buf);
-                g_free(buf);
             }
         }
     }
     else if (code == SCP_GW_AUTHENTICATION)
     {
-        /* g_writeln("Command is SCP_GW_AUTHENTICATION"); */
-        session = scp_session_create();
-
-        if (0 == session)
-        {
-            /* until syslog merge log_message(s_log, LOG_LEVEL_WARNING, "[v0:%d] connection aborted: network error",      __LINE__);*/
-            return SCP_SERVER_STATE_INTERNAL_ERR;
-        }
-
-        scp_session_set_version(session, version);
         scp_session_set_type(session, SCP_GW_AUTHENTICATION);
         /* reading username */
-        in_uint16_be(c->in_s, sz);
+        if (!in_string16(c->in_s, buf, "username", __LINE__))
-        buf = g_new0(char, sz + 1);
+        {
-        in_uint8a(c->in_s, buf, sz);
+            return SCP_SERVER_STATE_SIZE_ERR;
-        buf[sz] = '\0';
+        }
 
         /* g_writeln("Received user name: %s",buf); */
         if (0 != scp_session_set_username(session, buf))
         {
-            scp_session_destroy(session);
             /* until syslog merge log_message(s_log, LOG_LEVEL_WARNING, "[v0:%d] connection aborted: error setting        username", __LINE__);*/
-            g_free(buf);
             return SCP_SERVER_STATE_INTERNAL_ERR;
         }
-        g_free(buf);
 
         /* reading password */
-        in_uint16_be(c->in_s, sz);
+        if (!in_string16(c->in_s, buf, "passwd", __LINE__))
-        buf = g_new0(char, sz + 1);
+        {
-        in_uint8a(c->in_s, buf, sz);
+            return SCP_SERVER_STATE_SIZE_ERR;
-        buf[sz] = '\0';
+        }
 
         /* g_writeln("Received password: %s",buf); */
         if (0 != scp_session_set_password(session, buf))
         {
-            scp_session_destroy(session);
             /* until syslog merge log_message(s_log, LOG_LEVEL_WARNING, "[v0:%d] connection aborted: error setting password", __LINE__); */
-            g_free(buf);
             return SCP_SERVER_STATE_INTERNAL_ERR;
         }
-        g_free(buf);
     }
     else
     {
@@ -380,10 +418,67 @@ scp_v0s_accept(struct SCP_CONNECTION *c, struct SCP_SESSION **s, int skipVchk)
         return SCP_SERVER_STATE_SEQUENCE_ERR;
     }
 
-    (*s) = session;
     return SCP_SERVER_STATE_OK;
 }
 
+
+/* server API */
+/******************************************************************************/
+enum SCP_SERVER_STATES_E
+scp_v0s_accept(struct SCP_CONNECTION *c, struct SCP_SESSION **s, int skipVchk)
+{
+    enum SCP_SERVER_STATES_E result = SCP_SERVER_STATE_OK;
+    struct SCP_SESSION *session = NULL;
+    tui32 version = 0;
+
+    if (!skipVchk)
+    {
+        LOG_DBG("[v0:%d] starting connection", __LINE__);
+
+        if (0 == scp_tcp_force_recv(c->in_sck, c->in_s->data, 8))
+        {
+            c->in_s->end = c->in_s->data + 8;
+            in_uint32_be(c->in_s, version);
+
+            if (version != 0)
+            {
+                log_message(LOG_LEVEL_WARNING, "[v0:%d] connection aborted: version error", __LINE__);
+                result = SCP_SERVER_STATE_VERSION_ERR;
+            }
+        }
+        else
+        {
+            log_message(LOG_LEVEL_WARNING, "[v0:%d] connection aborted: network error", __LINE__);
+            result = SCP_SERVER_STATE_NETWORK_ERR;
+        }
+    }
+
+    if (result == SCP_SERVER_STATE_OK)
+    {
+        session = scp_session_create();
+        if (NULL == session)
+        {
+            log_message(LOG_LEVEL_WARNING,
+                        "[v0:%d] connection aborted: no memory",
+                        __LINE__);
+            result = SCP_SERVER_STATE_INTERNAL_ERR;
+        }
+        else
+        {
+            result = scp_v0s_init_session(c, session);
+            if (result != SCP_SERVER_STATE_OK)
+            {
+                scp_session_destroy(session);
+                session = NULL;
+            }
+        }
+    }
+
+    (*s) = session;
+
+    return result;
+}
+
 /******************************************************************************/
 enum SCP_SERVER_STATES_E
 scp_v0s_allow_connection(struct SCP_CONNECTION *c, SCP_DISPLAY d, const tui8 *guid)

sesman/libscp/libscp_v1s.c

@@ -35,16 +35,194 @@
 
 //extern struct log_config* s_log;
 
+/**
+ * Reads a uint8 followed by a string into a buffer
+ *
+ * Buffer is null-terminated on success
+ *
+ * @param s Input stream
+ * @param [out] Output buffer (must be >= 256 chars)
+ * @param param Parameter we're reading
+ * @param line Line number reference
+ * @return != 0 if string read OK
+ *
+ * @todo
+ *     This needs to be merged with the func of the same name in
+ *     libscp_v1s_mng.c
+ */
+static
+int in_string8(struct stream *s, char str[], const char *param, int line)
+{
+    int result;
+
+    if (!s_check_rem(s, 1))
+    {
+        log_message(LOG_LEVEL_WARNING,
+                    "[v1s:%d] connection aborted: %s len missing",
+                    line, param);
+        result = 0;
+    }
+    else
+    {
+        unsigned int sz;
+
+        in_uint8(s, sz);
+        result = s_check_rem(s, sz);
+        if (!result)
+        {
+            log_message(LOG_LEVEL_WARNING,
+                        "[v1s:%d] connection aborted: %s data missing",
+                        line, param);
+        }
+        else
+        {
+            in_uint8a(s, str, sz);
+            str[sz] = '\0';
+        }
+    }
+    return result;
+}
+/* server API */
+
+/**
+ * Initialises a V1 session object
+ *
+ * This is called after the V1 header, command set and command have been read
+ *
+ * @param c Connection
+ * @param [out] session pre-allocated session object
+ * @return SCP_SERVER_STATE_OK for success
+ */
+static enum SCP_SERVER_STATES_E
+scp_v1s_init_session(struct SCP_CONNECTION *c, struct SCP_SESSION *session)
+{
+    tui8 type;
+    tui16 height;
+    tui16 width;
+    tui8 bpp;
+    tui8 sz;
+    char buf[256];
+
+    scp_session_set_version(session, 1);
+
+    /* Check there's data for the session type, the height, the width, the
+     * bpp, the resource sharing indicator and the locale */
+    if (!s_check_rem(c->in_s, 1 + 2 + 2 + 1 + 1 + 17))
+    {
+        log_message(LOG_LEVEL_WARNING,
+                    "[v1s:%d] connection aborted: short packet",
+                    __LINE__);
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
+
+    in_uint8(c->in_s, type);
+
+    if ((type != SCP_SESSION_TYPE_XVNC) && (type != SCP_SESSION_TYPE_XRDP))
+    {
+        log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: unknown session type", __LINE__);
+        return SCP_SERVER_STATE_SESSION_TYPE_ERR;
+    }
+
+    scp_session_set_type(session, type);
+
+    in_uint16_be(c->in_s, height);
+    scp_session_set_height(session, height);
+    in_uint16_be(c->in_s, width);
+    scp_session_set_width(session, width);
+    in_uint8(c->in_s, bpp);
+    if (0 != scp_session_set_bpp(session, bpp))
+    {
+        log_message(LOG_LEVEL_WARNING,
+                    "[v1s:%d] connection aborted: unsupported bpp: %d",
+                    __LINE__, bpp);
+        return SCP_SERVER_STATE_INTERNAL_ERR;
+    }
+    in_uint8(c->in_s, sz);
+    scp_session_set_rsr(session, sz);
+    in_uint8a(c->in_s, buf, 17);
+    buf[17] = '\0';
+    scp_session_set_locale(session, buf);
+
+    /* Check there's enough data left for at least an IPv4 address (+len) */
+    if (!s_check_rem(c->in_s, 1 + 4))
+    {
+        log_message(LOG_LEVEL_WARNING,
+                    "[v1s:%d] connection aborted: IP addr len missing",
+                    __LINE__);
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
+
+    in_uint8(c->in_s, sz);
+
+    if (sz == SCP_ADDRESS_TYPE_IPV4)
+    {
+        tui32 ipv4;
+        in_uint32_be(c->in_s, ipv4);
+        scp_session_set_addr(session, sz, &ipv4);
+    }
+    else if (sz == SCP_ADDRESS_TYPE_IPV6)
+    {
+        if (!s_check_rem(c->in_s, 16))
+        {
+            log_message(LOG_LEVEL_WARNING,
+                        "[v1s:%d] connection aborted: IP addr missing",
+                        __LINE__);
+            return SCP_SERVER_STATE_SIZE_ERR;
+        }
+        in_uint8a(c->in_s, buf, 16);
+        scp_session_set_addr(session, sz, buf);
+    }
+
+    /* reading hostname */
+    if (!in_string8(c->in_s, buf, "hostname", __LINE__))
+    {
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
+
+    if (0 != scp_session_set_hostname(session, buf))
+    {
+        log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: internal error", __LINE__);
+        return SCP_SERVER_STATE_INTERNAL_ERR;
+    }
+
+    /* reading username */
+    if (!in_string8(c->in_s, buf, "username", __LINE__))
+    {
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
+
+    if (0 != scp_session_set_username(session, buf))
+    {
+        log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: internal error", __LINE__);
+        return SCP_SERVER_STATE_INTERNAL_ERR;
+    }
+
+    /* reading password */
+    if (!in_string8(c->in_s, buf, "passwd", __LINE__))
+    {
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
+
+    if (0 != scp_session_set_password(session, buf))
+    {
+        log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: internal error", __LINE__);
+        return SCP_SERVER_STATE_INTERNAL_ERR;
+    }
+
+    return SCP_SERVER_STATE_OK;
+}
+
 /* server API */
 enum SCP_SERVER_STATES_E scp_v1s_accept(struct SCP_CONNECTION *c, struct SCP_SESSION **s, int skipVchk)
 {
+    enum SCP_SERVER_STATES_E result;
     struct SCP_SESSION *session;
     tui32 version;
-    tui32 size;
+    int size;
     tui16 cmdset;
     tui16 cmd;
-    tui8 sz;
+
-    char buf[257];
+    (*s) = NULL;
 
     if (!skipVchk)
     {
@@ -68,13 +246,15 @@ enum SCP_SERVER_STATES_E scp_v1s_accept(struct SCP_CONNECTION *c, struct SCP_SES
 
     in_uint32_be(c->in_s, size);
 
-    if (size < 12)
+    /* Check the message is big enough for the header, the command set, and
+     * the command (but not too big) */
+    if (size < (8 + 2 + 2) || size > SCP_MAX_MESSAGE_SIZE)
     {
         log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: size error", __LINE__);
         return SCP_SERVER_STATE_SIZE_ERR;
     }
 
-    init_stream(c->in_s, c->in_s->size);
+    init_stream(c->in_s, size - 8);
 
     if (0 != scp_tcp_force_recv(c->in_sck, c->in_s->data, (size - 8)))
     {
@@ -82,6 +262,8 @@ enum SCP_SERVER_STATES_E scp_v1s_accept(struct SCP_CONNECTION *c, struct SCP_SES
         return SCP_SERVER_STATE_NETWORK_ERR;
     }
 
+    c->in_s->end = c->in_s->data + (size - 8);
+
     /* reading command set */
     in_uint16_be(c->in_s, cmdset);
 
@@ -111,98 +293,27 @@ enum SCP_SERVER_STATES_E scp_v1s_accept(struct SCP_CONNECTION *c, struct SCP_SES
 
     session = scp_session_create();
 
-    if (0 == session)
+    if (NULL == session)
     {
-        log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: internal error (malloc returned NULL)", __LINE__);
-        return SCP_SERVER_STATE_INTERNAL_ERR;
-    }
-
-    scp_session_set_version(session, 1);
-
-    in_uint8(c->in_s, sz);
-
-    if ((sz != SCP_SESSION_TYPE_XVNC) && (sz != SCP_SESSION_TYPE_XRDP))
-    {
-        scp_session_destroy(session);
-        log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: unknown session type", __LINE__);
-        return SCP_SERVER_STATE_SESSION_TYPE_ERR;
-    }
-
-    scp_session_set_type(session, sz);
-
-    in_uint16_be(c->in_s, cmd);
-    scp_session_set_height(session, cmd);
-    in_uint16_be(c->in_s, cmd);
-    scp_session_set_width(session, cmd);
-    in_uint8(c->in_s, sz);
-    if (0 != scp_session_set_bpp(session, sz))
-    {
-        scp_session_destroy(session);
         log_message(LOG_LEVEL_WARNING,
-                    "[v1s:%d] connection aborted: unsupported bpp: %d",
+                    "[v1s:%d] connection aborted: internal error "
-                    __LINE__, sz);
+                    "(malloc returned NULL)", __LINE__);
-        return SCP_SERVER_STATE_INTERNAL_ERR;
+        result = SCP_SERVER_STATE_INTERNAL_ERR;
     }
-    in_uint8(c->in_s, sz);
+    else
-    scp_session_set_rsr(session, sz);
-    in_uint8a(c->in_s, buf, 17);
-    buf[17] = '\0';
-    scp_session_set_locale(session, buf);
-
-    in_uint8(c->in_s, sz);
-
-    if (sz == SCP_ADDRESS_TYPE_IPV4)
     {
-        in_uint32_be(c->in_s, size);
+        result = scp_v1s_init_session(c, session);
-        scp_session_set_addr(session, sz, &size);
+        if (result != SCP_SERVER_STATE_OK)
-    }
-    else if (sz == SCP_ADDRESS_TYPE_IPV6)
-    {
-        in_uint8a(c->in_s, buf, 16);
-        scp_session_set_addr(session, sz, buf);
-    }
-
-    buf[256] = '\0';
-    /* reading hostname */
-    in_uint8(c->in_s, sz);
-    buf[sz] = '\0';
-    in_uint8a(c->in_s, buf, sz);
-
-    if (0 != scp_session_set_hostname(session, buf))
         {
             scp_session_destroy(session);
-        log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: internal error", __LINE__);
+            session = NULL;
-        return SCP_SERVER_STATE_INTERNAL_ERR;
         }
-
-    /* reading username */
-    in_uint8(c->in_s, sz);
-    buf[sz] = '\0';
-    in_uint8a(c->in_s, buf, sz);
-
-    if (0 != scp_session_set_username(session, buf))
-    {
-        scp_session_destroy(session);
-        log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: internal error", __LINE__);
-        return SCP_SERVER_STATE_INTERNAL_ERR;
-    }
-
-    /* reading password */
-    in_uint8(c->in_s, sz);
-    buf[sz] = '\0';
-    in_uint8a(c->in_s, buf, sz);
-
-    if (0 != scp_session_set_password(session, buf))
-    {
-        scp_session_destroy(session);
-        log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: internal error", __LINE__);
-        return SCP_SERVER_STATE_INTERNAL_ERR;
     }
 
     /* returning the struct */
     (*s) = session;
 
-    return SCP_SERVER_STATE_OK;
+    return result;
 }
 
 enum SCP_SERVER_STATES_E
@@ -242,13 +353,12 @@ enum SCP_SERVER_STATES_E
 scp_v1s_request_password(struct SCP_CONNECTION *c, struct SCP_SESSION *s,
                          const char *reason)
 {
-    tui8 sz;
     tui32 version;
-    tui32 size;
+    int size;
     tui16 cmdset;
     tui16 cmd;
     int rlen;
-    char buf[257];
+    char buf[256];
 
     init_stream(c->in_s, c->in_s->size);
     init_stream(c->out_s, c->out_s->size);
@@ -296,13 +406,17 @@ scp_v1s_request_password(struct SCP_CONNECTION *c, struct SCP_SESSION *s,
 
     in_uint32_be(c->in_s, size);
 
-    if (size < 12)
+    /* Check the message is big enough for the header, the command set, and
+     * the command (but not too big) */
+    if (size < (8 + 2 + 2) || size > SCP_MAX_MESSAGE_SIZE)
     {
-        log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: size error", __LINE__);
+        log_message(LOG_LEVEL_WARNING,
+                    "[v1s:%d] connection aborted: size error",
+                    __LINE__);
         return SCP_SERVER_STATE_SIZE_ERR;
     }
 
-    init_stream(c->in_s, c->in_s->size);
+    init_stream(c->in_s, size - 8);
 
     if (0 != scp_tcp_force_recv(c->in_sck, c->in_s->data, (size - 8)))
     {
@@ -310,6 +424,8 @@ scp_v1s_request_password(struct SCP_CONNECTION *c, struct SCP_SESSION *s,
         return SCP_SERVER_STATE_NETWORK_ERR;
     }
 
+    c->in_s->end = c->in_s->data + (size - 8);
+
     in_uint16_be(c->in_s, cmdset);
 
     if (cmdset != SCP_COMMAND_SET_DEFAULT)
@@ -326,11 +442,11 @@ scp_v1s_request_password(struct SCP_CONNECTION *c, struct SCP_SESSION *s,
         return SCP_SERVER_STATE_SEQUENCE_ERR;
     }
 
-    buf[256] = '\0';
     /* reading username */
-    in_uint8(c->in_s, sz);
+    if (!in_string8(c->in_s, buf, "username", __LINE__))
-    buf[sz] = '\0';
+    {
-    in_uint8a(c->in_s, buf, sz);
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
 
     if (0 != scp_session_set_username(s, buf))
     {
@@ -339,9 +455,10 @@ scp_v1s_request_password(struct SCP_CONNECTION *c, struct SCP_SESSION *s,
     }
 
     /* reading password */
-    in_uint8(c->in_s, sz);
+    if (!in_string8(c->in_s, buf, "passwd", __LINE__))
-    buf[sz] = '\0';
+    {
-    in_uint8a(c->in_s, buf, sz);
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
 
     if (0 != scp_session_set_password(s, buf))
     {
@@ -422,7 +539,7 @@ enum SCP_SERVER_STATES_E
 scp_v1s_list_sessions(struct SCP_CONNECTION *c, int sescnt, struct SCP_DISCONNECTED_SESSION *ds, SCP_SID *sid)
 {
     tui32 version = 1;
-    tui32 size = 12;
+    int size = 12;
     tui16 cmd = 40;
     int pktcnt;
     int idx;
@@ -468,13 +585,15 @@ scp_v1s_list_sessions(struct SCP_CONNECTION *c, int sescnt, struct SCP_DISCONNEC
 
     in_uint32_be(c->in_s, size);
 
-    if (size < 12)
+    /* Check the message is big enough for the header, the command set, and
+     * the command (but not too big) */
+    if (size < (8 + 2 + 2) || size > SCP_MAX_MESSAGE_SIZE)
     {
         log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: size error", __LINE__);
         return SCP_SERVER_STATE_SIZE_ERR;
     }
 
-    init_stream(c->in_s, c->in_s->size);
+    init_stream(c->in_s, size - 8);
 
     if (0 != scp_tcp_force_recv(c->in_sck, c->in_s->data, (size - 8)))
     {
@@ -482,6 +601,8 @@ scp_v1s_list_sessions(struct SCP_CONNECTION *c, int sescnt, struct SCP_DISCONNEC
         return SCP_SERVER_STATE_NETWORK_ERR;
     }
 
+    c->in_s->end = c->in_s->data + (size - 8);
+
     in_uint16_be(c->in_s, cmd);
 
     if (cmd != SCP_COMMAND_SET_DEFAULT)
@@ -606,14 +727,16 @@ scp_v1s_list_sessions(struct SCP_CONNECTION *c, int sescnt, struct SCP_DISCONNEC
 
     in_uint32_be(c->in_s, size);
 
-    if (size < 12)
+    /* Check the message is big enough for the header, the command set, and
+     * the command (but not too big) */
+    if (size < (8 + 2 + 2) || size > SCP_MAX_MESSAGE_SIZE)
     {
         log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: size error", __LINE__);
         return SCP_SERVER_STATE_SIZE_ERR;
     }
 
     /* rest of the packet */
-    init_stream(c->in_s, c->in_s->size);
+    init_stream(c->in_s, size - 8);
 
     if (0 != scp_tcp_force_recv(c->in_sck, c->in_s->data, (size - 8)))
     {
@@ -621,6 +744,8 @@ scp_v1s_list_sessions(struct SCP_CONNECTION *c, int sescnt, struct SCP_DISCONNEC
         return SCP_SERVER_STATE_NETWORK_ERR;
     }
 
+    c->in_s->end = c->in_s->data + (size - 8);
+
     in_uint16_be(c->in_s, cmd);
 
     if (cmd != SCP_COMMAND_SET_DEFAULT)
@@ -633,6 +758,11 @@ scp_v1s_list_sessions(struct SCP_CONNECTION *c, int sescnt, struct SCP_DISCONNEC
 
     if (cmd == 43)
     {
+        if (!s_check_rem(c->in_s, 4))
+        {
+            log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: missing session", __LINE__);
+            return SCP_SERVER_STATE_SIZE_ERR;
+        }
         /* select session */
         in_uint32_be(c->in_s, (*sid));
 

sesman/libscp/libscp_v1s_mng.c

@@ -38,17 +38,79 @@
 static enum SCP_SERVER_STATES_E
 _scp_v1s_mng_check_response(struct SCP_CONNECTION *c, struct SCP_SESSION *s);
 
-/* server API */
+/**
-enum SCP_SERVER_STATES_E
+ * Reads a uint8 followed by a string into a buffer
-scp_v1s_mng_accept(struct SCP_CONNECTION *c, struct SCP_SESSION **s)
+ *
+ * Buffer is null-terminated on success
+ *
+ * @param s Input stream
+ * @param [out] Output buffer (must be >= 256 chars)
+ * @param param Parameter we're reading
+ * @param line Line number reference
+ * @return != 0 if string read OK
+ *
+ * @todo
+ *     This needs to be merged with the func of the same name in
+ *     libscp_v1s.c
+ */
+static
+int in_string8(struct stream *s, char str[], const char *param, int line)
+{
+    int result;
+
+    if (!s_check_rem(s, 1))
+    {
+        log_message(LOG_LEVEL_WARNING,
+                    "[v1s_mng:%d] connection aborted: %s len missing",
+                    line, param);
+        result = 0;
+    }
+    else
+    {
+        unsigned int sz;
+
+        in_uint8(s, sz);
+        result = s_check_rem(s, sz);
+        if (!result)
+        {
+            log_message(LOG_LEVEL_WARNING,
+                        "[v1s_mng:%d] connection aborted: %s data missing",
+                        line, param);
+        }
+        else
+        {
+            in_uint8a(s, str, sz);
+            str[sz] = '\0';
+        }
+    }
+    return result;
+}
+/**
+ * Initialises a V1 management session object
+ *
+ * At call time, the command set value has been read from the wire, and
+ * the command still needs to be processed.
+ *
+ * @param c Connection
+ * @param [out] session pre-allocated session object
+ * @return SCP_SERVER_STATE_START_MANAGE for success
+ */
+static enum SCP_SERVER_STATES_E
+scp_v1s_mng_init_session(struct SCP_CONNECTION *c, struct SCP_SESSION *session)
 {
-    struct SCP_SESSION *session;
     tui32 ipaddr;
     tui16 cmd;
     tui8 sz;
-    char buf[257];
+    char buf[256];
+
+    scp_session_set_version(session, 1);
 
     /* reading command */
+    if (!s_check_rem(c->in_s, 2))
+    {
+        /* Caller should have checked this */
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
     in_uint16_be(c->in_s, cmd);
 
     if (cmd != 1) /* manager login */
@@ -56,41 +118,39 @@ scp_v1s_mng_accept(struct SCP_CONNECTION *c, struct SCP_SESSION **s)
         return SCP_SERVER_STATE_SEQUENCE_ERR;
     }
 
-    session = scp_session_create();
-
-    if (0 == session)
-    {
-        return SCP_SERVER_STATE_INTERNAL_ERR;
-    }
-
-    scp_session_set_version(session, 1);
-    scp_session_set_type(session, SCP_SESSION_TYPE_MANAGE);
-
     /* reading username */
-    in_uint8(c->in_s, sz);
+    if (!in_string8(c->in_s, buf, "username", __LINE__))
-    buf[sz] = '\0';
+    {
-    in_uint8a(c->in_s, buf, sz);
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
 
     if (0 != scp_session_set_username(session, buf))
     {
-        scp_session_destroy(session);
         return SCP_SERVER_STATE_INTERNAL_ERR;
     }
 
     /* reading password */
-    in_uint8(c->in_s, sz);
+    if (!in_string8(c->in_s, buf, "passwd", __LINE__))
-    buf[sz] = '\0';
+    {
-    in_uint8a(c->in_s, buf, sz);
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
 
     if (0 != scp_session_set_password(session, buf))
     {
-        scp_session_destroy(session);
         return SCP_SERVER_STATE_INTERNAL_ERR;
     }
 
-    /* reading remote address */
+    /* reading remote address
-    in_uint8(c->in_s, sz);
+     * Check there's enough data left for at least an IPv4 address (+len) */
+    if (!s_check_rem(c->in_s, 1 + 4))
+    {
+        log_message(LOG_LEVEL_WARNING,
+                    "[v1s_mng:%d] connection aborted: IP addr len missing",
+                    __LINE__);
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
 
+    in_uint8(c->in_s, sz);
     if (sz == SCP_ADDRESS_TYPE_IPV4)
     {
         in_uint32_be(c->in_s, ipaddr);
@@ -98,25 +158,57 @@ scp_v1s_mng_accept(struct SCP_CONNECTION *c, struct SCP_SESSION **s)
     }
     else if (sz == SCP_ADDRESS_TYPE_IPV6)
     {
+        if (!s_check_rem(c->in_s, 16))
+        {
+            log_message(LOG_LEVEL_WARNING,
+                        "[v1s_mng:%d] connection aborted: IP addr missing",
+                        __LINE__);
+            return SCP_SERVER_STATE_SIZE_ERR;
+        }
         in_uint8a(c->in_s, buf, 16);
         scp_session_set_addr(session, sz, buf);
     }
 
     /* reading hostname */
-    in_uint8(c->in_s, sz);
+    if (!in_string8(c->in_s, buf, "hostname", __LINE__))
-    buf[sz] = '\0';
+    {
-    in_uint8a(c->in_s, buf, sz);
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
 
     if (0 != scp_session_set_hostname(session, buf))
     {
-        scp_session_destroy(session);
         return SCP_SERVER_STATE_INTERNAL_ERR;
     }
 
-    /* returning the struct */
+    return SCP_SERVER_STATE_START_MANAGE;
+}
+
+enum SCP_SERVER_STATES_E
+scp_v1s_mng_accept(struct SCP_CONNECTION *c, struct SCP_SESSION **s)
+{
+    enum SCP_SERVER_STATES_E result;
+    struct SCP_SESSION *session;
+
+    session = scp_session_create();
+    if (NULL == session)
+    {
+        result = SCP_SERVER_STATE_INTERNAL_ERR;
+    }
+    else
+    {
+        scp_session_set_type(session, SCP_SESSION_TYPE_MANAGE);
+
+        result = scp_v1s_mng_init_session(c, session);
+        if (result != SCP_SERVER_STATE_START_MANAGE)
+        {
+            scp_session_destroy(session);
+            session = NULL;
+        }
+    }
+
     (*s) = session;
 
-    return SCP_SERVER_STATE_START_MANAGE;
+    return result;
 }
 
 /* 002 */
@@ -289,7 +381,7 @@ static enum SCP_SERVER_STATES_E
 _scp_v1s_mng_check_response(struct SCP_CONNECTION *c, struct SCP_SESSION *s)
 {
     tui32 version;
-    tui32 size;
+    int size;
     tui16 cmd;
     //   tui8 dim;
     //   char buf[257];
@@ -312,7 +404,15 @@ _scp_v1s_mng_check_response(struct SCP_CONNECTION *c, struct SCP_SESSION *s)
 
     in_uint32_be(c->in_s, size);
 
-    init_stream(c->in_s, c->in_s->size);
+    /* Check the message is big enough for the header, the command set, and
+     * the command (but not too big) */
+    if (size < (8 + 2 + 2) || size > SCP_MAX_MESSAGE_SIZE)
+    {
+        log_message(LOG_LEVEL_WARNING, "[v1s_mng:%d] connection aborted: size error", __LINE__);
+        return SCP_SERVER_STATE_SIZE_ERR;
+    }
+
+    init_stream(c->in_s, size - 8);
 
     /* read the rest of the packet */
     if (0 != scp_tcp_force_recv(c->in_sck, c->in_s->data, size - 8))
@@ -321,6 +421,8 @@ _scp_v1s_mng_check_response(struct SCP_CONNECTION *c, struct SCP_SESSION *s)
         return SCP_SERVER_STATE_NETWORK_ERR;
     }
 
+    c->in_s->end = c->in_s->data + (size - 8);
+
     in_uint16_be(c->in_s, cmd);
 
     if (cmd != SCP_COMMAND_SET_MANAGE)

sesman/scp.c

@@ -48,8 +48,8 @@ scp_process_start(void *sck)
     make_stream(scon.in_s);
     make_stream(scon.out_s);
 
-    init_stream(scon.in_s, 8192);
+    init_stream(scon.in_s, SCP_MAX_MESSAGE_SIZE);
-    init_stream(scon.out_s, 8192);
+    init_stream(scon.out_s, SCP_MAX_MESSAGE_SIZE);
 
     switch (scp_vXs_accept(&scon, &(sdata)))
     {
@@ -76,10 +76,12 @@ scp_process_start(void *sck)
             scp_v1_mng_process(&scon, sdata);
             break;
         case SCP_SERVER_STATE_VERSION_ERR:
-            /* an unknown scp version was requested, so we shut down the */
+        case SCP_SERVER_STATE_SIZE_ERR:
-            /* connection (and log the fact)                             */
+            /* an unknown scp version was requested, or the message sizes
+               are inconsistent. Shut down the connection and log the
+               fact */
             log_message(LOG_LEVEL_WARNING,
-                        "unknown protocol version specified. connection refused.");
+                        "protocol violation. connection refused.");
             break;
         case SCP_SERVER_STATE_NETWORK_ERR:
             log_message(LOG_LEVEL_WARNING, "libscp network error.");