From db42d52a8a1842726eb88e8da326ea37c2edaa27 Mon Sep 17 00:00:00 2001 From: Dominic Cleal Date: Sun, 25 Mar 2012 17:07:54 +0100 Subject: [PATCH] Prevent cross-mountpoint attacks via .augsave during saving Previously Augeas would open PATH.augsave for writing if a rename from PATH to PATH.augsave failed, then write the file contents in. Now if the rename fails, it tries to unlink PATH.augsave and open it with O_EXCL first. Mountpoints remain permitted at either PATH or PATH.augnew provided /augeas/save/copy_if_rename_fails exists. * src/transform.c (clone_file): add argument to perform unlink and O_EXCL on destination filename after a rename failure to prevent PATH.augsave being a mountpoint * src/transform.c (transform_save, remove_file): always try to unlink PATH.augsave if rename fails, only allowing PATH to be a mountpoint; allow PATH or PATH.augnew to be mountpoints * tests/ test-put-mount: check PATH being a mountpoint is supported test-put-mount-augnew.sh: check PATH.augnew being a mountpoint is supported test-put-mount-augsave.sh: check unlink error when PATH.augsave is a mount Fixes BZ 772261 (cherry picked from commit b8de6a8c5cfffb007149036ffa561ced4d11c462) --- src/transform.c | 40 ++++++++++++++++++++---- tests/Makefile.am | 5 +-- tests/test-put-mount-augnew.sh | 69 +++++++++++++++++++++++++++++++++++++++++ tests/test-put-mount-augsave.sh | 62 ++++++++++++++++++++++++++++++++++++ tests/test-put-mount.sh | 55 ++++++++++++++++++++++++++++++++ 5 files changed, 223 insertions(+), 8 deletions(-) create mode 100755 tests/test-put-mount-augnew.sh create mode 100755 tests/test-put-mount-augsave.sh create mode 100755 tests/test-put-mount.sh diff --git a/src/transform.c b/src/transform.c index 0936eb7..00ca3ec 100644 --- a/src/transform.c +++ b/src/transform.c @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -784,14 +785,21 @@ static int transfer_file_attrs(FILE *from, FILE *to, * means that FROM or TO is a bindmounted file), and COPY_IF_RENAME_FAILS * is true, copy the contents of FROM into TO and delete FROM. * + * If COPY_IF_RENAME_FAILS and UNLINK_IF_RENAME_FAILS are true, and the above + * copy mechanism is used, it will unlink the TO path and open with O_EXCL + * to ensure we only copy *from* a bind mount rather than into an attacker's + * mount placed at TO (e.g. for .augsave). + * * Return 0 on success (either rename succeeded or we copied the contents * over successfully), -1 on failure. */ static int clone_file(const char *from, const char *to, - const char **err_status, int copy_if_rename_fails) { + const char **err_status, int copy_if_rename_fails, + int unlink_if_rename_fails) { FILE *from_fp = NULL, *to_fp = NULL; char buf[BUFSIZ]; size_t len; + int to_fd = -1, to_oflags, r; int result = -1; if (rename(from, to) == 0) @@ -808,10 +816,23 @@ static int clone_file(const char *from, const char *to, goto done; } - if (!(to_fp = fopen(to, "w"))) { + if (unlink_if_rename_fails) { + r = unlink(to); + if (r < 0) { + *err_status = "clone_unlink_dst"; + goto done; + } + } + + to_oflags = unlink_if_rename_fails ? O_EXCL : O_TRUNC; + if ((to_fd = open(to, O_WRONLY|O_CREAT|to_oflags, S_IRUSR|S_IWUSR)) < 0) { *err_status = "clone_open_dst"; goto done; } + if (!(to_fp = fdopen(to_fd, "w"))) { + *err_status = "clone_fdopen_dst"; + goto done; + } if (transfer_file_attrs(from_fp, to_fp, err_status) < 0) goto done; @@ -838,8 +859,15 @@ static int clone_file(const char *from, const char *to, done: if (from_fp != NULL) fclose(from_fp); - if (to_fp != NULL && fclose(to_fp) != 0) + if (to_fp != NULL) { + if (fclose(to_fp) != 0) { + *err_status = "clone_fclose_dst"; + result = -1; + } + } else if (to_fd >= 0 && close(to_fd) < 0) { + *err_status = "clone_close_dst"; result = -1; + } if (result != 0) unlink(to); if (result == 0) @@ -1072,7 +1100,7 @@ int transform_save(struct augeas *aug, struct tree *xfm, goto done; } - r = clone_file(augorig_canon, augsave, &err_status, 1); + r = clone_file(augorig_canon, augsave, &err_status, 1, 1); if (r != 0) { dyn_err_status = strappend(err_status, "_augsave"); goto done; @@ -1080,7 +1108,7 @@ int transform_save(struct augeas *aug, struct tree *xfm, } } - r = clone_file(augtemp, augdest, &err_status, copy_if_rename_fails); + r = clone_file(augtemp, augdest, &err_status, copy_if_rename_fails, 0); if (r != 0) { dyn_err_status = strappend(err_status, "_augtemp"); goto done; @@ -1171,7 +1199,7 @@ int remove_file(struct augeas *aug, struct tree *tree) { goto error; } - r = clone_file(augorig_canon, augsave, &err_status, 1); + r = clone_file(augorig_canon, augsave, &err_status, 1, 1); if (r != 0) { dyn_err_status = strappend(err_status, "_augsave"); goto error; diff --git a/tests/Makefile.am b/tests/Makefile.am index 3af1a0e..69c3c89 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -152,8 +152,9 @@ check_SCRIPTS = \ $(lens_tests) \ test-get.sh test-augtool.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-put-symlink-augsave.sh test-put-symlink-augtemp.sh \ + test-put-mount.sh test-put-mount-augnew.sh test-put-mount-augsave.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-mount-augnew.sh b/tests/test-put-mount-augnew.sh new file mode 100755 index 0000000..cb95bda --- /dev/null +++ b/tests/test-put-mount-augnew.sh @@ -0,0 +1,69 @@ +#! /bin/bash + +# Test that we can write into a bind mount placed at PATH.augnew with the +# copy_if_rename_fails flag. +# This requires that EXDEV or EBUSY is returned from rename(2) to activate the +# code path, so set up a bind mount on Linux. + +if [ $UID -ne 0 -o "$(uname -s)" != "Linux" ]; then + echo "Test can only be run as root on Linux to create bind mounts" + exit 77 +fi + +ROOT=$abs_top_builddir/build/test-put-mount-augnew +LENSES=$abs_top_srcdir/lenses + +HOSTS=$ROOT/etc/hosts +HOSTS_AUGNEW=${HOSTS}.augnew +TARGET=$ROOT/other/real_hosts + +rm -rf $ROOT +mkdir -p $(dirname $HOSTS) +mkdir -p $(dirname $TARGET) + +echo 127.0.0.1 localhost > $HOSTS +touch $TARGET $HOSTS_AUGNEW + +mount --bind $TARGET $HOSTS_AUGNEW +Exit() { + umount $HOSTS_AUGNEW + exit $1 +} + +HOSTS_SUM=$(sum $HOSTS) + +augtool --nostdinc -I $LENSES -r $ROOT --new </dev/null; then + echo "/other/real_hosts does not contain the modification" + Exit 1 +fi + +Exit 0 diff --git a/tests/test-put-mount-augsave.sh b/tests/test-put-mount-augsave.sh new file mode 100755 index 0000000..31fcfca --- /dev/null +++ b/tests/test-put-mount-augsave.sh @@ -0,0 +1,62 @@ +#! /bin/bash + +# Test that we don't follow bind mounts when writing to .augsave. +# This requires that EXDEV or EBUSY is returned from rename(2) to activate the +# code path, so set up a bind mount on Linux. + +if [ $UID -ne 0 -o "$(uname -s)" != "Linux" ]; then + echo "Test can only be run as root on Linux to create bind mounts" + exit 77 +fi + +actual() { + (augtool --nostdinc -I $LENSES -r $ROOT --backup | grep ^/augeas) < $HOSTS +touch $ATTACK_FILE $HOSTS_AUGSAVE + +mount --bind $ATTACK_FILE $HOSTS_AUGSAVE +Exit() { + umount $HOSTS_AUGSAVE + exit $1 +} + +ACTUAL=$(actual) +EXPECTED=$(expected) +if [ "$ACTUAL" != "$EXPECTED" ]; then + echo "No error when trying to unlink augsave (a bind mount):" + echo "$ACTUAL" + exit 1 +fi + +if [ -s $ATTACK_FILE ]; then + echo "/other/attack now contains data, should be blank" + Exit 1 +fi + +Exit 0 diff --git a/tests/test-put-mount.sh b/tests/test-put-mount.sh new file mode 100755 index 0000000..210bc10 --- /dev/null +++ b/tests/test-put-mount.sh @@ -0,0 +1,55 @@ +#! /bin/bash + +# Test that we can write into a bind mount with the copy_if_rename_fails flag. +# This requires that EXDEV or EBUSY is returned from rename(2) to activate the +# code path, so set up a bind mount on Linux. + +if [ $UID -ne 0 -o "$(uname -s)" != "Linux" ]; then + echo "Test can only be run as root on Linux to create bind mounts" + exit 77 +fi + +ROOT=$abs_top_builddir/build/test-put-mount +LENSES=$abs_top_srcdir/lenses + +HOSTS=$ROOT/etc/hosts +TARGET=$ROOT/other/real_hosts + +rm -rf $ROOT +mkdir -p $(dirname $HOSTS) +mkdir -p $(dirname $TARGET) + +echo 127.0.0.1 localhost > $TARGET +touch $HOSTS + +mount --bind $TARGET $HOSTS +Exit() { + umount $HOSTS + exit $1 +} + +HOSTS_SUM=$(sum $HOSTS) + +augtool --nostdinc -I $LENSES -r $ROOT </dev/null; then + echo "/other/real_hosts does not contain the modification" + Exit 1 +fi + +Exit 0 -- 1.8.0