Skip to content

Commit b8de6a8

Browse files
Dominic ClealDavid Lutterkort
Dominic Cleal
authored and
David Lutterkort
committed
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
1 parent 1638774 commit b8de6a8

File tree

5 files changed

+223
-8
lines changed

5 files changed

+223
-8
lines changed

Diff for: src/transform.c

+34-6
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
#include <sys/types.h>
2929
#include <sys/stat.h>
30+
#include <fcntl.h>
3031
#include <unistd.h>
3132
#include <selinux/selinux.h>
3233
#include <stdbool.h>
@@ -844,14 +845,21 @@ static int transfer_file_attrs(FILE *from, FILE *to,
844845
* means that FROM or TO is a bindmounted file), and COPY_IF_RENAME_FAILS
845846
* is true, copy the contents of FROM into TO and delete FROM.
846847
*
848+
* If COPY_IF_RENAME_FAILS and UNLINK_IF_RENAME_FAILS are true, and the above
849+
* copy mechanism is used, it will unlink the TO path and open with O_EXCL
850+
* to ensure we only copy *from* a bind mount rather than into an attacker's
851+
* mount placed at TO (e.g. for .augsave).
852+
*
847853
* Return 0 on success (either rename succeeded or we copied the contents
848854
* over successfully), -1 on failure.
849855
*/
850856
static int clone_file(const char *from, const char *to,
851-
const char **err_status, int copy_if_rename_fails) {
857+
const char **err_status, int copy_if_rename_fails,
858+
int unlink_if_rename_fails) {
852859
FILE *from_fp = NULL, *to_fp = NULL;
853860
char buf[BUFSIZ];
854861
size_t len;
862+
int to_fd = -1, to_oflags, r;
855863
int result = -1;
856864

857865
if (rename(from, to) == 0)
@@ -867,10 +875,23 @@ static int clone_file(const char *from, const char *to,
867875
goto done;
868876
}
869877

870-
if (!(to_fp = fopen(to, "w"))) {
878+
if (unlink_if_rename_fails) {
879+
r = unlink(to);
880+
if (r < 0) {
881+
*err_status = "clone_unlink_dst";
882+
goto done;
883+
}
884+
}
885+
886+
to_oflags = unlink_if_rename_fails ? O_EXCL : O_TRUNC;
887+
if ((to_fd = open(to, O_WRONLY|O_CREAT|to_oflags, S_IRUSR|S_IWUSR)) < 0) {
871888
*err_status = "clone_open_dst";
872889
goto done;
873890
}
891+
if (!(to_fp = fdopen(to_fd, "w"))) {
892+
*err_status = "clone_fdopen_dst";
893+
goto done;
894+
}
874895

875896
if (transfer_file_attrs(from_fp, to_fp, err_status) < 0)
876897
goto done;
@@ -897,8 +918,15 @@ static int clone_file(const char *from, const char *to,
897918
done:
898919
if (from_fp != NULL)
899920
fclose(from_fp);
900-
if (to_fp != NULL && fclose(to_fp) != 0)
921+
if (to_fp != NULL) {
922+
if (fclose(to_fp) != 0) {
923+
*err_status = "clone_fclose_dst";
924+
result = -1;
925+
}
926+
} else if (to_fd >= 0 && close(to_fd) < 0) {
927+
*err_status = "clone_close_dst";
901928
result = -1;
929+
}
902930
if (result != 0)
903931
unlink(to);
904932
if (result == 0)
@@ -1132,15 +1160,15 @@ int transform_save(struct augeas *aug, struct tree *xfm,
11321160
goto done;
11331161
}
11341162

1135-
r = clone_file(augorig_canon, augsave, &err_status, 1);
1163+
r = clone_file(augorig_canon, augsave, &err_status, 1, 1);
11361164
if (r != 0) {
11371165
dyn_err_status = strappend(err_status, "_augsave");
11381166
goto done;
11391167
}
11401168
}
11411169
}
11421170

1143-
r = clone_file(augtemp, augdest, &err_status, copy_if_rename_fails);
1171+
r = clone_file(augtemp, augdest, &err_status, copy_if_rename_fails, 0);
11441172
if (r != 0) {
11451173
dyn_err_status = strappend(err_status, "_augtemp");
11461174
goto done;
@@ -1298,7 +1326,7 @@ int remove_file(struct augeas *aug, struct tree *tree) {
12981326
goto error;
12991327
}
13001328

1301-
r = clone_file(augorig_canon, augsave, &err_status, 1);
1329+
r = clone_file(augorig_canon, augsave, &err_status, 1, 1);
13021330
if (r != 0) {
13031331
dyn_err_status = strappend(err_status, "_augsave");
13041332
goto error;

Diff for: tests/Makefile.am

+3-2
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,9 @@ check_SCRIPTS = \
184184
$(lens_tests) \
185185
test-get.sh test-augtool.sh \
186186
test-put-symlink.sh test-put-symlink-augnew.sh \
187-
test-put-symlink-augsave.sh test-put-symlink-augtemp.sh test-save-empty.sh \
188-
test-bug-1.sh test-idempotent.sh test-preserve.sh \
187+
test-put-symlink-augsave.sh test-put-symlink-augtemp.sh \
188+
test-put-mount.sh test-put-mount-augnew.sh test-put-mount-augsave.sh \
189+
test-save-empty.sh test-bug-1.sh test-idempotent.sh test-preserve.sh \
189190
test-events-saved.sh test-save-mode.sh test-unlink-error.sh \
190191
test-augtool-empty-line.sh test-augtool-modify-root.sh
191192

Diff for: tests/test-put-mount-augnew.sh

+69
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
#! /bin/bash
2+
3+
# Test that we can write into a bind mount placed at PATH.augnew with the
4+
# copy_if_rename_fails flag.
5+
# This requires that EXDEV or EBUSY is returned from rename(2) to activate the
6+
# code path, so set up a bind mount on Linux.
7+
8+
if [ $UID -ne 0 -o "$(uname -s)" != "Linux" ]; then
9+
echo "Test can only be run as root on Linux to create bind mounts"
10+
exit 77
11+
fi
12+
13+
ROOT=$abs_top_builddir/build/test-put-mount-augnew
14+
LENSES=$abs_top_srcdir/lenses
15+
16+
HOSTS=$ROOT/etc/hosts
17+
HOSTS_AUGNEW=${HOSTS}.augnew
18+
TARGET=$ROOT/other/real_hosts
19+
20+
rm -rf $ROOT
21+
mkdir -p $(dirname $HOSTS)
22+
mkdir -p $(dirname $TARGET)
23+
24+
echo 127.0.0.1 localhost > $HOSTS
25+
touch $TARGET $HOSTS_AUGNEW
26+
27+
mount --bind $TARGET $HOSTS_AUGNEW
28+
Exit() {
29+
umount $HOSTS_AUGNEW
30+
exit $1
31+
}
32+
33+
HOSTS_SUM=$(sum $HOSTS)
34+
35+
augtool --nostdinc -I $LENSES -r $ROOT --new <<EOF
36+
set /augeas/save/copy_if_rename_fails 1
37+
set /files/etc/hosts/1/alias myhost
38+
save
39+
print /augeas//error
40+
EOF
41+
42+
if [ ! -f $HOSTS ] ; then
43+
echo "/etc/hosts is no longer a regular file"
44+
Exit 1
45+
fi
46+
if [ ! "x${HOSTS_SUM}" = "x$(sum $HOSTS)" ]; then
47+
echo "/etc/hosts has changed"
48+
Exit 1
49+
fi
50+
if [ ! "x${HOSTS_SUM}" = "x$(sum $HOSTS)" ]; then
51+
echo "/etc/hosts has changed"
52+
Exit 1
53+
fi
54+
55+
if [ ! -s $HOSTS_AUGNEW ]; then
56+
echo "/etc/hosts.augnew is empty"
57+
Exit 1
58+
fi
59+
if [ ! -s $TARGET ]; then
60+
echo "/other/real_hosts is empty"
61+
Exit 1
62+
fi
63+
64+
if ! grep myhost $TARGET >/dev/null; then
65+
echo "/other/real_hosts does not contain the modification"
66+
Exit 1
67+
fi
68+
69+
Exit 0

Diff for: tests/test-put-mount-augsave.sh

+62
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
#! /bin/bash
2+
3+
# Test that we don't follow bind mounts when writing to .augsave.
4+
# This requires that EXDEV or EBUSY is returned from rename(2) to activate the
5+
# code path, so set up a bind mount on Linux.
6+
7+
if [ $UID -ne 0 -o "$(uname -s)" != "Linux" ]; then
8+
echo "Test can only be run as root on Linux to create bind mounts"
9+
exit 77
10+
fi
11+
12+
actual() {
13+
(augtool --nostdinc -I $LENSES -r $ROOT --backup | grep ^/augeas) <<EOF
14+
set /augeas/save/copy_if_rename_fails 1
15+
set /files/etc/hosts/1/alias myhost
16+
save
17+
print /augeas//error
18+
EOF
19+
}
20+
21+
expected() {
22+
cat <<EOF
23+
/augeas/files/etc/hosts/error = "clone_unlink_dst_augsave"
24+
/augeas/files/etc/hosts/error/message = "Device or resource busy"
25+
EOF
26+
}
27+
28+
ROOT=$abs_top_builddir/build/test-put-mount-augsave
29+
LENSES=$abs_top_srcdir/lenses
30+
31+
HOSTS=$ROOT/etc/hosts
32+
HOSTS_AUGSAVE=${HOSTS}.augsave
33+
34+
ATTACK_FILE=$ROOT/other/attack
35+
36+
rm -rf $ROOT
37+
mkdir -p $(dirname $HOSTS)
38+
mkdir -p $(dirname $ATTACK_FILE)
39+
40+
echo 127.0.0.1 localhost > $HOSTS
41+
touch $ATTACK_FILE $HOSTS_AUGSAVE
42+
43+
mount --bind $ATTACK_FILE $HOSTS_AUGSAVE
44+
Exit() {
45+
umount $HOSTS_AUGSAVE
46+
exit $1
47+
}
48+
49+
ACTUAL=$(actual)
50+
EXPECTED=$(expected)
51+
if [ "$ACTUAL" != "$EXPECTED" ]; then
52+
echo "No error when trying to unlink augsave (a bind mount):"
53+
echo "$ACTUAL"
54+
exit 1
55+
fi
56+
57+
if [ -s $ATTACK_FILE ]; then
58+
echo "/other/attack now contains data, should be blank"
59+
Exit 1
60+
fi
61+
62+
Exit 0

Diff for: tests/test-put-mount.sh

+55
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
#! /bin/bash
2+
3+
# Test that we can write into a bind mount with the copy_if_rename_fails flag.
4+
# This requires that EXDEV or EBUSY is returned from rename(2) to activate the
5+
# code path, so set up a bind mount on Linux.
6+
7+
if [ $UID -ne 0 -o "$(uname -s)" != "Linux" ]; then
8+
echo "Test can only be run as root on Linux to create bind mounts"
9+
exit 77
10+
fi
11+
12+
ROOT=$abs_top_builddir/build/test-put-mount
13+
LENSES=$abs_top_srcdir/lenses
14+
15+
HOSTS=$ROOT/etc/hosts
16+
TARGET=$ROOT/other/real_hosts
17+
18+
rm -rf $ROOT
19+
mkdir -p $(dirname $HOSTS)
20+
mkdir -p $(dirname $TARGET)
21+
22+
echo 127.0.0.1 localhost > $TARGET
23+
touch $HOSTS
24+
25+
mount --bind $TARGET $HOSTS
26+
Exit() {
27+
umount $HOSTS
28+
exit $1
29+
}
30+
31+
HOSTS_SUM=$(sum $HOSTS)
32+
33+
augtool --nostdinc -I $LENSES -r $ROOT <<EOF
34+
set /augeas/save/copy_if_rename_fails 1
35+
set /files/etc/hosts/1/alias myhost
36+
save
37+
print /augeas//error
38+
EOF
39+
40+
if [ ! "x${HOSTS_SUM}" != "x$(sum $HOSTS)" ]; then
41+
echo "/etc/hosts hasn't changed"
42+
Exit 1
43+
fi
44+
45+
if [ ! "x${HOSTS_SUM}" != "x$(sum $TARGET)" ]; then
46+
echo "/other/real_hosts hasn't changed"
47+
Exit 1
48+
fi
49+
50+
if ! grep myhost $TARGET >/dev/null; then
51+
echo "/other/real_hosts does not contain the modification"
52+
Exit 1
53+
fi
54+
55+
Exit 0

0 commit comments

Comments
 (0)