aboutsummaryrefslogtreecommitdiff
path: root/editors
diff options
context:
space:
mode:
authorRob Landley <rob@landley.net>2005-05-18 05:56:16 +0000
committerRob Landley <rob@landley.net>2005-05-18 05:56:16 +0000
commit5797c7f0ef93f3efd0ba6634640f9591716214e3 (patch)
tree76722f5b18215592a6b9c7c36693812df84cf3d8 /editors
parent1fb7961e081f43fcfdb12bc588cc294115841f9c (diff)
downloadbusybox-5797c7f0ef93f3efd0ba6634640f9591716214e3.tar.gz
Doug Swarin pointed out a security bug in the -i option of sed.
While the permissions on the temp file are correct to prevent it from being maliciously mangled by passing strangers, (created with 600, opened O_EXCL, etc), the permissions on the _directory_ might not be, and we re-open the file to convert the filehandle to a FILE * (and automatically get an error message and exit if the directory's read-only or out of space or some such). This opens a potential race condition if somebody's using dnotify on the directory, deletes/renames the tempfile, and drops a symlink or something there. Somebody running sed -i as root in a world writeable directory could do damage. I dug up notes on an earlier discussion where we looked at the security implications of this (unfortunately on the #uclibc channel rather than email; I don't have a transcript, just notes-to-self) which pointed out that if the permissions on the directory allow other people's files to be deleted/renamed then the original file is vulnerable to sabotage anyway. However, there are two cases that discussion apparently didn't take into account: 1) Using another user's permissions to damage files in other directories you can't access (standard symlink attack). 2) Reading data another user couldn't otherwise access by having the new file belong to that other user. This patch uses fdopen to convert the filehandle into a FILE *, rather than reopening the file.
Diffstat (limited to 'editors')
-rw-r--r--editors/sed.c17
1 files changed, 8 insertions, 9 deletions
diff --git a/editors/sed.c b/editors/sed.c
index 7950b9303..1b6ed2b0f 100644
--- a/editors/sed.c
+++ b/editors/sed.c
@@ -135,7 +135,7 @@ static sed_cmd_t sed_cmd_head;
static sed_cmd_t *sed_cmd_tail = &sed_cmd_head;
/* Linked list of append lines */
-static struct append_list {
+struct append_list {
char *string;
struct append_list *next;
};
@@ -1187,10 +1187,7 @@ extern int sed_main(int argc, char **argv)
* files were specified or '-' was specified, take input from stdin.
* Otherwise, we process all the files specified. */
if (argv[optind] == NULL) {
- if(in_place) {
- fprintf(stderr,"sed: Filename required for -i\n");
- exit(1);
- }
+ if(in_place) bb_error_msg_and_die("Filename required for -i");
add_input_file(stdin);
process_files();
} else {
@@ -1206,14 +1203,16 @@ extern int sed_main(int argc, char **argv)
if (file) {
if(in_place) {
struct stat statbuf;
+ int nonstdoutfd;
+
outname=bb_xstrndup(argv[i],strlen(argv[i])+6);
strcat(outname,"XXXXXX");
- mkstemp(outname);
- nonstdout=bb_wfopen(outname,"w");
+ if(-1==(nonstdoutfd=mkstemp(outname)))
+ bb_error_msg_and_die("no temp file");
+ nonstdout=fdopen(nonstdoutfd,"w");
/* Set permissions of output file */
fstat(fileno(file),&statbuf);
- fchmod(fileno(nonstdout),statbuf.st_mode);
- atexit(cleanup_outname);
+ fchmod(nonstdoutfd,statbuf.st_mode);
add_input_file(file);
process_files();
fclose(nonstdout);