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

["René Scharfe" <l.s.r@web.de>]

Am 05.12.22 um 20:00 schrieb 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 non modifying git operations, like `git status` or `git
> stash list`, which might be called by a parent to gather information
> about the 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 before opening /dev/null to populate a possible non-present
> standard file descriptor if actually any is missing.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> Alternatively one could add a command line argument
> (`--no-stdfd-sanitization`).
> ---
>  setup.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/setup.c b/setup.c
> index cefd5f6..2af7170 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1669,7 +1669,14 @@ 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;
> +
> +	if (fcntl(0, F_GETFD) != -1 &&
> +	    fcntl(1, F_GETFD) != -1 &&
> +	    fcntl(2, F_GETFD) != -1)
> +		return;
> +
> +	fd = xopen("/dev/null", O_RDWR);
>  	while (fd < 2)
>  		fd = xdup(fd);
>  	if (fd > 2)

If read-only access is allowed, how about this?

diff --git a/setup.c b/setup.c
index cefd5f63c4..0f52c51759 100644
--- a/setup.c
+++ b/setup.c
@@ -1669,7 +1669,12 @@ 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);
+	if (fd > 0)
+		close(fd);
+	if (fd > 2)
+		return;
+	fd = xopen("/dev/null", O_WRONLY);
 	while (fd < 2)
 		fd = xdup(fd);
 	if (fd > 2)

Requires an extra open/close pair if fd 0 is already open, but no extra
syscalls if 0, 1 and 2 are all open.

Can opening /dev/null (or NUL on Windows) multiple times instead of dup'ing
cause issues?  Can we e.g. lock ourselves out?

René