Security improvements

1) In FIPS mode, Classic RDP security is not allowed at all.
2) In FIPS mode xrdp-keygen creates an empty file
3) Documentation wording improved around the security_level setting
4) Logging improved around the security negotiation
5) Warnings now generated if Classic RDP security is negotiated

Author: matt335672

common/xrdp_client_info.h

@@ -67,6 +67,12 @@ struct display_size_description
     unsigned int session_height;
 };
 
+/* Values used for the security_layer */
+/* TODO: Make this an enum, and move it below xrdp_client_info */
+#define SECURITY_LAYER_NEGOTIATE 0
+#define SECURITY_LAYER_RDP 1
+#define SECURITY_LAYER_TLS 2
+
 enum client_resize_mode
 {
     CRMODE_NONE,
@@ -174,7 +180,7 @@ struct xrdp_client_info
     int use_fast_path;
     int require_credentials; /* when true, credentials *must* be passed on cmd line */
 
-    int security_layer; /* 0 = rdp, 1 = tls , 2 = hybrid */
+    int security_layer; /* SECURITY_LAYER_* */
     int multimon; /* 0 = deny , 1 = allow */
     struct display_size_description display_sizes;
 

docs/man/xrdp-keygen.8.in

@@ -31,6 +31,10 @@ If \fBauto\fP is used as \fIoutfile\fP, the default file \fI@sysconfdir@/@syscon
 .B test
 Generate a test key pair and print information to standard output.
 
+.SH NOTES
+On machines with FIPS enabled, this program will generate an empty file,
+and a warning. On these machines, xrdp cannot use Classic RDP encryption.
+
 .SH FILES
 .TP
 .I @sysconfdir@/@sysconfsubdir@/rsakeys.ini

docs/man/xrdp.ini.5.in

@@ -62,10 +62,10 @@ See section \fBCHANNELS\fP below for more fine grained options.
 .TP
 \fBcrypt_level\fP=\fI[low|medium|high|fips]\fP
 .\" <http://blogs.msdn.com/b/openspecification/archive/2011/12/08/encryption-negotiation-in-rdp-connection.aspx>
-Regulate encryption level of Standard RDP Security.
+Regulate encryption level of Classic RDP Security.
 This parameter is effective only if \fBsecurity_layer\fP is set to \fBrdp\fP or \fBnegotiate\fP.
 
-Encryption in Standard RDP Security is controlled by two settings: \fIEncryption Level\fP
+Encryption in Classic RDP Security is controlled by two settings: \fIEncryption Level\fP
 and \fIEncryption Method\fP.  The only supported \fIEncryption Method\fP are \fB40BIT_ENCRYPTION\fP
 and \fB128BIT_ENCRYPTION\fP. \fB56BIT_ENCRYPTION\fP is not supported.
 This option controls the \fIEncryption Level\fP:
@@ -86,7 +86,8 @@ the server's maximum key strength (sever compatible).
 .TP
 .B fips
 All data sent between the client and server is protected using Federal Information
-Processing Standard 140-1 validated encryption methods.
+Processing Standard 140-1 validated encryption methods. Note that FIPS 140-1 is
+no longer considered secure.
 .I This level is required for Windows clients (mstsc.exe) if the client's group policy
 .I enforces FIPS-compliance mode.
 .RE
@@ -174,8 +175,9 @@ verification, and server authentication) are implemented by TLS.
 
 .TP
 .B rdp
-Standard RDP Security, which is not safe from man-in-the-middle attack, is used. The encryption level
-of Standard RDP Security is controlled by \fBcrypt_level\fP.
+Classic RDP Security is used. The encryption level
+of Classic RDP Security is controlled by \fBcrypt_level\fP.
+Use this setting for testing only.
 
 .TP
 .B negotiate

keygen/keygen.c

