cli: restrict copy and erase commands

This is a follow-up to PR #717 where path traversal protection was
discussed.  A year later and it's clear that having a user-friendly
copy tool in the shell is a good thing, but that we proably want to
restrict what it can do when called from the CLI.

A sanitize flag (-s) is added to control the behavior, when used in the
shell without -s, both commands act like traditional UNIX tools and do
assume . for relative paths, and allow ../, whereas when running from
the CLI only /media/ is allowed and otherwise files are assumed to be
in $HOME or /cfg

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

src/bin/copy.c

@@ -36,6 +36,7 @@ static const char *prognm = "copy";
 static int timeout;
 static int dry_run;
 static int quiet;
+static int sanitize;
 
 
 /*
@@ -204,7 +205,7 @@ static int replace_running(sr_conn_ctx_t *conn, sr_session_ctx_t *sess,
 
 	if (dry_run) {
 		if (!quiet) {
-			printf("Configuration validated, %s: OK\n", file);
+			printf("Configuration validated, %s OK\n", file);
 			fflush(stdout);
 		}
 		lyd_free_all(data);
@@ -334,10 +335,10 @@ static int copy(const char *src, const char *dst, const char *remote_user)
 		if (dstds && dstds->path)
 			fn = dstds->path;
 		else
-			fn = cfg_adjust(dst, src, adjust, sizeof(adjust));
+			fn = cfg_adjust(dst, src, adjust, sizeof(adjust), sanitize);
 
 		if (!fn) {
-			fprintf(stderr, ERRMSG "invalid destination path.\n");
+			fprintf(stderr, ERRMSG "file not found.\n");
 			rc = -1;
 			goto err;
 		}
@@ -371,9 +372,9 @@ static int copy(const char *src, const char *dst, const char *remote_user)
 			tmpfn = mktemp(temp_file);
 			fn = tmpfn;
 		} else {
-			fn = cfg_adjust(src, NULL, adjust, sizeof(adjust));
+			fn = cfg_adjust(src, NULL, adjust, sizeof(adjust), sanitize);
 			if (!fn) {
-				fprintf(stderr, ERRMSG "invalid source file location.\n");
+				fprintf(stderr, ERRMSG "file not found.\n");
 				rc = 1;
 				goto err;
 			}
@@ -423,9 +424,9 @@ static int copy(const char *src, const char *dst, const char *remote_user)
 		}
 
 		if (strstr(src, "://")) {
-			fn = cfg_adjust(dst, src, adjust, sizeof(adjust));
+			fn = cfg_adjust(dst, src, adjust, sizeof(adjust), sanitize);
 			if (!fn) {
-				fprintf(stderr, ERRMSG "invalid destination file location.\n");
+				fprintf(stderr, ERRMSG "file not found.\n");
 				rc = 1;
 				goto err;
 			}
@@ -439,9 +440,9 @@ static int copy(const char *src, const char *dst, const char *remote_user)
 
 			rc = systemf("curl %s -Lo %s %s", remote_user, fn, src);
 		} else if (strstr(dst, "://")) {
-			fn = cfg_adjust(src, NULL, adjust, sizeof(adjust));
+			fn = cfg_adjust(src, NULL, adjust, sizeof(adjust), sanitize);
 			if (!fn) {
-				fprintf(stderr, ERRMSG "invalid source file location.\n");
+				fprintf(stderr, ERRMSG "file not found.\n");
 				rc = 1;
 				goto err;
 			}
@@ -479,6 +480,7 @@ static int usage(int rc)
 	       "  -h         This help text\n"
 	       "  -n         Dry-run, validate configuration without applying\n"
 	       "  -q         Quiet mode, suppress informational messages\n"
+	       "  -s         Sanitize paths for CLI use (restrict path traversal)\n"
 	       "  -u USER    Username for remote commands, like scp\n"
 	       "  -t SEEC    Timeout for the operation, or default %d sec\n"
 	       "  -v         Show version\n", prognm, timeout);
@@ -493,7 +495,7 @@ int main(int argc, char *argv[])
 
 	timeout = fgetint("/etc/default/confd", "=", "CONFD_TIMEOUT");
 
-	while ((c = getopt(argc, argv, "hnqt:u:v")) != EOF) {
+	while ((c = getopt(argc, argv, "hnqst:u:v")) != EOF) {
 		switch(c) {
 		case 'h':
 			return usage(0);
@@ -503,6 +505,9 @@ int main(int argc, char *argv[])
 		case 'q':
 			quiet = 1;
 			break;
+		case 's':
+			sanitize = 1;
+			break;
 		case 't':
 			timeout = atoi(optarg);
 			break;

src/bin/erase.c

@@ -10,31 +10,25 @@
 #include "util.h"
 
 static const char *prognm = "erase";
-
+static int sanitize;
 
 static int do_erase(const char *path)
 {
-	char *fn;
+	char buf[PATH_MAX];
+	const char *fn;
 
-	if (access(path, F_OK)) {
+	fn = cfg_adjust(path, NULL, buf, sizeof(buf), sanitize);
-		size_t len = strlen(path) + 10;
+	if (!fn) {
+		fprintf(stderr, ERRMSG "file not found.\n");
+		return 1;
+	}
 
-		fn = alloca(len);
+	if (!yorn("Remove %s, are you sure", path))
-		if (!fn) {
-			fprintf(stderr, ERRMSG "failed allocating memory.\n");
-			return -1;
-		}
-
-		cfg_adjust(path, NULL, fn, len);
-	} else
-		fn = (char *)path;
-
-	if (!yorn("Remove %s, are you sure", fn))
 		return 0;
 
 	if (remove(fn)) {
-		fprintf(stderr, ERRMSG "failed removing %s: %s\n", fn, strerror(errno));
+		fprintf(stderr, ERRMSG "failed removing %s: %s\n", path, strerror(errno));
-		return -1;
+		return 11;
 	}
 
 	return 0;
@@ -46,6 +40,7 @@ static int usage(int rc)
 	       "\n"
 	       "Options:\n"
 	       "  -h         This help text\n"
+	       "  -s         Sanitize paths for CLI use (restrict path traversal)\n"
 	       "  -v         Show version\n", prognm);
 
 	return rc;
@@ -55,10 +50,13 @@ int main(int argc, char *argv[])
 {
 	int c;
 
-	while ((c = getopt(argc, argv, "hv")) != EOF) {
+	while ((c = getopt(argc, argv, "hsv")) != EOF) {
 		switch(c) {
 		case 'h':
 			return usage(0);
+		case 's':
+			sanitize = 1;
+			break;
 		case 'v':
 			puts(PACKAGE_VERSION);
 			return 0;

src/bin/util.c

@@ -5,6 +5,7 @@
 #include <errno.h>
 #include <stdarg.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <termios.h>
 
@@ -67,7 +68,7 @@ int has_ext(const char *fn, const char *ext)
 	return 0;
 }
 
-static const char *basenm(const char *fn)
+const char *basenm(const char *fn)
 {
 	const char *ptr;
 
@@ -75,44 +76,65 @@ static const char *basenm(const char *fn)
 		return "";
 
 	ptr = strrchr(fn, '/');
-	if (!ptr)
+	if (ptr)
+		ptr++;
+	else
 		ptr = fn;
 
 	return ptr;
 }
 
-char *cfg_adjust(const char *fn, const char *tmpl, char *buf, size_t len)
+static int path_allowed(const char *path)
 {
-	if (strstr(fn, "../"))
+	const char *accepted[] = {
-		return NULL;	/* relative paths not allowed */
+		"/media/",
+		"/cfg/",
+		getenv("HOME"),
+		NULL
+	};
 
