[RFC PATCH v2] setup: avoid unconditional open syscall with write flags

[RFC PATCH v2] setup: avoid unconditional open syscall with write flags

[Christian Göttsche]

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;
+
+	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)
-- 
2.39.0

Re: [RFC PATCH v2] setup: avoid unconditional open syscall with write flags

[Æ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?

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?

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.

Re: [RFC PATCH v2] setup: avoid unconditional open syscall with write flags

[René Scharfe]

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é

Re: [RFC PATCH v2] setup: avoid unconditional open syscall with write flags

[Junio C Hamano]

Æ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