Am 27.12.22 um 15:40 schrieb Ævar Arnfjörð Bjarmason: > > On Tue, Dec 27 2022, Christian Göttsche wrote: > > When you submit a re-roll to the git ML please use the "--in-reply-to" > option to format-patch, or equivalent. > > I see the original thread is at > https://lore.kernel.org/git/20221205190019.52829-1-cgzones@xxxxxxxxxxxxxx/, > but this is now detached from it. > >> Commit 57f5d52a942 ("common-main: call sanitize_stdfds()") added the >> sanitization for standard file descriptors (stdin, stdout, stderr) to >> all binaries. The lead to all binaries unconditionally opening >> /dev/null with the flag O_RDWR (read and write). Most of the time the >> standard file descriptors should be set up properly and the sanitization >> ends up doing nothing. >> >> There are many git operations, like `git status` or `git stash list`, >> which might be called by a parent to gather information about the >> repository and should work on a read-only repository. That parent might >> run under a seccomp filter to avoid accidental modification or unwanted >> command execution on memory corruptions. As part of that seccomp filter >> open(2) and openat(2) might be only allowed in read-only mode >> (O_RDONLY), thus preventing git's sanitation and stopping the >> application. >> >> Check the need of sanitization with a file descriptor in read-only mode, >> keep it as replacement for stdin and open replacement file descriptors >> for stdout and stderr in write-only mode. >> >> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx> >> --- >> >> v2: >> - switch to xopen("/dev/null", O_RDONLY) to stay at 2 syscalls in the >> common case and use O_WRONLY for stdout and stderr, as suggested >> by René Scharfe >> --- >> setup.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/setup.c b/setup.c >> index cefd5f6..c57582b 100644 >> --- a/setup.c >> +++ b/setup.c >> @@ -1669,7 +1669,15 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code) >> /* if any standard file descriptor is missing open it to /dev/null */ >> void sanitize_stdfds(void) >> { >> - int fd = xopen("/dev/null", O_RDWR); >> + int fd; > > Aside from the meaningful part of this change, please change this to > avoid refactoring the "int fd = open(...)" part of this. > > In your v1 you needed to do that, as we had code between the "int fd" > and "xopen". > > But in your v2 here that code has gone away, but you kept the > now-unnecessary refactoring. > >> + fd = xopen("/dev/null", O_RDONLY); >> + if (fd > 0) >> + close(fd); >> + if (fd > 2) >> + return; >> + >> + fd = xopen("/dev/null", O_WRONLY); >> while (fd < 2) >> fd = xdup(fd); >> if (fd > 2) > > I don't really get this, if it's safe under seccomp to open this > O_RDONLY wasn't it always redundant to use O_WRONLY, and this change > should just be switching to O_RDONLY? File descriptor 0 (stdin) should only be read. FDs 1 and 2 (stdout and stderr) should only be written to. The old code called open(2) once for all three, thus needed O_RDWR. The new code uses minimal file access modes. > If I just make this function "return;" (and do nothing else) I get test > failures in t6500-gc.sh, which matches the description of 1d999ddd1da > (daemon/shell: refactor redirection of 0/1/2 from /dev/null, > 2013-07-16). > > This will have the tests pass: > > diff --git a/setup.c b/setup.c > index cefd5f63c46..8a218961cb1 100644 > --- a/setup.c > +++ b/setup.c > @@ -1669,7 +1669,7 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code) > /* if any standard file descriptor is missing open it to /dev/null */ > void sanitize_stdfds(void) > { > - int fd = xopen("/dev/null", O_RDWR); > + int fd = xopen("/dev/null", O_RDONLY); > while (fd < 2) > fd = xdup(fd); > if (fd > 2) > > Now, I'm not saying that's safe, we may just have missing test coverage, > but if it isn't safe and results in some subtle issue isn't that the > issue you're going to be exposing here? No, sanitized fds 1 and 2 must be writable, and the proposed change uses O_WRONLY for them. > Another thing that passes tests for me is: > > diff --git a/common-main.c b/common-main.c > index 0a22861f1ce..7b6059e4ee0 100644 > --- a/common-main.c > +++ b/common-main.c > @@ -30,12 +30,6 @@ int main(int argc, const char **argv) > > trace2_initialize_clock(); > > - /* > - * Always open file descriptors 0/1/2 to avoid clobbering files > - * in die(). It also avoids messing up when the pipes are dup'ed > - * onto stdin/stdout/stderr in the child processes we spawn. > - */ > - sanitize_stdfds(); > restore_sigpipe_to_default(); > > git_resolve_executable_dir(argv[0]); > diff --git a/setup.c b/setup.c > index cefd5f63c46..27828f6f076 100644 > --- a/setup.c > +++ b/setup.c > @@ -1695,6 +1695,12 @@ int daemonize(void) > close(0); > close(1); > close(2); > + > + /* > + * Always open file descriptors 0/1/2 to avoid clobbering files > + * in die(). It also avoids messing up when the pipes are dup'ed > + * onto stdin/stdout/stderr in the child processes we spawn. > + */ > sanitize_stdfds(); > return 0; > #endif > > Which would avoid changing the behavior away from O_RDWR, but mov it to > daemonize(). > > Which again, may be horribly broken, I don't know, I'm just noting these > for discussion, as it seems to me that your proposed change is > implicitly suggesting that doing some version of of these is safe. Not sanitizing the standard file descriptors of commands risks the consequences described in the moved comment. How could the proposed change lead to one or more of the standard file descriptors not being sanitized? René