-	if (fn[0] == '/') {
+	for (int i = 0; accepted[i]; i++) {
-		strlcpy(buf, fn, len);
+		if (!strncmp(path, accepted[i], strlen(accepted[i])))
-		return buf;	/* allow absolute paths */
+			return 1;
 	}
 
-	/* Files in /cfg must end in .cfg */
+	return 0;
-	if (!strncmp(fn, "/cfg/", 5)) {
+}
-		strlcpy(buf, fn, len);
-		if (!has_ext(fn, ".cfg"))
-			strlcat(buf, ".cfg", len);
 
-		return buf;
+char *cfg_adjust(const char *fn, const char *tmpl, char *buf, size_t len, int sanitize)
+{
+	char tmp[256], resolved[PATH_MAX];
+
+	if (strlen(basenm(fn)) == 0) {
+		if (!tmpl)
+			return NULL;
+
+		fn = basenm(tmpl);
+		/* Fall through */
 	}
 
-	/* Files ending with .cfg belong in /cfg */
+	if (sanitize) {
-	if (has_ext(fn, ".cfg")) {
+		if (strstr(fn, "../"))
-		snprintf(buf, len, "/cfg/%s", fn);
+			return NULL;
-		return buf;
+
+		if (fn[0] == '/') {
+			if (!path_allowed(fn))
+				return NULL;
+		} else {
+			snprintf(tmp, sizeof(tmp), "/cfg/%s", fn);
+			if (!has_ext(tmp, ".cfg"))
+				strlcat(tmp, ".cfg", sizeof(tmp));
+			fn = tmp;
+		}
+
+		/* If file exists, resolve symlinks and verify still in whitelist */
+		if (!access(fn, F_OK) && realpath(fn, resolved)) {
+			if (!path_allowed(resolved))
+				return NULL;
+			fn = resolved;
+		}
 	}
 
-	if (strlen(fn) > 0 && fn[0] == '.' && tmpl) {
+	strlcpy(buf, fn, len);
-		if (fn[1] == '/' && fn[2] != 0)
-			strlcpy(buf, fn, len);
-		else
-			strlcpy(buf, basenm(tmpl), len);
-	} else
-		strlcpy(buf, fn, len);
-
 	return buf;
 }

src/bin/util.h

@@ -7,11 +7,12 @@
 #define ERRMSG "Error: "
 #define INFMSG "Note: "
 
-int   yorn       (const char *fmt, ...);
+int         yorn       (const char *fmt, ...);
 
-int   files      (const char *path, const char *stripext);
+int         files      (const char *path, const char *stripext);
 
-int   has_ext    (const char *fn, const char *ext);
+const char *basenm     (const char *fn);
-char *cfg_adjust (const char *fn, const char *tmpl, char *buf, size_t len);
+int         has_ext    (const char *fn, const char *ext);
+char       *cfg_adjust (const char *fn, const char *tmpl, char *buf, size_t len, int sanitize);
 
 #endif /* BIN_UTIL_H_ */

src/klish-plugin-infix/src/infix.c

@@ -120,7 +120,7 @@ int infix_erase(kcontext_t *ctx)
 
 	cd_home(ctx);
 
-	return systemf("erase %s", path);
+	return systemf("erase -s %s", path);
 }
 
 int infix_files(kcontext_t *ctx)
@@ -221,7 +221,7 @@ int infix_copy(kcontext_t *ctx)
 		strlcpy(validate, "-n", sizeof(validate));
 
 	/* Ensure we run the copy command as the logged-in user, not root (klishd) */
-	return systemf("doas -u %s copy %s %s %s %s", cd_home(ctx), validate, user, src, dst);
+	return systemf("doas -u %s copy -s %s %s %s %s", cd_home(ctx), validate, user, src, dst);
 }
 
 int infix_shell(kcontext_t *ctx)