CVE-2019-14889: scp: Quote location to be used on shell

Single quote file paths to be used on commands to be executed on remote
shell.

Fixes T181

Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit 3830c7ae6eec751b7618d3fc159cb5bb3c8806a6)

1 files changed, 56 insertions, 6 deletions

diff --git a/src/scp.c b/src/scp.c
index 4b00aa5f..652551e3 100644
--- a/src/scp.c
+++ b/src/scp.c
@@ -29,6 +29,7 @@
 
 #include "libssh/priv.h"
 #include "libssh/scp.h"
+#include "libssh/misc.h"
 
 /**
  * @defgroup libssh_scp The SSH scp functions
@@ -119,6 +120,9 @@ int ssh_scp_init(ssh_scp scp)
 {
     int rc;
     char execbuffer[1024] = {0};
+    char *quoted_location = NULL;
+    size_t quoted_location_len = 0;
+    size_t scp_location_len;
 
     if (scp == NULL) {
         return SSH_ERROR;
@@ -130,33 +134,79 @@ int ssh_scp_init(ssh_scp scp)
         return SSH_ERROR;
     }
 
-    SSH_LOG(SSH_LOG_PROTOCOL,
-            "Initializing scp session %s %son location '%s'",
+    if (scp->location == NULL) {
+        ssh_set_error(scp->session, SSH_FATAL,
+                      "Invalid scp context: location is NULL");
+        return SSH_ERROR;
+    }
+
+    SSH_LOG(SSH_LOG_PROTOCOL, "Initializing scp session %s %son location '%s'",
             scp->mode == SSH_SCP_WRITE?"write":"read",
-            scp->recursive?"recursive ":"",
+            scp->recursive ? "recursive " : "",
             scp->location);
 
     scp->channel = ssh_channel_new(scp->session);
     if (scp->channel == NULL) {
+        ssh_set_error(scp->session, SSH_FATAL,
+                      "Channel creation failed for scp");
         scp->state = SSH_SCP_ERROR;
         return SSH_ERROR;
     }
 
     rc = ssh_channel_open_session(scp->channel);
     if (rc == SSH_ERROR) {
+        ssh_set_error(scp->session, SSH_FATAL,
+                      "Failed to open channel for scp");
+        scp->state = SSH_SCP_ERROR;
+        return SSH_ERROR;
+    }
+
+    /* In the worst case, each character would be replaced by 3 plus the string
+     * terminator '\0' */
+    scp_location_len = strlen(scp->location);
+    quoted_location_len = ((size_t)3 * scp_location_len) + 1;
+    /* Paranoia check */
+    if (quoted_location_len < scp_location_len) {
+        ssh_set_error(scp->session, SSH_FATAL,
+                      "Buffer overflow detected");
+        scp->state = SSH_SCP_ERROR;
+        return SSH_ERROR;
+    }
+
+    quoted_location = (char *)calloc(1, quoted_location_len);
+    if (quoted_location == NULL) {
+        ssh_set_error(scp->session, SSH_FATAL,
+                      "Failed to allocate memory for quoted location");
+        scp->state = SSH_SCP_ERROR;
+        return SSH_ERROR;
+    }
+
+    rc = ssh_quote_file_name(scp->location, quoted_location,
+                             quoted_location_len);
+    if (rc <= 0) {
+        ssh_set_error(scp->session, SSH_FATAL,
+                      "Failed to single quote command location");
+        SAFE_FREE(quoted_location);
         scp->state = SSH_SCP_ERROR;
         return SSH_ERROR;
     }
 
     if (scp->mode == SSH_SCP_WRITE) {
         snprintf(execbuffer, sizeof(execbuffer), "scp -t %s %s",
-                scp->recursive ? "-r":"", scp->location);
+                scp->recursive ? "-r" : "", quoted_location);
     } else {
         snprintf(execbuffer, sizeof(execbuffer), "scp -f %s %s",
-                scp->recursive ? "-r":"", scp->location);
+                scp->recursive ? "-r" : "", quoted_location);
     }
 
-    if (ssh_channel_request_exec(scp->channel, execbuffer) == SSH_ERROR) {
+    SAFE_FREE(quoted_location);
+
+    SSH_LOG(SSH_LOG_DEBUG, "Executing command: %s", execbuffer);
+
+    rc = ssh_channel_request_exec(scp->channel, execbuffer);
+    if (rc == SSH_ERROR){
+        ssh_set_error(scp->session, SSH_FATAL,
+                      "Failed executing command: %s", execbuffer);
         scp->state = SSH_SCP_ERROR;
         return SSH_ERROR;
     }