* Re: [RFC PATCH v2] setup: avoid unconditional open syscall with write flags
2022-12-27 14:40 ` Ævar Arnfjörð Bjarmason
@ 2022-12-27 20:36 ` René Scharfe
2022-12-27 23:03 ` Junio C Hamano
1 sibling, 0 replies; 4+ messages in thread
From: René Scharfe @ 2022-12-27 20:36 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, Christian Göttsche; +Cc: git
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@googlemail.com/,
> 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@googlemail.com>
>> ---
>>
>> 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é
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH v2] setup: avoid unconditional open syscall with write flags
2022-12-27 14:40 ` Ævar Arnfjörð Bjarmason
2022-12-27 20:36 ` René Scharfe
@ 2022-12-27 23:03 ` Junio C Hamano
1 sibling, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2022-12-27 23:03 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Christian Göttsche, git
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> + 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?
The first "if we successfully opened /dev/null and also found that
stdin is already connected to something above 0, then close the fd"
may be tricky to reason about. There are four cases.
(1) The first xopen(/dev/null, O_RDONLY) fails and returns -1.
In this case, we go down the original codepath.
(2) It returns 0 (upon startup, stdin was closed). We keep the fd
to use as the standard input, and go down the original
codepath, but now standard input is connected, the original
codepath needs to deal with only stdout and stderr, so we can
safely use O_WRONLY and dup it into stdout and stderr.
(3) It returns 1 or 2 (upon startup, stdin was open, stdin or
stderr was closed). The fd is closed, and we go to the
original codepath. Again, the original codepath does not have
to touch stdin, so we can afford to use O_WRONLY.
(4) It returns 3 or above, indicating that the three fd's we care
about was already open. We return without making further
damage after closing the fd.
The updated code does a single system call to probe for the lowest
unused file descriptor and returns in the common case. And that
single system call is an extra overhead for the "oops some of the
low FD were not open" case compared to the original code, but the
read-only FD may get reused when we need to.
So, I think the updated code does what it explains in its proposed
log message. I.e.
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.
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
^ permalink raw reply [flat|nested] 4+ messages in thread