On Sat, Oct 07, 2006 at 03:32:45PM +0200, Bram Moolenaar wrote: > > here comes another bug report from the Debian bug tracking system, with > > a small security issue. > > > > Quoting from http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=290507: > > > > > Vim does not close temporary file (.file.swp) when executing shell, so > > > program executed in shell can read and write from/to that file, even if > > > It is not possible with normal command invocation. Not sure wheter it is > > > really recurity problem though. > > > > > > Example: > > > > > > # cd > > > # vim file > > > [edit file and enter :sh to run shell] > > > # su user > > > $ ls -l .file.swp > > > ls: .file.swp: Permission denied > > > $ ls -l /proc/self/fd > > > ... > > > lrwx------ 1 user user 64 2005-01-14 15:55 11 -> /root/.file.swp > > > ... > > > $ echo -e '\nqwerty' >&11 > > > $ ^D > > > # tail -1 .file.swp > > > qwerty > > > # > > > > What's your opinion on this? Do you consider it a bug or not? > > It's very normal for a program to keep files open when executing a shell > command. I don't see why Vim should close files before executing a > shell command. > > If there is a security issue it's that Linux allows writing directly > into a file descriptor from another process. In my opinion only a > process itself should be able to do that. > > Hmm, perhaps the problem is that system() or execvp() doesn't close the > file descriptors in a child process?
Indeed, that is the case. Children inherit their parent's file descriptors unless the file descriptors are set to close on exec(). Attached patch does that. -- James GPG Key: 1024D/61326D40 2003-09-02 James Vega <[email protected]>
diff --git a/src/ex_cmds2.c b/src/ex_cmds2.c
index 1108ab9..0b788f4 100644
--- a/src/ex_cmds2.c
+++ b/src/ex_cmds2.c
@@ -2801,7 +2801,6 @@ source_level(cookie)
static char_u *get_one_sourceline __ARGS((struct source_cookie *sp));
-#if defined(WIN32) && defined(FEAT_CSCOPE)
static FILE *fopen_noinh_readbin __ARGS((char *filename));
/*
@@ -2811,13 +2810,21 @@ static FILE *fopen_noinh_readbin __ARGS((char *filename));
fopen_noinh_readbin(filename)
char *filename;
{
- int fd_tmp = mch_open(filename, O_RDONLY | O_BINARY | O_NOINHERIT, 0);
+ int fd_tmp = mch_open(filename, O_RDONLY
+#ifdef WIN32
+ O_BINARY | O_NOINHERIT
+#endif
+ , 0);
if (fd_tmp == -1)
return NULL;
+#ifdef UNIX
+ else
+ fcntl(fd_tmp, F_SETFD, FD_CLOEXEC);
+#endif
+
return fdopen(fd_tmp, READBIN);
}
-#endif
/*
@@ -2894,11 +2901,7 @@ do_source(fname, check_other, is_vimrc)
apply_autocmds(EVENT_SOURCEPRE, fname_exp, fname_exp, FALSE, curbuf);
#endif
-#if defined(WIN32) && defined(FEAT_CSCOPE)
cookie.fp = fopen_noinh_readbin((char *)fname_exp);
-#else
- cookie.fp = mch_fopen((char *)fname_exp, READBIN);
-#endif
if (cookie.fp == NULL && check_other)
{
/*
@@ -2915,11 +2918,7 @@ do_source(fname, check_other, is_vimrc)
*p = '.';
else
*p = '_';
-#if defined(WIN32) && defined(FEAT_CSCOPE)
cookie.fp = fopen_noinh_readbin((char *)fname_exp);
-#else
- cookie.fp = mch_fopen((char *)fname_exp, READBIN);
-#endif
}
}
diff --git a/src/fileio.c b/src/fileio.c
index b17806b..00c2f6c 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2252,6 +2252,10 @@ failed:
if (!read_buffer && !read_stdin)
close(fd); /* errors are ignored */
+#ifdef UNIX
+ else
+ fcntl(fd, F_SETFD, FD_CLOEXEC);
+#endif
vim_free(buffer);
#ifdef HAVE_DUP
diff --git a/src/memfile.c b/src/memfile.c
index 0b3cdd9..b1ca017 100644
--- a/src/memfile.c
+++ b/src/memfile.c
@@ -1347,5 +1347,8 @@ mf_do_open(mfp, fname, flags)
mch_copy_sec(fname, mfp->mf_fname);
#endif
mch_hide(mfp->mf_fname); /* try setting the 'hidden' flag */
+#ifdef UNIX
+ fcntl(mfp->mf_fd, F_SETFD, FD_CLOEXEC);
+#endif
}
}
diff --git a/src/memline.c b/src/memline.c
index 8c8019a..707ab92 100644
--- a/src/memline.c
+++ b/src/memline.c
@@ -490,6 +490,12 @@ ml_setname(buf)
EMSG(_("E301: Oops, lost the swap file!!!"));
return;
}
+#ifdef UNIX
+ else
+ {
+ fcntl(mfp->mf_fd, F_SETFD, FD_CLOEXEC);
+ }
+#endif
}
if (!success)
EMSG(_("E302: Could not rename swap file"));
signature.asc
Description: Digital signature
