From ad96965b16a95217d1df98d9bb33cf87cf6fa77c Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Mon, 20 Oct 2025 20:09:39 +0200 Subject: [PATCH] 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 --- src/bin/copy.c | 25 ++++++---- src/bin/erase.c | 34 +++++++------- src/bin/util.c | 74 +++++++++++++++++++----------- src/bin/util.h | 9 ++-- src/klish-plugin-infix/src/infix.c | 4 +- 5 files changed, 86 insertions(+), 60 deletions(-) diff --git a/src/bin/copy.c b/src/bin/copy.c index 77f7a2b3..ad0c9ce7 100644 --- a/src/bin/copy.c +++ b/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; diff --git a/src/bin/erase.c b/src/bin/erase.c index 716c310b..acfe8c4a 100644 --- a/src/bin/erase.c +++ b/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)) { - size_t len = strlen(path) + 10; + fn = cfg_adjust(path, NULL, buf, sizeof(buf), sanitize); + if (!fn) { + fprintf(stderr, ERRMSG "file not found.\n"); + return 1; + } - fn = alloca(len); - 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)) + if (!yorn("Remove %s, are you sure", path)) return 0; if (remove(fn)) { - fprintf(stderr, ERRMSG "failed removing %s: %s\n", fn, strerror(errno)); - return -1; + fprintf(stderr, ERRMSG "failed removing %s: %s\n", path, strerror(errno)); + 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; diff --git a/src/bin/util.c b/src/bin/util.c index 0e739b1e..629ecfde 100644 --- a/src/bin/util.c +++ b/src/bin/util.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -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, "../")) - return NULL; /* relative paths not allowed */ + const char *accepted[] = { + "/media/", + "/cfg/", + getenv("HOME"), + NULL + }; - if (fn[0] == '/') { - strlcpy(buf, fn, len); - return buf; /* allow absolute paths */ + for (int i = 0; accepted[i]; i++) { + if (!strncmp(path, accepted[i], strlen(accepted[i]))) + return 1; } - /* Files in /cfg must end in .cfg */ - if (!strncmp(fn, "/cfg/", 5)) { - strlcpy(buf, fn, len); - if (!has_ext(fn, ".cfg")) - strlcat(buf, ".cfg", len); + return 0; +} - 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 (has_ext(fn, ".cfg")) { - snprintf(buf, len, "/cfg/%s", fn); - return buf; + if (sanitize) { + if (strstr(fn, "../")) + return NULL; + + 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) { - if (fn[1] == '/' && fn[2] != 0) - strlcpy(buf, fn, len); - else - strlcpy(buf, basenm(tmpl), len); - } else - strlcpy(buf, fn, len); - + strlcpy(buf, fn, len); return buf; } diff --git a/src/bin/util.h b/src/bin/util.h index 6c5ded9d..2b34e25b 100644 --- a/src/bin/util.h +++ b/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); -char *cfg_adjust (const char *fn, const char *tmpl, char *buf, size_t len); +const char *basenm (const char *fn); +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_ */ diff --git a/src/klish-plugin-infix/src/infix.c b/src/klish-plugin-infix/src/infix.c index 5ee0d0ed..47c821b1 100644 --- a/src/klish-plugin-infix/src/infix.c +++ b/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)