@@ -371,6 +371,13 @@ save_all(const char *e_data, int e_len, const char *n_data, int n_len,
 
     if (fd != -1)
     {
+        if (e_data == NULL)
+        {
+            /* FIPS mode */
+            g_file_close(fd);
+            return 0;
+        }
+
         if (g_file_write(fd, "[keys]\n", 7) == -1)
         {
             g_writeln("problem writing to %s, maybe no rights", filename);
@@ -397,48 +404,49 @@ save_all(const char *e_data, int e_len, const char *n_data, int n_len,
 static int
 key_gen(const char *path_and_file_name)
 {
-    char *e_data;
-    char *n_data;
-    char *d_data;
-    char *sign_data;
-    int e_len;
-    int n_len;
-    int d_len;
-    int sign_len;
-    int error;
-
-    e_data = (char *)g_exponent;
-    n_data = (char *)g_malloc(256, 0);
-    d_data = (char *)g_malloc(256, 0);
-    sign_data = (char *)g_malloc(64, 0);
-    e_len = 4;
-    n_len = g_key_size_bits / 8;
-    d_len = n_len;
-    sign_len = 64;
-    error = 0;
-    g_writeln("%s", "");
-    g_writeln("Generating %d bit rsa key...", g_key_size_bits);
-    g_writeln("%s", "");
-
-    if (error == 0)
+    char *e_data = NULL;
+    char n_data[256] = {0};
+    char d_data[256] = {0};
+    char sign_data[64] = {0};
+    int e_len = 4;
+    int n_len = g_key_size_bits / 8;
+    int d_len = n_len;
+    int sign_len = sizeof(sign_data);
+    int error = 0;
+
+    if (g_fips_mode_enabled())
     {
-        error = ssl_gen_key_xrdp1(g_key_size_bits, e_data, e_len, n_data, n_len,
-                                  d_data, d_len);
-        if (error != 0)
-        {
-            g_writeln("error %d in key_gen, ssl_gen_key_xrdp1", error);
-        }
+        g_writeln("%s", "");
+        g_writeln("This machine is running in FIPS mode - keys will not be generated");
+        g_writeln("%s", "");
     }
-
-    if (error == 0)
+    else
     {
-        g_writeln("ssl_gen_key_xrdp1 ok");
+        e_data = (char *)g_exponent;
+        g_writeln("%s", "");
+        g_writeln("Generating %d bit rsa key...", g_key_size_bits);
         g_writeln("%s", "");
-        error = sign_key(e_data, e_len, n_data, n_len, sign_data, sign_len);
 
-        if (error != 0)
+        if (error == 0)
         {
-            g_writeln("error %d in key_gen, sign_key", error);
+            error = ssl_gen_key_xrdp1(g_key_size_bits, e_data, e_len, n_data, n_len,
+                                      d_data, d_len);
+            if (error != 0)
+            {
+                g_writeln("error %d in key_gen, ssl_gen_key_xrdp1", error);
+            }
+        }
+
+        if (error == 0)
+        {
+            g_writeln("ssl_gen_key_xrdp1 ok");
+            g_writeln("%s", "");
+            error = sign_key(e_data, e_len, n_data, n_len, sign_data, sign_len);
+
+            if (error != 0)
+            {
+                g_writeln("error %d in key_gen, sign_key", error);
+            }
         }
     }
 
@@ -453,9 +461,6 @@ key_gen(const char *path_and_file_name)
         }
     }
 
-    g_free(n_data);
-    g_free(d_data);
-    g_free(sign_data);
     return error;
 }
 

libxrdp/xrdp_iso.c

@@ -60,20 +60,7 @@ protocol_mask_to_str(int protocol, char *buff, int bufflen)
         BITMASK_STRING_END_OF_LIST
     };
 
-    int rlen = g_bitmask_to_str(protocol, bits, delim, buff, bufflen);
-
-    /* Append "RDP" */
-    if (rlen == 0)
-    {
-        /* String is empty */
-        rlen = g_snprintf(buff, bufflen, "RDP");
-    }
-    else if (rlen > 0 && rlen < bufflen)
-    {
-        rlen += g_snprintf(buff + rlen, bufflen - rlen, "%cRDP", delim);
-    }
-
-    return rlen;
+    return g_bitmask_to_str(protocol, bits, delim, buff, bufflen);
 }
 
 /*****************************************************************************/
