summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDominic Cleal <dcleal@redhat.com>2012-02-12 21:41:37 (GMT)
committerDavid Lutterkort <lutter@redhat.com>2012-07-19 18:20:06 (GMT)
commit163877441402e464266b45ee98e24bb652971def (patch)
tree69dc1dce89a491e8172ed5b854cb50e7bec7a60d
parent730cdda472417566ff9c99336293ae6e240467c5 (diff)
downloadaugeas-16387744.zip
augeas-16387744.tar.gz
augeas-16387744.tar.xz
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
-rwxr-xr-xbootstrap1
-rw-r--r--src/internal.c15
-rw-r--r--src/internal.h3
-rw-r--r--src/transform.c141
-rw-r--r--tests/Makefile.am3
-rwxr-xr-xtests/test-put-symlink-augnew.sh52
-rwxr-xr-xtests/test-put-symlink-augsave.sh52
-rwxr-xr-xtests/test-put-symlink-augtemp.sh52
-rwxr-xr-xtests/test-put-symlink.sh5
-rwxr-xr-xtests/test-save-empty.sh5
10 files changed, 270 insertions, 59 deletions
diff --git a/bootstrap b/bootstrap
index 4240dab..22e6460 100755
--- a/bootstrap
+++ b/bootstrap
@@ -72,6 +72,7 @@ gitlog-to-changelog
canonicalize-lgpl
isblank
locale
+mkstemp
regex
safe-alloc
selinux-h
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 064c5d1..933722c 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -287,6 +287,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 39bfa50..6ebcbd6 100644
--- a/src/transform.c
+++ b/src/transform.c
@@ -799,35 +799,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;
}
@@ -869,7 +872,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) {
@@ -946,19 +949,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;
@@ -986,19 +1008,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) {
@@ -1011,31 +1020,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;
}
@@ -1045,22 +1076,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;
}
@@ -1069,33 +1100,33 @@ int transform_save(struct augeas *aug, struct tree *xfm,
if (err != NULL) {
err_status = err->pos >= 0 ? "parse_skel_failed" : "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;
@@ -1107,13 +1138,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:
@@ -1138,6 +1170,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);
@@ -1147,6 +1180,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 d853777..f664537 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -183,7 +183,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