From 55048a57d6f0e5603fbf7f1e45e287df4c2a2c07 Mon Sep 17 00:00:00 2001 From: Dominic Cleal Date: Sun, 12 Feb 2012 21:41:37 +0000 Subject: [PATCH] Prevent symlink attacks via .augnew during saving Instead of saving into a predictable PATH.augnew file, save into a securely created PATH.augnew.XXXXXX * src/transform.c (transform_save): write changes to a temporary file in the same directory as the destination (either the file's canonical path or the path of .augnew), before renaming * src/transform.c (transfer_file_attrs): use fchown, fchmod etc. on the same file handles to ensure consistent permission changes * bootstrap: add mkstemp gnulib module * tests/ test-put-symlink-augnew.sh: test symlink attack when writing .augnew test-put-symlink-augsave.sh: test symlink attack when writing .augsave test-put-symlink-augtemp.sh: test symlink attack via temp .augnew test-put-symlink.sh: also test file modification Fixes BZ 772257 Conflicts: src/transform.c --- bootstrap | 1 + src/internal.c | 15 +++- src/internal.h | 3 + src/transform.c | 141 ++++++++++++++++++++++++-------------- tests/Makefile.am | 3 +- tests/test-put-symlink-augnew.sh | 52 ++++++++++++++ tests/test-put-symlink-augsave.sh | 52 ++++++++++++++ tests/test-put-symlink-augtemp.sh | 52 ++++++++++++++ tests/test-put-symlink.sh | 5 ++ tests/test-save-empty.sh | 5 +- 10 files changed, 270 insertions(+), 59 deletions(-) create mode 100755 tests/test-put-symlink-augnew.sh create mode 100755 tests/test-put-symlink-augsave.sh create mode 100755 tests/test-put-symlink-augtemp.sh diff --git a/src/internal.c b/src/internal.c index 566498d..44956f8 100644 --- a/src/internal.c +++ b/src/internal.c @@ -125,8 +125,7 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) return NULL; } -char* xread_file(const char *path) { - FILE *fp = fopen(path, "r"); +char* xfread_file(FILE *fp) { char *result; size_t len; @@ -134,7 +133,6 @@ char* xread_file(const char *path) { return NULL; result = fread_file_lim(fp, MAX_READ_LEN, &len); - fclose (fp); if (result != NULL && len <= MAX_READ_LEN @@ -145,6 +143,17 @@ char* xread_file(const char *path) { return NULL; } +char* xread_file(const char *path) { + FILE *fp; + char *result; + + fp = fopen(path, "r"); + result = xfread_file(fp); + fclose (fp); + + return result; +} + /* * Escape/unescape of string literals */ diff --git a/src/internal.h b/src/internal.h index 7317842..a77ad28 100644 --- a/src/internal.h +++ b/src/internal.h @@ -283,6 +283,9 @@ char *format_pos(const char *text, int pos); */ char* xread_file(const char *path); +/* Like xread_file, but caller supplies a file pointer */ +char* xfread_file(FILE *fp); + /* Get the error message for ERRNUM in a threadsafe way. Based on libvirt's * virStrError */ diff --git a/src/transform.c b/src/transform.c index b36b944..0936eb7 100644 --- a/src/transform.c +++ b/src/transform.c @@ -739,35 +739,38 @@ int transform_applies(struct tree *xfm, const char *path) { return filter_matches(xfm, path + strlen(AUGEAS_FILES_TREE)); } -static int transfer_file_attrs(const char *from, const char *to, +static int transfer_file_attrs(FILE *from, FILE *to, const char **err_status) { struct stat st; int ret = 0; int selinux_enabled = (is_selinux_enabled() > 0); security_context_t con = NULL; - ret = lstat(from, &st); + int from_fd = fileno(from); + int to_fd = fileno(to); + + ret = fstat(from_fd, &st); if (ret < 0) { *err_status = "replace_stat"; return -1; } if (selinux_enabled) { - if (lgetfilecon(from, &con) < 0 && errno != ENOTSUP) { + if (fgetfilecon(from_fd, &con) < 0 && errno != ENOTSUP) { *err_status = "replace_getfilecon"; return -1; } } - if (lchown(to, st.st_uid, st.st_gid) < 0) { + if (fchown(to_fd, st.st_uid, st.st_gid) < 0) { *err_status = "replace_chown"; return -1; } - if (chmod(to, st.st_mode) < 0) { + if (fchmod(to_fd, st.st_mode) < 0) { *err_status = "replace_chmod"; return -1; } if (selinux_enabled && con != NULL) { - if (lsetfilecon(to, con) < 0 && errno != ENOTSUP) { + if (fsetfilecon(to_fd, con) < 0 && errno != ENOTSUP) { *err_status = "replace_setfilecon"; return -1; } @@ -810,7 +813,7 @@ static int clone_file(const char *from, const char *to, goto done; } - if (transfer_file_attrs(from, to, err_status) < 0) + if (transfer_file_attrs(from_fp, to_fp, err_status) < 0) goto done; while ((len = fread(buf, 1, BUFSIZ, from_fp)) > 0) { @@ -887,19 +890,38 @@ static int file_saved_event(struct augeas *aug, const char *path) { * are noted in the /augeas/files hierarchy in AUG->ORIGIN under * PATH/error. * - * Writing the file happens by first writing into PATH.augnew, transferring - * all file attributes of PATH to PATH.augnew, and then renaming - * PATH.augnew to PATH. If the rename fails, and the entry - * AUGEAS_COPY_IF_FAILURE exists in AUG->ORIGIN, PATH is overwritten by - * copying file contents + * Writing the file happens by first writing into a temp file, transferring all + * file attributes of PATH to the temp file, and then renaming the temp file + * back to PATH. + * + * Temp files are created alongside the destination file to enable the rename, + * which may be the canonical path (PATH_canon) if PATH is a symlink. + * + * If the AUG_SAVE_NEWFILE flag is set, instead rename to PATH.augnew rather + * than PATH. If AUG_SAVE_BACKUP is set, move the original to PATH.augsave. + * (Always PATH.aug{new,save} irrespective of whether PATH is a symlink.) + * + * If the rename fails, and the entry AUGEAS_COPY_IF_FAILURE exists in + * AUG->ORIGIN, PATH is instead overwritten by copying file contents. + * + * The table below shows the locations for each permutation. + * + * PATH save flag temp file dest file backup? + * regular - PATH.augnew.XXXX PATH - + * regular BACKUP PATH.augnew.XXXX PATH PATH.augsave + * regular NEWFILE PATH.augnew.XXXX PATH.augnew - + * symlink - PATH_canon.XXXX PATH_canon - + * symlink BACKUP PATH_canon.XXXX PATH_canon PATH.augsave + * symlink NEWFILE PATH.augnew.XXXX PATH.augnew - * * Return 0 on success, -1 on failure. */ int transform_save(struct augeas *aug, struct tree *xfm, const char *path, struct tree *tree) { - FILE *fp = NULL; - char *augnew = NULL, *augorig = NULL, *augsave = NULL; - char *augorig_canon = NULL; + int fd; + FILE *fp = NULL, *augorig_canon_fp = NULL; + char *augtemp = NULL, *augnew = NULL, *augorig = NULL, *augsave = NULL; + char *augorig_canon = NULL, *augdest = NULL; int augorig_exists; int copy_if_rename_fails = 0; char *text = NULL; @@ -926,19 +948,6 @@ int transform_save(struct augeas *aug, struct tree *xfm, goto done; } - if (access(augorig, R_OK) == 0) { - text = xread_file(augorig); - } else { - text = strdup(""); - } - - if (text == NULL) { - err_status = "put_read"; - goto done; - } - - text = append_newline(text, strlen(text)); - augorig_canon = canonicalize_file_name(augorig); augorig_exists = 1; if (augorig_canon == NULL) { @@ -951,31 +960,53 @@ int transform_save(struct augeas *aug, struct tree *xfm, } } - /* Figure out where to put the .augnew file. If we need to rename it - later on, put it next to augorig_canon */ + if (access(augorig_canon, R_OK) == 0) { + augorig_canon_fp = fopen(augorig_canon, "r"); + text = xfread_file(augorig_canon_fp); + } else { + text = strdup(""); + } + + if (text == NULL) { + err_status = "put_read"; + goto done; + } + + text = append_newline(text, strlen(text)); + + /* Figure out where to put the .augnew and temp file. If no .augnew file + then put the temp file next to augorig_canon, else next to .augnew. */ if (aug->flags & AUG_SAVE_NEWFILE) { if (xasprintf(&augnew, "%s" EXT_AUGNEW, augorig) < 0) { err_status = "augnew_oom"; goto done; } + augdest = augnew; } else { - if (xasprintf(&augnew, "%s" EXT_AUGNEW, augorig_canon) < 0) { - err_status = "augnew_oom"; - goto done; - } + augdest = augorig_canon; + } + + if (xasprintf(&augtemp, "%s.XXXXXX", augdest) < 0) { + err_status = "augtemp_oom"; + goto done; } // FIXME: We might have to create intermediate directories // to be able to write augnew, but we have no idea what permissions // etc. they should get. Just the process default ? - fp = fopen(augnew, "w"); + fd = mkstemp(augtemp); + if (fd < 0) { + err_status = "mk_augtemp"; + goto done; + } + fp = fdopen(fd, "w"); if (fp == NULL) { - err_status = "open_augnew"; + err_status = "open_augtemp"; goto done; } if (augorig_exists) { - if (transfer_file_attrs(augorig_canon, augnew, &err_status) != 0) { + if (transfer_file_attrs(augorig_canon_fp, fp, &err_status) != 0) { err_status = "xfer_attrs"; goto done; } @@ -985,22 +1016,22 @@ int transform_save(struct augeas *aug, struct tree *xfm, lns_put(fp, lens, tree->children, text, &err); if (ferror(fp)) { - err_status = "error_augnew"; + err_status = "error_augtemp"; goto done; } if (fflush(fp) != 0) { - err_status = "flush_augnew"; + err_status = "flush_augtemp"; goto done; } if (fsync(fileno(fp)) < 0) { - err_status = "sync_augnew"; + err_status = "sync_augtemp"; goto done; } if (fclose(fp) != 0) { - err_status = "close_augnew"; + err_status = "close_augtemp"; fp = NULL; goto done; } @@ -1009,33 +1040,33 @@ int transform_save(struct augeas *aug, struct tree *xfm, if (err != NULL) { err_status = "put_failed"; - unlink(augnew); + unlink(augtemp); goto done; } { - char *new_text = xread_file(augnew); + char *new_text = xread_file(augtemp); int same = 0; if (new_text == NULL) { - err_status = "read_augnew"; + err_status = "read_augtemp"; goto done; } same = STREQ(text, new_text); FREE(new_text); if (same) { result = 0; - unlink(augnew); + unlink(augtemp); goto done; } else if (aug->flags & AUG_SAVE_NOOP) { result = 1; - unlink(augnew); + unlink(augtemp); goto done; } } if (!(aug->flags & AUG_SAVE_NEWFILE)) { if (augorig_exists && (aug->flags & AUG_SAVE_BACKUP)) { - r = asprintf(&augsave, "%s%s" EXT_AUGSAVE, aug->root, filename); + r = xasprintf(&augsave, "%s" EXT_AUGSAVE, augorig); if (r == -1) { augsave = NULL; goto done; @@ -1047,13 +1078,14 @@ int transform_save(struct augeas *aug, struct tree *xfm, goto done; } } - r = clone_file(augnew, augorig_canon, &err_status, - copy_if_rename_fails); - if (r != 0) { - dyn_err_status = strappend(err_status, "_augnew"); - goto done; - } } + + r = clone_file(augtemp, augdest, &err_status, copy_if_rename_fails); + if (r != 0) { + dyn_err_status = strappend(err_status, "_augtemp"); + goto done; + } + result = 1; done: @@ -1077,6 +1109,7 @@ int transform_save(struct augeas *aug, struct tree *xfm, free(dyn_err_status); lens_release(lens); free(text); + free(augtemp); free(augnew); if (augorig_canon != augorig) free(augorig_canon); @@ -1086,6 +1119,8 @@ int transform_save(struct augeas *aug, struct tree *xfm, if (fp != NULL) fclose(fp); + if (augorig_canon_fp != NULL) + fclose(augorig_canon_fp); return result; } diff --git a/tests/Makefile.am b/tests/Makefile.am index 817f368..3af1a0e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -151,7 +151,8 @@ check_SCRIPTS = \ test-interpreter.sh \ $(lens_tests) \ test-get.sh test-augtool.sh \ - test-put-symlink.sh test-save-empty.sh \ + test-put-symlink.sh test-put-symlink-augnew.sh \ + test-put-symlink-augsave.sh test-put-symlink-augtemp.sh test-save-empty.sh \ test-bug-1.sh test-idempotent.sh test-preserve.sh \ test-events-saved.sh test-save-mode.sh test-unlink-error.sh \ test-augtool-empty-line.sh test-augtool-modify-root.sh diff --git a/tests/test-put-symlink-augnew.sh b/tests/test-put-symlink-augnew.sh new file mode 100755 index 0000000..eb361df --- /dev/null +++ b/tests/test-put-symlink-augnew.sh @@ -0,0 +1,52 @@ +#! /bin/bash + +# Test that we don't follow symlinks when writing to .augnew + +ROOT=$abs_top_builddir/build/test-put-symlink-augnew +LENSES=$abs_top_srcdir/lenses + +HOSTS=$ROOT/etc/hosts +HOSTS_AUGNEW=${HOSTS}.augnew + +ATTACK_FILE=$ROOT/other/attack + +rm -rf $ROOT +mkdir -p $(dirname $HOSTS) +mkdir -p $(dirname $ATTACK_FILE) + +cat < $HOSTS +127.0.0.1 localhost +EOF +touch $ATTACK_FILE + +(cd $(dirname $HOSTS) && ln -s ../other/attack $(basename $HOSTS).augnew) + +HOSTS_SUM=$(sum $HOSTS) + +augtool --nostdinc -I $LENSES -r $ROOT --new > /dev/null </dev/null; then + echo "/etc/hosts does not contain the modification" + exit 1 +fi + +if [ -s $ATTACK_FILE ]; then + echo "/other/attack now contains data, should be blank" + exit 1 +fi diff --git a/tests/test-put-symlink-augsave.sh b/tests/test-put-symlink-augsave.sh new file mode 100755 index 0000000..8b4dbcf --- /dev/null +++ b/tests/test-put-symlink-augsave.sh @@ -0,0 +1,52 @@ +#! /bin/bash + +# Test that we don't follow .augsave symlinks + +ROOT=$abs_top_builddir/build/test-put-symlink-augsave +LENSES=$abs_top_srcdir/lenses + +HOSTS=$ROOT/etc/hosts +HOSTS_AUGSAVE=${HOSTS}.augsave + +ATTACK_FILE=$ROOT/other/attack + +rm -rf $ROOT +mkdir -p $(dirname $HOSTS) +mkdir -p $(dirname $ATTACK_FILE) + +cat < $HOSTS +127.0.0.1 localhost +EOF +HOSTS_SUM=$(sum $HOSTS) + +touch $ATTACK_FILE +(cd $(dirname $HOSTS) && ln -s ../other/attack $(basename $HOSTS).augsave) + +# Now ask for the original to be saved in .augsave +augtool --nostdinc -I $LENSES -r $ROOT --backup > /dev/null </dev/null; then + echo "/etc/hosts does not contain the modification" + exit 1 +fi + +if [ -s $ATTACK_FILE ]; then + echo "/other/attack now contains data, should be blank" + exit 1 +fi diff --git a/tests/test-put-symlink-augtemp.sh b/tests/test-put-symlink-augtemp.sh new file mode 100755 index 0000000..0076e64 --- /dev/null +++ b/tests/test-put-symlink-augtemp.sh @@ -0,0 +1,52 @@ +#! /bin/bash + +# Test that we don't follow .augnew symlinks (regression test) + +ROOT=$abs_top_builddir/build/test-put-symlink-augtemp +LENSES=$abs_top_srcdir/lenses + +HOSTS=$ROOT/etc/hosts +HOSTS_AUGNEW=${HOSTS}.augnew + +ATTACK_FILE=$ROOT/other/attack + +rm -rf $ROOT +mkdir -p $(dirname $HOSTS) +mkdir -p $(dirname $ATTACK_FILE) + +cat < $HOSTS +127.0.0.1 localhost +EOF +touch $ATTACK_FILE + +(cd $(dirname $HOSTS) && ln -s ../other/attack $(basename $HOSTS).augnew) + +# Test the normal save code path which would use a temp augnew file +augtool --nostdinc -I $LENSES -r $ROOT > /dev/null </dev/null; then + echo "/etc/hosts does not contain the modification" + exit 1 +fi + +if [ ! -h $HOSTS_AUGNEW ] ; then + echo "/etc/hosts.augnew is not a symbolic link" + exit 1 +fi +LINK=$(readlink $HOSTS_AUGNEW) +if [ "x$LINK" != "x../other/attack" ] ; then + echo "/etc/hosts.augnew no longer links to ../other/attack" + exit 1 +fi + +if [ -s $ATTACK_FILE ]; then + echo "/other/attack now contains data, should be blank" + exit 1 +fi diff --git a/tests/test-put-symlink.sh b/tests/test-put-symlink.sh index 6996b23..64749ea 100755 --- a/tests/test-put-symlink.sh +++ b/tests/test-put-symlink.sh @@ -41,3 +41,8 @@ if [ "x$LINK" != "x../other/hosts" ] ; then echo "/etc/hosts does not link to ../other/hosts" exit 1 fi + +if ! grep myhost $REAL_HOSTS >/dev/null; then + echo "/other/hosts does not contain the modification" + exit 1 +fi diff --git a/tests/test-save-empty.sh b/tests/test-save-empty.sh index 00741da..847fd69 100755 --- a/tests/test-save-empty.sh +++ b/tests/test-save-empty.sh @@ -15,7 +15,7 @@ EOF expected_errors() { cat <