path: root/main/augeas/CVE-2012-0786.patch

From 55048a57d6f0e5603fbf7f1e45e287df4c2a2c07 Mon Sep 17 00:00:00 2001
From: Dominic Cleal <dcleal@redhat.com>
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 <<EOF > $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 <<EOF
+set /files/etc/hosts/1/alias myhost
+save
+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 [ ! -f $HOSTS_AUGNEW ] ; then
+    echo "/etc/hosts.augnew is still a symlink, should be unlinked"
+    exit 1
+fi
+if ! grep myhost $HOSTS_AUGNEW >/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 <<EOF > $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 <<EOF
+set /files/etc/hosts/1/alias myhost
+save
+EOF
+
+if [ ! -f $HOSTS ] ; then
+    echo "/etc/hosts is no longer a regular file"
+    exit 1
+fi
+if [ ! -f $HOSTS_AUGNEW ] ; then
+    echo "/etc/hosts.augsave is still a symlink, should be unlinked"
+    exit 1
+fi
+
+if [ ! "x${HOSTS_SUM}" = "x$(sum $HOSTS_AUGSAVE)" ]; then
+    echo "/etc/hosts.augsave has changed from the original /etc/hosts"
+    exit 1
+fi
+if ! grep myhost $HOSTS >/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 <<EOF > $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 <<EOF
+set /files/etc/hosts/1/alias myhost1
+save
+EOF
+
+if [ -h $HOSTS ] ; then
+    echo "/etc/hosts is now a symlink, pointing to" $(readlink $HOSTS)
+    exit 1
+fi
+if ! grep myhost1 $HOSTS >/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 <<EOF
-/augeas/files/etc/hosts/error = "open_augnew"
+/augeas/files/etc/hosts/error = "mk_augtemp"
 /augeas/files/etc/hosts/error/message = "No such file or directory"
 EOF
 }
@@ -30,7 +30,8 @@ EXPECTED=$(expected_errors)
 
 if [ "$ACTUAL" != "$EXPECTED" ]
 then
-    echo "No error on missing /etc directory"
+    echo "No error on missing /etc directory:"
+    echo "$ACTUAL"
     exit 1
 fi
 
-- 
1.8.0