@@ -105,96 +92,98 @@ xrdp_iso_delete(struct xrdp_iso *self)
 static int
 xrdp_iso_negotiate_security(struct xrdp_iso *self)
 {
-    char requested_str[64];
-    const char *selected_str = "";
-    const char *configured_str = "";
-
     int rv = 0;
     struct xrdp_client_info *client_info = &(self->mcs_layer->sec_layer->rdp_layer->client_info);
+    char protostr[64];
+    int got_protocol = 0;
+    int security_type_mask;
 
-    /* Can we do TLS/SSL? (basic check) */
-    int ssl_capable = g_file_readable(client_info->certificate) &&
-                      g_file_readable(client_info->key_file);
-
-    /* Work out what's actually configured in xrdp.ini. The
-     * selection happens later, but we can do some error checking here */
-    switch (client_info->security_layer)
+    /* Map the configuration from xrdp.ini to a mask of allowed
+     * security types ([MS-RDPBCGR] 2.2.1.2.1)
+     *
+     * There's some oddness around PROTOCOL_RDP. This value is 0,
+     * for compatibility reasons, and it's OK for the server to
+     * suggest RDP as the fallback protocol if nothing else is
+     * agreed on. Nowadays, classic RDP security should
+     * not be used, if at all avoidable */
+
+    /* At present we only support SSL and RDP security */
+    if (client_info->security_layer == SECURITY_LAYER_RDP)
+    {
+        security_type_mask = PROTOCOL_RDP;
+    }
+    else
     {
-        case PROTOCOL_RDP:
-            configured_str = "RDP";
-            break;
+        security_type_mask = PROTOCOL_SSL;
+    }
 
-        case PROTOCOL_SSL:
-            /* We *must* use TLS. Check we can offer it, and it's requested */
-            if (ssl_capable)
-            {
-                configured_str = "SSL";
-                if ((self->requestedProtocol & PROTOCOL_SSL) == 0)
-                {
-                    LOG(LOG_LEVEL_ERROR, "Server requires TLS for security, "
-                        "but the client did not request TLS.");
-                    self->failureCode = SSL_REQUIRED_BY_SERVER;
-                    rv = 1; /* error */
-                }
-            }
-            else
+    /* Logically 'and' this value with the mask requested by the client, and
+     * see what's left */
+    protocol_mask_to_str(self->requestedProtocol, protostr, sizeof(protostr));
+    LOG(LOG_LEVEL_INFO, "Client requested security types (RDP assumed) : %s",
+        protostr);
+    security_type_mask &= self->requestedProtocol;
+
+    /* Is there a match on SSL/TLS? */
+    if ((security_type_mask & PROTOCOL_SSL) != 0)
+    {
+        /* Can we do TLS? (basic check) */
+        if (g_file_readable(client_info->certificate) &&
+                g_file_readable(client_info->key_file))
+        {
+            LOG(LOG_LEVEL_INFO, "Selected TLS security");
+            self->selectedProtocol = PROTOCOL_SSL;
+            got_protocol = 1;
+        }
+        else
+        {
+            LOG(LOG_LEVEL_WARNING, "Cannot accept TLS connections because "
+                "certificate or private key file is not readable. "
+                "certificate file: [%s], private key file: [%s]",
+                client_info->certificate, client_info->key_file);
+
+            /* If we're configured to ONLY use TLS, this is a problem.
+             * If not, we can fall back to RDP */
+            if (client_info->security_layer == SECURITY_LAYER_TLS)
             {
-                configured_str = "";
-                LOG(LOG_LEVEL_ERROR, "Cannot accept TLS connections because "
-                    "certificate or private key file is not readable. "
-                    "certificate file: [%s], private key file: [%s]",
-                    client_info->certificate, client_info->key_file);
+                LOG(LOG_LEVEL_ERROR,
+                    "Server requires TLS (security_layer=tls)");
                 self->failureCode = SSL_CERT_NOT_ON_SERVER;
-                rv = 1; /* error */
-            }
-            break;
-        case PROTOCOL_HYBRID:
-        case PROTOCOL_HYBRID_EX:
-        default:
-            /* We don't yet support CredSSP */
-            if (ssl_capable)
-            {
-                configured_str = "SSL|RDP";
+                rv = 1;
             }
-            else
-            {
-                /*
-                 * Tell the user we can't offer TLS, but this isn't fatal */
-                configured_str = "RDP";
-                LOG(LOG_LEVEL_WARNING, "Cannot accept TLS connections because "
-                    "certificate or private key file is not readable. "
-                    "certificate file: [%s], private key file: [%s]",
-                    client_info->certificate, client_info->key_file);
-            }
-            break;
-    }
-
-    /* Currently the choice comes down to RDP or SSL */
-    if (rv != 0)
-    {
-        self->selectedProtocol = PROTOCOL_RDP;
-        selected_str = "";
+        }
     }
-    else if (ssl_capable && (self->requestedProtocol &
-                             client_info->security_layer &
-                             PROTOCOL_SSL) != 0)
+    else if (client_info->security_layer == SECURITY_LAYER_TLS)
     {
-        self->selectedProtocol = PROTOCOL_SSL;
-        selected_str = "SSL";
+        /* We don't have a match on TLS, but we'll accept nothing less */
+        LOG(LOG_LEVEL_ERROR, "Server requires TLS (security_layer=tls)");
+        self->failureCode = SSL_REQUIRED_BY_SERVER;
+        rv = 1;
     }
-    else
+
+    /* If we haven't got a match so far, and we haven't got a fail,
+     * try RDP */
+    if (!got_protocol && !rv)
     {
-        self->selectedProtocol = PROTOCOL_RDP;
-        selected_str = "RDP";
+        if (g_fips_mode_enabled())
+        {
+            /* This is a FIPS-mode machine, and we don't support classic RDP
+             * encryption */
+            LOG(LOG_LEVEL_ERROR,
+                "Server in FIPS mode requires TLS for security");
+            self->failureCode = SSL_REQUIRED_BY_SERVER;
+            rv = 1; /* error */
+        }
+        else
+        {
+            self->selectedProtocol = PROTOCOL_RDP;
+            LOG(LOG_LEVEL_INFO, "Selected classic RDP security");
+            LOG(LOG_LEVEL_WARNING, "Classic RDP security is not secure -"
+                " please configure TLS on the client and server");
+            got_protocol = 1;
+        }
     }
 
-    protocol_mask_to_str(self->requestedProtocol,
-                         requested_str, sizeof(requested_str));
-
-    LOG(LOG_LEVEL_INFO, "Security protocol: configured [%s], requested [%s],"
-        " selected [%s]",
-        configured_str, requested_str, selected_str);
-
     return rv;
 }