Attachment 572544 Details for Bug 772261 – proposed upstream fix #1

[patch] proposed upstream fix #1

772261_proposed_1.patch (text/plain), 10.72 KB, created by Dominic Cleal on 2012-03-25 16:19:09 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Dominic Cleal

Created: 2012-03-25 16:19:09 UTC

Size: 10.72 KB

patch

obsolete

>From 61a8fa10d2cff0810b6dba68f4d2b9d49abc021c Mon Sep 17 00:00:00 2001
>From: Dominic Cleal <dcleal@redhat.com>
>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
>---
> 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 2b05dcc..23ac4ed 100644
>--- a/src/transform.c
>+++ b/src/transform.c
>@@ -27,6 +27,7 @@
> 
> #include <sys/types.h>
> #include <sys/stat.h>
>+#include <fcntl.h>
> #include <unistd.h>
> #include <selinux/selinux.h>
> #include <stdbool.h>
>@@ -787,14 +788,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)
>@@ -811,10 +819,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;
>@@ -841,8 +862,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)
>@@ -1075,7 +1103,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;
>@@ -1083,7 +1111,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;
>@@ -1174,7 +1202,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 3fcf72a..86107c6 100644
>--- a/tests/Makefile.am
>+++ b/tests/Makefile.am
>@@ -172,8 +172,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 <<EOF
>+set /augeas/save/copy_if_rename_fails 1
>+set /files/etc/hosts/1/alias myhost
>+save
>+print /augeas//error
>+EOF
>+
>+if [ ! -f $HOSTS ] ; then
>+    echo "/etc/hosts is no longer a regular file"
>+    Exit 1
>+fi
>+if [ ! "x${HOSTS_SUM}" = "x$(sum $HOSTS)" ]; then
>+    echo "/etc/hosts has changed"
>+    Exit 1
>+fi
>+if [ ! "x${HOSTS_SUM}" = "x$(sum $HOSTS)" ]; then
>+    echo "/etc/hosts has changed"
>+    Exit 1
>+fi
>+
>+if [ ! -s $HOSTS_AUGNEW ]; then
>+    echo "/etc/hosts.augnew is empty"
>+    Exit 1
>+fi
>+if [ ! -s $TARGET ]; then
>+    echo "/other/real_hosts is empty"
>+    Exit 1
>+fi
>+
>+if ! grep myhost $TARGET >/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) <<EOF
>+    set /augeas/save/copy_if_rename_fails 1
>+    set /files/etc/hosts/1/alias myhost
>+    save
>+    print /augeas//error
>+EOF
>+}
>+
>+expected() {
>+    cat <<EOF
>+/augeas/files/etc/hosts/error = "clone_unlink_dst_augsave"
>+/augeas/files/etc/hosts/error/message = "Device or resource busy"
>+EOF
>+}
>+
>+ROOT=$abs_top_builddir/build/test-put-mount-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)
>+
>+echo 127.0.0.1 localhost > $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 <<EOF
>+set /augeas/save/copy_if_rename_fails 1
>+set /files/etc/hosts/1/alias myhost
>+save
>+print /augeas//error
>+EOF
>+
>+if [ ! "x${HOSTS_SUM}" != "x$(sum $HOSTS)" ]; then
>+    echo "/etc/hosts hasn't changed"
>+    Exit 1
>+fi
>+
>+if [ ! "x${HOSTS_SUM}" != "x$(sum $TARGET)" ]; then
>+    echo "/other/real_hosts hasn't changed"
>+    Exit 1
>+fi
>+
>+if ! grep myhost $TARGET >/dev/null; then
>+    echo "/other/real_hosts does not contain the modification"
>+    Exit 1
>+fi
>+
>+Exit 0
>-- 
>1.7.7.6
>

Actions: View | Diff

Attachments on bug 772261: 572544