From 5ea99680c851b19f76ce5bf38615d7d43d3cf015 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Mon, 20 Oct 2025 14:39:55 +0200 Subject: [PATCH] bin: backport fixes to shell/cli copy command This is a backport of the following commits from origin/main: 3b24fab e6a04fb 92e80b4 ad96965 3e03ece 21256a8 b7d91e4 d26e311 0977ab4 f83fbc6 --- cli: fix 'copy FILE running-config' use-case When copying to the running datastore we cannot use sr_copy_config(), instead we must use sr_replace_config(). This fix covers both the case of 'copy startup-config running-config' and 'copy FILE running-config'. Fixes #1203 --- cli: add 'validate', or '-n', dry run to copy command This commit adds config file validation to the copy command, discussed in #373. Allowing users to test their config files before restoring a backup. The feature could also be used for the automatic rollback when downgrading to an earlier version of the OS. Fixes #373 --- cli: fix copy to missing startup-config file Fixes #981 --- 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 --- cli: sanitize regular file to file copy The regular file-to-file copy, was missing calls to cfg_adjust(), this commit fixes that and adds some helpful comments for each use-case. Also, drop insecure mktemp() in favor of our own version which uses the basename of the remote source file. --- bin: add bash completion for copy command Add bash completion for the common datastores, like we already do in the CLI, and update the usage text accordingly. Also, make sure to install to /usr/bin, not /bin since we've now merged the hierarchies since a while back. --- bin: copy: Refactor copy() made some...creative...use of control flow that made it quite difficult to follow. Take a first priciples approach to simplify the logic. --- bin: copy: Always get startup from sysrepo This will make sure to apply NACM rules for all the data. It also makes it possible for a luser access a subset of the data, even if they to do not have read access to /cfg/startup-config.cfg. Signed-off-by: Joachim Wiberg --- package/bin/bin.mk | 2 +- src/bin/Makefile.am | 8 +- src/bin/configure.ac | 5 +- src/bin/copy.bash | 93 ++++ src/bin/copy.c | 665 ++++++++++++++++++--------- src/bin/erase.c | 50 +- src/bin/util.c | 117 +++-- src/bin/util.h | 10 +- src/klish-plugin-infix/src/infix.c | 26 +- src/klish-plugin-infix/xml/infix.xml | 1 + 10 files changed, 665 insertions(+), 312 deletions(-) create mode 100644 src/bin/copy.bash diff --git a/package/bin/bin.mk b/package/bin/bin.mk index 5ac94ac9..15d276a9 100644 --- a/package/bin/bin.mk +++ b/package/bin/bin.mk @@ -11,7 +11,7 @@ BIN_LICENSE = BSD-3-Clause BIN_LICENSE_FILES = LICENSE BIN_REDISTRIBUTE = NO BIN_DEPENDENCIES = sysrepo libite -BIN_CONF_OPTS = --prefix= --disable-silent-rules +BIN_CONF_OPTS = --disable-silent-rules BIN_AUTORECONF = YES define BIN_CONF_ENV diff --git a/src/bin/Makefile.am b/src/bin/Makefile.am index 15ede0cf..f33dd3e9 100644 --- a/src/bin/Makefile.am +++ b/src/bin/Makefile.am @@ -3,11 +3,15 @@ ACLOCAL_AMFLAGS = -I m4 bin_PROGRAMS = copy erase files +# Bash completion +bashcompdir = $(datadir)/bash-completion/completions +dist_bashcomp_DATA = copy.bash + copy_SOURCES = copy.c util.c util.h copy_CPPFLAGS = -D_DEFAULT_SOURCE -D_GNU_SOURCE copy_CFLAGS = -W -Wall -Wextra -copy_CFLAGS += $(libite_CFLAGS) $(sysrepo_CFLAGS) -copy_LDADD = $(libite_LIBS) $(sysrepo_LIBS) +copy_CFLAGS += $(libite_CFLAGS) $(libyang_CFLAGS) $(sysrepo_CFLAGS) +copy_LDADD = $(libite_LIBS) $(libyang_LIBS) $(sysrepo_LIBS) erase_SOURCES = erase.c util.c util.h erase_CPPFLAGS = -D_DEFAULT_SOURCE -D_GNU_SOURCE diff --git a/src/bin/configure.ac b/src/bin/configure.ac index 10aaf0a8..8b5c12c8 100644 --- a/src/bin/configure.ac +++ b/src/bin/configure.ac @@ -15,6 +15,7 @@ AC_PROG_INSTALL PKG_PROG_PKG_CONFIG PKG_CHECK_MODULES([libite], [libite >= 2.5.0]) +PKG_CHECK_MODULES([libyang], [libyang >= 3.0.0]) PKG_CHECK_MODULES([sysrepo], [sysrepo >= 2.2.36]) # Misc variable replacements for below Summary @@ -55,8 +56,8 @@ cat < #include +#include #include #include @@ -18,58 +19,29 @@ struct infix_ds { char *name; /* startup-config, etc. */ - char *sysrepocfg; /* ds name in sysrepocfg */ int datastore; /* sr_datastore_t and -1 */ - int rw; /* read-write:1 or not:0 */ + bool rw; /* read-write:1 or not:0 */ char *path; /* local path or NULL */ }; -struct infix_ds infix_config[] = { - { "startup-config", "startup", SR_DS_STARTUP, 1, "/cfg/startup-config.cfg" }, - { "running-config", "running", SR_DS_RUNNING, 1, NULL }, - { "candidate-config", "candidate", SR_DS_CANDIDATE, 1, NULL }, - { "operational-config", "operational", SR_DS_OPERATIONAL, 1, NULL }, - { "factory-config", "factory-default", SR_DS_FACTORY_DEFAULT, 0, NULL } +const struct infix_ds infix_config[] = { + { "startup-config", SR_DS_STARTUP, true, "/cfg/startup-config.cfg" }, + { "running-config", SR_DS_RUNNING, true, NULL }, + /* { "candidate-config", SR_DS_CANDIDATE, true, NULL }, */ + { "operational-state", SR_DS_OPERATIONAL, false, NULL }, + { "factory-config", SR_DS_FACTORY_DEFAULT, false, NULL } }; static const char *prognm = "copy"; +static const char *remote_user; static int timeout; - - -/* - * Print sysrepo session errors followed by an optional string. - */ -static void emsg(sr_session_ctx_t *sess, const char *fmt, ...) -{ - const sr_error_info_t *err = NULL; - va_list ap; - size_t i; - int rc; - - if (!sess) - goto end; - - rc = sr_session_get_error(sess, &err); - if ((rc != SR_ERR_OK) || !err) - goto end; - - // Show the first error only. Because probably next errors are - // originated from internal sysrepo code but is not from subscribers. -// for (i = 0; i < err->err_count; i++) - for (i = 0; i < (err->err_count < 1 ? err->err_count : 1); i++) - fprintf(stderr, ERRMSG "%s\n", err->err[i].message); -end: - if (fmt) { - va_start(ap, fmt); - vfprintf(stderr, fmt, ap); - va_end(ap); - } -} +static int dry_run; +static int sanitize; /* * Current system user, same as sysrepo user */ -static char *getuser(void) +static const char *getuser(void) { const struct passwd *pw; uid_t uid; @@ -163,7 +135,7 @@ static void set_owner(const char *fn, const char *user) } } -static const char *infix_ds(const char *text, struct infix_ds **ds) +static const char *infix_ds(const char *text, const struct infix_ds **ds) { size_t i, len = strlen(text); @@ -174,209 +146,424 @@ static const char *infix_ds(const char *text, struct infix_ds **ds) } } + *ds = NULL; return text; } +static bool is_uri(const char *str) +{ + return strstr(str, "://") != NULL; +} -static int copy(const char *src, const char *dst, const char *remote_user) +static char *mktmp(void) { - struct infix_ds *srcds = NULL, *dstds = NULL; - char temp_file[20] = "/tmp/copy.XXXXXX"; - const char *tmpfn = NULL; - sr_session_ctx_t *sess; - const char *fn = NULL; - sr_conn_ctx_t *conn; - const char *user; - char adjust[256]; mode_t oldmask; - int rc = 0; + char *path; + int fd; + + path = strdup("/tmp/copy-XXXXXX"); + if (!path) + goto err; + + oldmask = umask(0077); + fd = mkstemp(path); + umask(oldmask); + + if (fd < 0) + goto err; + + close(fd); + return path; +err: + free(path); + return NULL; +} + +static void rmtmp(const char *path) +{ + if (remove(path)) { + if (errno == ENOENT) + return; + + fprintf(stderr, ERRMSG "removal of temporary file %s failed\n", path); + } +} + + +static void sysrepo_print_error(sr_session_ctx_t *sess) +{ + const sr_error_info_t *erri = NULL; + int err; + + err = sr_session_get_error(sess, &erri); + if (err || !erri || !erri->err_count) + return; + + fprintf(stderr, ERRMSG "%s (%d)\n", erri->err->message, erri->err->err_code); +} + +static sr_session_ctx_t *sysrepo_session(const struct infix_ds *ds) +{ + static sr_session_ctx_t *sess; + + sr_subscription_ctx_t *sub = NULL; + const char *user = getuser(); + sr_conn_ctx_t *conn = NULL; + int err; + + if (!ds) { + if (!sess) + return NULL; + + conn = sr_session_get_connection(sess); + sr_session_stop(sess); + sr_disconnect(conn); + return NULL; + } + + if (!sess) { + err = sr_connect(0, &conn); + if (err != SR_ERR_OK) { + sysrepo_print_error(sess); + fprintf(stderr, ERRMSG "could not connect to %s\n", ds->name); + goto err; + } + + /* Always open running, because sr_nacm_init() does not work + * against the factory DS. + */ + err = sr_session_start(conn, SR_DS_RUNNING, &sess); + if (err != SR_ERR_OK) { + sysrepo_print_error(sess); + fprintf(stderr, ERRMSG "%s session setup failed\n", ds->name); + goto err_disconnect; + } + + err = sr_nacm_init(sess, 0, &sub); + if (err != SR_ERR_OK) { + sysrepo_print_error(sess); + fprintf(stderr, ERRMSG "%s NACM setup failed\n", ds->name); + goto err_stop; + } + + err = sr_nacm_set_user(sess, user); + if (err != SR_ERR_OK) { + sysrepo_print_error(sess); + fprintf(stderr, ERRMSG "%s NACM setup for %s failed\n", ds->name, user); + goto err_nacm_destroy; + } + } + + err = sr_session_switch_ds(sess, ds->datastore); + if (err) { + sysrepo_print_error(sess); + fprintf(stderr, ERRMSG "%s activation failed\n", ds->name); + return NULL; + } + + return sess; + +err_nacm_destroy: + sr_nacm_destroy(); +err_stop: + sr_session_stop(sess); +err_disconnect: + sr_disconnect(conn); +err: + sess = NULL; + return NULL; +} + +static int sysrepo_export(const struct infix_ds *ds, const char *path) +{ + sr_session_ctx_t *sess; + sr_data_t *data; + int err; + + sess = sysrepo_session(ds); + if (!sess) + return 1; + + err = sr_get_data(sess, "/*", 0, timeout * 1000, SR_OPER_DEFAULT, &data); + if (err) { + sysrepo_print_error(sess); + fprintf(stderr, ERRMSG "retrieval of %s data failed\n", ds->name); + return err; + } + + err = lyd_print_path(path, data->tree, LYD_JSON, LYD_PRINT_WITHSIBLINGS); + sr_release_data(data); + if (err) { + sysrepo_print_error(sess); + fprintf(stderr, ERRMSG "failed to store %s data\n", ds->name); + return err; + } + + return 0; +} + +static int sysrepo_import(const struct infix_ds *ds, const char *path) +{ + const struct ly_ctx *ly; + sr_session_ctx_t *sess; + struct lyd_node *data; + int err; + + sess = sysrepo_session(ds); + if (!sess) + return 1; + + ly = sr_acquire_context(sr_session_get_connection(sess)); + + err = lyd_parse_data_path(ly, path, LYD_JSON, + LYD_PARSE_NO_STATE | LYD_PARSE_ONLY | + LYD_PARSE_STORE_ONLY | LYD_PARSE_STRICT, 0, &data); + if (err) { + fprintf(stderr, ERRMSG "failed to parse %s data\n", ds->name); + goto out; + } + + err = dry_run ? 0 : sr_replace_config(sess, NULL, data, timeout * 1000); + if (err) { + sysrepo_print_error(sess); + fprintf(stderr, ERRMSG "failed import %s data\n", ds->name); + } + +out: + sr_release_context(sr_session_get_connection(sess)); + return err ? 1 : 0; + /* return sysrepo_do(sysrepo_import_op, ds, path) ? 1 : 0; */ +} + +static int subprocess(char * const *argv) +{ + int pid, status; + + pid = fork(); + if (!pid) { + execvp(argv[0], argv); + exit(1); + } + + if (pid < 0) + return 1; + + if (waitpid(pid, &status, 0) < 0) + return 1; + + if (!WIFEXITED(status)) + return 1; + + return WEXITSTATUS(status); +} + +static int curl(char *op, const char *path, const char *uri) +{ + char *argv[] = { + "curl", "-L", op, NULL, NULL, NULL, NULL, NULL, + }; + int err = 1; + + argv[3] = strdup(path); + argv[4] = strdup(uri); + if (!(argv[3] && argv[4])) + goto out; + + if (remote_user) { + argv[5] = strdup("-u"); + argv[6] = strdup(remote_user); + if (!(argv[5] && argv[6])) + goto out; + } + err = subprocess(argv); + +out: + free(argv[6]); + free(argv[5]); + free(argv[4]); + free(argv[3]); + return err; +} + +static int curl_upload(const char *srcpath, const char *uri) +{ + char upload[] = "-T"; + + if (curl(upload, srcpath, uri)) { + fprintf(stderr, ERRMSG "upload to %s failed\n", uri); + return 1; + } + + return 0; +} + +static int curl_download(const char *uri, const char *dstpath) +{ + char download[] = "-o"; + + if (curl(download, dstpath, uri)) { + fprintf(stderr, ERRMSG "download of %s failed\n", uri); + return 1; + } + + return 0; +} + +static int cp(const char *srcpath, const char *dstpath) +{ + char *argv[] = { + "cp", NULL, NULL, NULL, + }; + int err = 1; + + argv[1] = strdup(srcpath); + argv[2] = strdup(dstpath); + if (!(argv[1] && argv[2])) + goto out; + + err = subprocess(argv); + if (err) + fprintf(stderr, ERRMSG "failed to save %s\n", dstpath); +out: + free(argv[2]); + free(argv[1]); + return err; +} + +static int put(const char *srcpath, const char *dst, + const struct infix_ds *ds, const char *path) +{ + int err = 0; + + if (ds) + err = sysrepo_import(ds, srcpath); + else if (is_uri(dst)) + err = curl_upload(srcpath, dst); + + if (err) + return err; + + if (path) { + err = cp(srcpath, path); + if (!err) + set_owner(path, getuser()); + } + + return 0; +} + +static int get(const char *src, const struct infix_ds *ds, const char *path) +{ + int err = 0; + + if (ds) + err = sysrepo_export(ds, path); + else if (is_uri(src)) + err = curl_download(src, path); + + return err; +} + +static int resolve_src(const char **src, const struct infix_ds **ds, char **path, bool *rm) +{ + *src = infix_ds(*src, ds); + + if (*ds || is_uri(*src)) { + *path = mktmp(); + if (!*path) + return 1; + + *rm = true; + return 0; + } else { + *path = cfg_adjust(*src, NULL, sanitize); + } + + if (!*path) { + fprintf(stderr, ERRMSG "no such file %s.", *src); + return 1; + } + + *rm = false; + return 0; +} + +static int resolve_dst(const char **dst, const struct infix_ds **ds, char **path) +{ + *dst = infix_ds(*dst, ds); + + if (*ds) { + if (!(*ds)->rw) { + fprintf(stderr, ERRMSG "%s is not writable", (*ds)->name); + return 1; + } + + if (!(*ds)->path) + return 0; + + *path = strdup((*ds)->path); + } else if (is_uri(*dst)) { + return 0; + } else { + *path = cfg_adjust(*dst, NULL, sanitize); + } + + if (!*path) { + fprintf(stderr, ERRMSG "no such file: %s", *dst); + return 1; + } + + if (!*ds && !access(*path, F_OK) && !yorn("Overwrite existing file %s", *path)) { + fprintf(stderr, "OK, aborting.\n"); + return 1; + } + + return 0; +} + +static int copy(const char *src, const char *dst) +{ + char *srcpath = NULL, *dstpath = NULL; + const struct infix_ds *srcds, *dstds; + bool rmsrc = false; + mode_t oldmask; + int err = 1; /* rw for user and group only */ oldmask = umask(0006); - src = infix_ds(src, &srcds); - if (!src) - goto err; - dst = infix_ds(dst, &dstds); - if (!dst) - goto err; - if (!strcmp(src, dst)) { fprintf(stderr, ERRMSG "source and destination are the same, aborting.\n"); goto err; } - user = getuser(); + err = resolve_src(&src, &srcds, &srcpath, &rmsrc); + if (err) + goto err; - /* 1. Regular ds copy */ - if (srcds && dstds) { - /* Ensure the dst ds is writable */ - if (!dstds->rw) { - fprintf(stderr, ERRMSG "not possible to write to \"%s\", skipping.\n", dst); - rc = 1; - goto err; - } + err = resolve_dst(&dst, &dstds, &dstpath); + if (err) + goto err; - if (sr_connect(SR_CONN_DEFAULT, &conn)) { - fprintf(stderr, ERRMSG "connection to datastore failed\n"); - rc = 1; - goto err; - } + err = get(src, srcds, srcpath); + if (err) + goto err; - sr_log_syslog("klishd", SR_LL_WRN); - - if (sr_session_start(conn, dstds->datastore, &sess)) { - fprintf(stderr, ERRMSG "unable to open transaction to %s\n", dst); - } else { - sr_nacm_set_user(sess, user); - rc = sr_copy_config(sess, NULL, srcds->datastore, timeout * 1000); - if (rc) - emsg(sess, ERRMSG "unable to copy configuration, err %d: %s\n", - rc, sr_strerror(rc)); - else - set_owner(dstds->path, user); - } - rc = sr_disconnect(conn); - - if (!srcds->path || !dstds->path) - goto err; /* done, not an error */ - - /* allow copy factory startup */ - } - - if (srcds) { - /* 2. Export from a datastore somewhere else */ - if (strstr(dst, "://")) { - if (srcds->path) - fn = srcds->path; - else { - snprintf(adjust, sizeof(adjust), "/tmp/%s.cfg", srcds->name); - fn = tmpfn = adjust; - rc = systemf("sysrepocfg -d %s -X%s -f json", srcds->sysrepocfg, fn); - } - - if (rc) - fprintf(stderr, ERRMSG "failed exporting %s to %s\n", src, fn); - else { - rc = systemf("curl %s -LT %s %s", remote_user, fn, dst); - if (rc) - fprintf(stderr, ERRMSG "failed uploading %s to %s\n", src, dst); - else - set_owner(dst, user); - } - goto err; - } - - if (dstds && dstds->path) - fn = dstds->path; - else - fn = cfg_adjust(dst, src, adjust, sizeof(adjust)); - - if (!fn) { - fprintf(stderr, ERRMSG "invalid destination path.\n"); - rc = -1; - goto err; - } - - if (!access(fn, F_OK) && !yorn("Overwrite existing file %s", fn)) { - fprintf(stderr, "OK, aborting.\n"); - return 0; - } - - if (srcds->path) - rc = systemf("cp %s %s", srcds->path, fn); - else - rc = systemf("sysrepocfg -d %s -X%s -f json", srcds->sysrepocfg, fn); - if (rc) - fprintf(stderr, ERRMSG "failed copy %s to %s\n", src, fn); - else - set_owner(fn, user); - } else if (dstds) { - if (!dstds->sysrepocfg) { - fprintf(stderr, ERRMSG "not possible to import to this datastore.\n"); - rc = 1; - goto err; - } - if (!dstds->rw) { - fprintf(stderr, ERRMSG "not possible to write to %s", dst); - goto err; - } - - /* 3. Import from somewhere to a datastore */ - if (strstr(src, "://")) { - tmpfn = mktemp(temp_file); - fn = tmpfn; - } else { - fn = cfg_adjust(src, NULL, adjust, sizeof(adjust)); - if (!fn) { - fprintf(stderr, ERRMSG "invalid source file location.\n"); - rc = 1; - goto err; - } - } - - if (tmpfn) - rc = systemf("curl %s -Lo %s %s", remote_user, fn, src); - if (rc) { - fprintf(stderr, ERRMSG "failed downloading %s", src); - } else { - rc = systemf("sysrepocfg -d %s -I%s -f json", dstds->sysrepocfg, fn); - if (rc) - fprintf(stderr, ERRMSG "failed loading %s from %s", dst, src); - } - } else { - if (strstr(src, "://") && strstr(dst, "://")) { - fprintf(stderr, ERRMSG "copy from remote to remote is not supported.\n"); - goto err; - } - - if (strstr(src, "://")) { - fn = cfg_adjust(dst, src, adjust, sizeof(adjust)); - if (!fn) { - fprintf(stderr, ERRMSG "invalid destination file location.\n"); - rc = 1; - goto err; - } - - if (!access(fn, F_OK)) { - if (!yorn("Overwrite existing file %s", fn)) { - fprintf(stderr, "OK, aborting.\n"); - return 0; - } - } - - rc = systemf("curl %s -Lo %s %s", remote_user, fn, src); - } else if (strstr(dst, "://")) { - fn = cfg_adjust(src, NULL, adjust, sizeof(adjust)); - if (!fn) { - fprintf(stderr, ERRMSG "invalid source file location.\n"); - rc = 1; - goto err; - } - - if (access(fn, F_OK)) - fprintf(stderr, ERRMSG "no such file %s, aborting.", fn); - else - rc = systemf("curl %s -LT %s %s", remote_user, fn, dst); - } else { - if (!access(dst, F_OK)) { - if (!yorn("Overwrite existing file %s", dst)) { - fprintf(stderr, "OK, aborting.\n"); - return 0; - } - } - rc = systemf("cp %s %s", src, dst); - } - } + err = put(srcpath, dst, dstds, dstpath); err: - if (tmpfn) - rc = remove(tmpfn); + /* If either src or dst came from sysrepo, close the session */ + sysrepo_session(NULL); - sync(); /* ensure command is flushed to disk */ + if (rmsrc) + rmtmp(srcpath); + + free(dstpath); + free(srcpath); + + sync(); umask(oldmask); - - return rc; + return err; } static int usage(int rc) @@ -384,30 +571,48 @@ static int usage(int rc) printf("Usage: %s [OPTIONS] SRC DST\n" "\n" "Options:\n" - " -h This help text\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); + " -h This help text\n" + " -n Dry-run, validate configuration without applying\n" + " -s Sanitize paths for CLI use (restrict path traversal)\n" + " -t SEC Timeout for the operation, or default %d sec\n" + " -u USER Username for remote commands, like scp\n" + " -v Show version\n" + "\n" + "Files:\n" + " SRC JSON configuration file, or a datastore\n" + " DST A file or datastore, except factory-config\n" + "\n" + "Datastores:\n" + " running-config The running datastore, current active config\n" + " startup-config The non-volatile config used at startup\n" + " factory-config The device's factory default configuration\n" + "\n", prognm, timeout); return rc; } int main(int argc, char *argv[]) { - const char *user = NULL, *src = NULL, *dst = NULL; + const char *src = NULL, *dst = NULL; int c; timeout = fgetint("/etc/default/confd", "=", "CONFD_TIMEOUT"); - while ((c = getopt(argc, argv, "ht:u:v")) != EOF) { + while ((c = getopt(argc, argv, "hnst:u:v")) != EOF) { switch(c) { case 'h': return usage(0); + case 'n': + dry_run = 1; + break; + case 's': + sanitize = 1; + break; case 't': timeout = atoi(optarg); break; case 'u': - user = optarg; + remote_user = optarg; break; case 'v': puts(PACKAGE_VERSION); @@ -424,5 +629,5 @@ int main(int argc, char *argv[]) src = argv[optind++]; dst = argv[optind++]; - return copy(src, dst, user); + return copy(src, dst); } diff --git a/src/bin/erase.c b/src/bin/erase.c index 716c310b..d78ba1a4 100644 --- a/src/bin/erase.c +++ b/src/bin/erase.c @@ -4,40 +4,38 @@ #include #include #include +#include #include #include #include "util.h" static const char *prognm = "erase"; +static int sanitize; - -static int do_erase(const char *path) +static int do_erase(const char *name) { - char *fn; + char *path; + int rc = 0; - if (access(path, F_OK)) { - size_t len = strlen(path) + 10; - - 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)) - return 0; - - if (remove(fn)) { - fprintf(stderr, ERRMSG "failed removing %s: %s\n", fn, strerror(errno)); - return -1; + path = cfg_adjust(name, NULL, sanitize); + if (!path) { + fprintf(stderr, ERRMSG "file not found.\n"); + rc = 1; + goto out; } - return 0; + if (!yorn("Remove %s, are you sure?", path)) + goto out; + + if (remove(path)) { + fprintf(stderr, ERRMSG "failed removing %s: %s\n", path, strerror(errno)); + rc = 11; + } + +out: + free(path); + return rc; } static int usage(int rc) @@ -46,6 +44,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 +54,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..f922f484 100644 --- a/src/bin/util.c +++ b/src/bin/util.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -67,52 +68,100 @@ int has_ext(const char *fn, const char *ext) return 0; } -static const char *basenm(const char *fn) +int dirlen(const char *path) { - const char *ptr; + const char *slash; - if (!fn) - return ""; + slash = strrchr(path, '/'); + if (slash) + return slash - path; - ptr = strrchr(fn, '/'); - if (!ptr) - ptr = fn; - - return ptr; + return 0; } -char *cfg_adjust(const char *fn, const char *tmpl, char *buf, size_t len) +const char *basenm(const char *path) { - if (strstr(fn, "../")) - return NULL; /* relative paths not allowed */ + const char *slash; - if (fn[0] == '/') { - strlcpy(buf, fn, len); - return buf; /* allow absolute paths */ - } + if (!path) + return NULL; - /* 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); + slash = strrchr(path, '/'); + if (slash) + return slash[1] ? slash + 1 : NULL; - return buf; + return path; +} + +static int path_allowed(const char *path) +{ + const char *accepted[] = { + "/media/", + "/cfg/", + getenv("HOME"), + NULL + }; + + for (int i = 0; accepted[i]; i++) { + if (!strncmp(path, accepted[i], strlen(accepted[i]))) + return 1; } + + return 0; +} + +char *cfg_adjust(const char *path, const char *template, bool sanitize) +{ + char *expanded = NULL, *resolved = NULL; + const char *basename; + int dlen; + + dlen = dirlen(path); + basename = basenm(path) ? : basenm(template); + if (!basename) + goto err; + + if (sanitize) { + if (strstr(path, "../")) + goto err; + + if (path[0] == '/') { + if (!path_allowed(path)) + goto err; + } + } + + if (asprintf(&expanded, "%s%.*s/%s%s", + path[0] == '/' ? "" : "/cfg/", + dlen, path, + basename, + strchr(basename, '.') ? "" : ".cfg") < 0) + goto err; + + if (sanitize) { + resolved = realpath(expanded, NULL); + if (!resolved) { + if (errno == ENOENT) + goto out; + else + goto err; + } + + /* File exists, make sure that the resolved symlink + * still matches the whitelist. + */ + if (!path_allowed(resolved)) + goto err; - /* Files ending with .cfg belong in /cfg */ - if (has_ext(fn, ".cfg")) { - snprintf(buf, len, "/cfg/%s", fn); - return buf; + free(expanded); + expanded = 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); +out: + return expanded; - return buf; +err: + free(resolved); + free(expanded); + return NULL; } diff --git a/src/bin/util.h b/src/bin/util.h index 6c5ded9d..e9a5b6f8 100644 --- a/src/bin/util.h +++ b/src/bin/util.h @@ -1,17 +1,19 @@ /* SPDX-License-Identifier: ISC */ #ifndef BIN_UTIL_H_ #define BIN_UTIL_H_ +#include #include #include #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 *path, const char *template, bool sanitize); #endif /* BIN_UTIL_H_ */ diff --git a/src/klish-plugin-infix/src/infix.c b/src/klish-plugin-infix/src/infix.c index 54bae655..b041da20 100644 --- a/src/klish-plugin-infix/src/infix.c +++ b/src/klish-plugin-infix/src/infix.c @@ -28,7 +28,7 @@ const uint8_t kplugin_infix_major = 1; const uint8_t kplugin_infix_minor = 0; -static void cd_home(kcontext_t *ctx) +static const char *cd_home(kcontext_t *ctx) { const char *user = "root"; ksession_t *session; @@ -43,6 +43,8 @@ static void cd_home(kcontext_t *ctx) pw = getpwnam(user); chdir(pw->pw_dir); + + return user; } static int systemf(const char *fmt, ...) @@ -118,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) @@ -146,8 +148,8 @@ int infix_copy(kcontext_t *ctx) { kpargv_t *pargv = kcontext_pargv(ctx); const char *src, *dst; - const char *username; char user[256] = ""; + char validate[8] = ""; kparg_t *parg; src = kparg_value(kpargv_find(pargv, "src")); @@ -159,26 +161,20 @@ int infix_copy(kcontext_t *ctx) if (parg) snprintf(user, sizeof(user), "-u %s", kparg_value(parg)); - cd_home(ctx); - username = ksession_user(kcontext_session(ctx)); + parg = kpargv_find(pargv, "validate"); + if (parg) + strlcpy(validate, "-n", sizeof(validate)); - return systemf("sudo -u %s copy %s %s %s", username, user, src, dst); + /* Ensure we run the copy command as the logged-in user, not root (klishd) */ + return systemf("doas -u %s copy -s %s %s %s %s", cd_home(ctx), validate, user, src, dst); } int infix_shell(kcontext_t *ctx) { - const char *user = "root"; - ksession_t *session; + const char *user = cd_home(ctx); pid_t pid; int rc; - session = kcontext_session(ctx); - if (session) { - user = ksession_user(session); - if (!user) - user = "root"; - } - pid = fork(); if (pid == -1) return -1; diff --git a/src/klish-plugin-infix/xml/infix.xml b/src/klish-plugin-infix/xml/infix.xml index 8da12d07..52210a2e 100644 --- a/src/klish-plugin-infix/xml/infix.xml +++ b/src/klish-plugin-infix/xml/infix.xml @@ -167,6 +167,7 @@ +