Skip to content

Commit 1638774

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

10 files changed

+270
-59
lines changed

Diff for: bootstrap

+1
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ gitlog-to-changelog
7272
canonicalize-lgpl
7373
isblank
7474
locale
75+
mkstemp
7576
regex
7677
safe-alloc
7778
selinux-h

Diff for: src/internal.c

+12-3
Original file line numberDiff line numberDiff line change
@@ -125,16 +125,14 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length)
125125
return NULL;
126126
}
127127

128-
char* xread_file(const char *path) {
129-
FILE *fp = fopen(path, "r");
128+
char* xfread_file(FILE *fp) {
130129
char *result;
131130
size_t len;
132131

133132
if (!fp)
134133
return NULL;
135134

136135
result = fread_file_lim(fp, MAX_READ_LEN, &len);
137-
fclose (fp);
138136

139137
if (result != NULL
140138
&& len <= MAX_READ_LEN
@@ -145,6 +143,17 @@ char* xread_file(const char *path) {
145143
return NULL;
146144
}
147145

146+
char* xread_file(const char *path) {
147+
FILE *fp;
148+
char *result;
149+
150+
fp = fopen(path, "r");
151+
result = xfread_file(fp);
152+
fclose (fp);
153+
154+
return result;
155+
}
156+
148157
/*
149158
* Escape/unescape of string literals
150159
*/

Diff for: src/internal.h

+3
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,9 @@ char *format_pos(const char *text, int pos);
287287
*/
288288
char* xread_file(const char *path);
289289

290+
/* Like xread_file, but caller supplies a file pointer */
291+
char* xfread_file(FILE *fp);
292+
290293
/* Get the error message for ERRNUM in a threadsafe way. Based on libvirt's
291294
* virStrError
292295
*/

Diff for: src/transform.c

+88-53
Original file line numberDiff line numberDiff line change
@@ -799,35 +799,38 @@ int transform_applies(struct tree *xfm, const char *path) {
799799
return filter_matches(xfm, path + strlen(AUGEAS_FILES_TREE));
800800
}
801801

802-
static int transfer_file_attrs(const char *from, const char *to,
802+
static int transfer_file_attrs(FILE *from, FILE *to,
803803
const char **err_status) {
804804
struct stat st;
805805
int ret = 0;
806806
int selinux_enabled = (is_selinux_enabled() > 0);
807807
security_context_t con = NULL;
808808

809-
ret = lstat(from, &st);
809+
int from_fd = fileno(from);
810+
int to_fd = fileno(to);
811+
812+
ret = fstat(from_fd, &st);
810813
if (ret < 0) {
811814
*err_status = "replace_stat";
812815
return -1;
813816
}
814817
if (selinux_enabled) {
815-
if (lgetfilecon(from, &con) < 0 && errno != ENOTSUP) {
818+
if (fgetfilecon(from_fd, &con) < 0 && errno != ENOTSUP) {
816819
*err_status = "replace_getfilecon";
817820
return -1;
818821
}
819822
}
820823

821-
if (lchown(to, st.st_uid, st.st_gid) < 0) {
824+
if (fchown(to_fd, st.st_uid, st.st_gid) < 0) {
822825
*err_status = "replace_chown";
823826
return -1;
824827
}
825-
if (chmod(to, st.st_mode) < 0) {
828+
if (fchmod(to_fd, st.st_mode) < 0) {
826829
*err_status = "replace_chmod";
827830
return -1;
828831
}
829832
if (selinux_enabled && con != NULL) {
830-
if (lsetfilecon(to, con) < 0 && errno != ENOTSUP) {
833+
if (fsetfilecon(to_fd, con) < 0 && errno != ENOTSUP) {
831834
*err_status = "replace_setfilecon";
832835
return -1;
833836
}
@@ -869,7 +872,7 @@ static int clone_file(const char *from, const char *to,
869872
goto done;
870873
}
871874

872-
if (transfer_file_attrs(from, to, err_status) < 0)
875+
if (transfer_file_attrs(from_fp, to_fp, err_status) < 0)
873876
goto done;
874877

875878
while ((len = fread(buf, 1, BUFSIZ, from_fp)) > 0) {
@@ -946,19 +949,38 @@ static int file_saved_event(struct augeas *aug, const char *path) {
946949
* are noted in the /augeas/files hierarchy in AUG->ORIGIN under
947950
* PATH/error.
948951
*
949-
* Writing the file happens by first writing into PATH.augnew, transferring
950-
* all file attributes of PATH to PATH.augnew, and then renaming
951-
* PATH.augnew to PATH. If the rename fails, and the entry
952-
* AUGEAS_COPY_IF_FAILURE exists in AUG->ORIGIN, PATH is overwritten by
953-
* copying file contents
952+
* Writing the file happens by first writing into a temp file, transferring all
953+
* file attributes of PATH to the temp file, and then renaming the temp file
954+
* back to PATH.
955+
*
956+
* Temp files are created alongside the destination file to enable the rename,
957+
* which may be the canonical path (PATH_canon) if PATH is a symlink.
958+
*
959+
* If the AUG_SAVE_NEWFILE flag is set, instead rename to PATH.augnew rather
960+
* than PATH. If AUG_SAVE_BACKUP is set, move the original to PATH.augsave.
961+
* (Always PATH.aug{new,save} irrespective of whether PATH is a symlink.)
962+
*
963+
* If the rename fails, and the entry AUGEAS_COPY_IF_FAILURE exists in
964+
* AUG->ORIGIN, PATH is instead overwritten by copying file contents.
965+
*
966+
* The table below shows the locations for each permutation.
967+
*
968+
* PATH save flag temp file dest file backup?
969+
* regular - PATH.augnew.XXXX PATH -
970+
* regular BACKUP PATH.augnew.XXXX PATH PATH.augsave
971+
* regular NEWFILE PATH.augnew.XXXX PATH.augnew -
972+
* symlink - PATH_canon.XXXX PATH_canon -
973+
* symlink BACKUP PATH_canon.XXXX PATH_canon PATH.augsave
974+
* symlink NEWFILE PATH.augnew.XXXX PATH.augnew -
954975
*
955976
* Return 0 on success, -1 on failure.
956977
*/
957978
int transform_save(struct augeas *aug, struct tree *xfm,
958979
const char *path, struct tree *tree) {
959-
FILE *fp = NULL;
960-
char *augnew = NULL, *augorig = NULL, *augsave = NULL;
961-
char *augorig_canon = NULL;
980+
int fd;
981+
FILE *fp = NULL, *augorig_canon_fp = NULL;
982+
char *augtemp = NULL, *augnew = NULL, *augorig = NULL, *augsave = NULL;
983+
char *augorig_canon = NULL, *augdest = NULL;
962984
int augorig_exists;
963985
int copy_if_rename_fails = 0;
964986
char *text = NULL;
@@ -986,19 +1008,6 @@ int transform_save(struct augeas *aug, struct tree *xfm,
9861008
goto done;
9871009
}
9881010

989-
if (access(augorig, R_OK) == 0) {
990-
text = xread_file(augorig);
991-
} else {
992-
text = strdup("");
993-
}
994-
995-
if (text == NULL) {
996-
err_status = "put_read";
997-
goto done;
998-
}
999-
1000-
text = append_newline(text, strlen(text));
1001-
10021011
augorig_canon = canonicalize_file_name(augorig);
10031012
augorig_exists = 1;
10041013
if (augorig_canon == NULL) {
@@ -1011,31 +1020,53 @@ int transform_save(struct augeas *aug, struct tree *xfm,
10111020
}
10121021
}
10131022

1014-
/* Figure out where to put the .augnew file. If we need to rename it
1015-
later on, put it next to augorig_canon */
1023+
if (access(augorig_canon, R_OK) == 0) {
1024+
augorig_canon_fp = fopen(augorig_canon, "r");
1025+
text = xfread_file(augorig_canon_fp);
1026+
} else {
1027+
text = strdup("");
1028+
}
1029+
1030+
if (text == NULL) {
1031+
err_status = "put_read";
1032+
goto done;
1033+
}
1034+
1035+
text = append_newline(text, strlen(text));
1036+
1037+
/* Figure out where to put the .augnew and temp file. If no .augnew file
1038+
then put the temp file next to augorig_canon, else next to .augnew. */
10161039
if (aug->flags & AUG_SAVE_NEWFILE) {
10171040
if (xasprintf(&augnew, "%s" EXT_AUGNEW, augorig) < 0) {
10181041
err_status = "augnew_oom";
10191042
goto done;
10201043
}
1044+
augdest = augnew;
10211045
} else {
1022-
if (xasprintf(&augnew, "%s" EXT_AUGNEW, augorig_canon) < 0) {
1023-
err_status = "augnew_oom";
1024-
goto done;
1025-
}
1046+
augdest = augorig_canon;
1047+
}
1048+
1049+
if (xasprintf(&augtemp, "%s.XXXXXX", augdest) < 0) {
1050+
err_status = "augtemp_oom";
1051+
goto done;
10261052
}
10271053

10281054
// FIXME: We might have to create intermediate directories
10291055
// to be able to write augnew, but we have no idea what permissions
10301056
// etc. they should get. Just the process default ?
1031-
fp = fopen(augnew, "w");
1057+
fd = mkstemp(augtemp);
1058+
if (fd < 0) {
1059+
err_status = "mk_augtemp";
1060+
goto done;
1061+
}
1062+
fp = fdopen(fd, "w");
10321063
if (fp == NULL) {
1033-
err_status = "open_augnew";
1064+
err_status = "open_augtemp";
10341065
goto done;
10351066
}
10361067

10371068
if (augorig_exists) {
1038-
if (transfer_file_attrs(augorig_canon, augnew, &err_status) != 0) {
1069+
if (transfer_file_attrs(augorig_canon_fp, fp, &err_status) != 0) {
10391070
err_status = "xfer_attrs";
10401071
goto done;
10411072
}
@@ -1045,22 +1076,22 @@ int transform_save(struct augeas *aug, struct tree *xfm,
10451076
lns_put(fp, lens, tree->children, text, &err);
10461077

10471078
if (ferror(fp)) {
1048-
err_status = "error_augnew";
1079+
err_status = "error_augtemp";
10491080
goto done;
10501081
}
10511082

10521083
if (fflush(fp) != 0) {
1053-
err_status = "flush_augnew";
1084+
err_status = "flush_augtemp";
10541085
goto done;
10551086
}
10561087

10571088
if (fsync(fileno(fp)) < 0) {
1058-
err_status = "sync_augnew";
1089+
err_status = "sync_augtemp";
10591090
goto done;
10601091
}
10611092

10621093
if (fclose(fp) != 0) {
1063-
err_status = "close_augnew";
1094+
err_status = "close_augtemp";
10641095
fp = NULL;
10651096
goto done;
10661097
}
@@ -1069,33 +1100,33 @@ int transform_save(struct augeas *aug, struct tree *xfm,
10691100

10701101
if (err != NULL) {
10711102
err_status = err->pos >= 0 ? "parse_skel_failed" : "put_failed";
1072-
unlink(augnew);
1103+
unlink(augtemp);
10731104
goto done;
10741105
}
10751106

10761107
{
1077-
char *new_text = xread_file(augnew);
1108+
char *new_text = xread_file(augtemp);
10781109
int same = 0;
10791110
if (new_text == NULL) {
1080-
err_status = "read_augnew";
1111+
err_status = "read_augtemp";
10811112
goto done;
10821113
}
10831114
same = STREQ(text, new_text);
10841115
FREE(new_text);
10851116
if (same) {
10861117
result = 0;
1087-
unlink(augnew);
1118+
unlink(augtemp);
10881119
goto done;
10891120
} else if (aug->flags & AUG_SAVE_NOOP) {
10901121
result = 1;
1091-
unlink(augnew);
1122+
unlink(augtemp);
10921123
goto done;
10931124
}
10941125
}
10951126

10961127
if (!(aug->flags & AUG_SAVE_NEWFILE)) {
10971128
if (augorig_exists && (aug->flags & AUG_SAVE_BACKUP)) {
1098-
r = asprintf(&augsave, "%s%s" EXT_AUGSAVE, aug->root, filename);
1129+
r = xasprintf(&augsave, "%s" EXT_AUGSAVE, augorig);
10991130
if (r == -1) {
11001131
augsave = NULL;
11011132
goto done;
@@ -1107,13 +1138,14 @@ int transform_save(struct augeas *aug, struct tree *xfm,
11071138
goto done;
11081139
}
11091140
}
1110-
r = clone_file(augnew, augorig_canon, &err_status,
1111-
copy_if_rename_fails);
1112-
if (r != 0) {
1113-
dyn_err_status = strappend(err_status, "_augnew");
1114-
goto done;
1115-
}
11161141
}
1142+
1143+
r = clone_file(augtemp, augdest, &err_status, copy_if_rename_fails);
1144+
if (r != 0) {
1145+
dyn_err_status = strappend(err_status, "_augtemp");
1146+
goto done;
1147+
}
1148+
11171149
result = 1;
11181150

11191151
done:
@@ -1138,6 +1170,7 @@ int transform_save(struct augeas *aug, struct tree *xfm,
11381170
free(dyn_err_status);
11391171
lens_release(lens);
11401172
free(text);
1173+
free(augtemp);
11411174
free(augnew);
11421175
if (augorig_canon != augorig)
11431176
free(augorig_canon);
@@ -1147,6 +1180,8 @@ int transform_save(struct augeas *aug, struct tree *xfm,
11471180

11481181
if (fp != NULL)
11491182
fclose(fp);
1183+
if (augorig_canon_fp != NULL)
1184+
fclose(augorig_canon_fp);
11501185
return result;
11511186
}
11521187

Diff for: tests/Makefile.am

+2-1
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,8 @@ check_SCRIPTS = \
183183
test-interpreter.sh \
184184
$(lens_tests) \
185185
test-get.sh test-augtool.sh \
186-
test-put-symlink.sh test-save-empty.sh \
186+
test-put-symlink.sh test-put-symlink-augnew.sh \
187+
test-put-symlink-augsave.sh test-put-symlink-augtemp.sh test-save-empty.sh \
187188
test-bug-1.sh test-idempotent.sh test-preserve.sh \
188189
test-events-saved.sh test-save-mode.sh test-unlink-error.sh \
189190
test-augtool-empty-line.sh test-augtool-modify-root.sh

0 commit comments

Comments
 (0)