{"thread":{"id":"59007","subject":"[RFC PATCH v2] setup: avoid unconditional open syscall with write flags","startedAt":"2022-12-27T14:32:13Z","lastAt":"2022-12-27T23:03:46Z","messageCount":4,"participants":["Christian Göttsche","Ævar Arnfjörð Bjarmason","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"469550","messageId":"20221227143200.1182341-1-cgzones@googlemail.com","threadId":"59007","inReplyTo":null,"subject":"[RFC PATCH v2] setup: avoid unconditional open syscall with write flags","fromName":"Christian Göttsche","fromEmail":"cgzones@googlemail.com","sentAt":"2022-12-27T14:32:00Z","receivedAt":"2022-12-27T14:32:13Z","isPatch":true,"sender":{"key":"cgzones@googlemail.com","avatar":null},"body":"Commit 57f5d52a942 (\"common-main: call sanitize_stdfds()\") added the\nsanitization for standard file descriptors (stdin, stdout, stderr) to\nall binaries.  The lead to all binaries unconditionally opening\n/dev/null with the flag O_RDWR (read and write).  Most of the time the\nstandard file descriptors should be set up properly and the sanitization\nends up doing nothing.\n\nThere are many git operations, like `git status` or `git stash list`,\nwhich might be called by a parent to gather information about the\nrepository and should work on a read-only repository.  That parent might\nrun under a seccomp filter to avoid accidental modification or unwanted\ncommand execution on memory corruptions.  As part of that seccomp filter\nopen(2) and openat(2) might be only allowed in read-only mode\n(O_RDONLY), thus preventing git's sanitation and stopping the\napplication.\n\nCheck the need of sanitization with a file descriptor in read-only mode,\nkeep it as replacement for stdin and open replacement file descriptors\nfor stdout and stderr in write-only mode.\n\nSigned-off-by: Christian Göttsche <cgzones@googlemail.com>\n---\n\nv2:\n  - switch to xopen(\"/dev/null\", O_RDONLY) to stay at 2 syscalls in the\n    common case and use O_WRONLY for stdout and stderr, as suggested\n    by René Scharfe\n---\n setup.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex cefd5f6..c57582b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1669,7 +1669,15 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)\n /* if any standard file descriptor is missing open it to /dev/null */\n void sanitize_stdfds(void)\n {\n-\tint fd = xopen(\"/dev/null\", O_RDWR);\n+\tint fd;\n+\n+\tfd = xopen(\"/dev/null\", O_RDONLY);\n+\tif (fd > 0)\n+\t\tclose(fd);\n+\tif (fd > 2)\n+\t\treturn;\n+\n+\tfd = xopen(\"/dev/null\", O_WRONLY);\n \twhile (fd < 2)\n \t\tfd = xdup(fd);\n \tif (fd > 2)\n-- \n2.39.0\n\n"},{"id":"469552","messageId":"221227.86lemsvqmb.gmgdl@evledraar.gmail.com","threadId":"59007","inReplyTo":"20221227143200.1182341-1-cgzones@googlemail.com","subject":"Re: [RFC PATCH v2] setup: avoid unconditional open syscall with write flags","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-27T14:40:59Z","receivedAt":"2022-12-27T15:00:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 27 2022, Christian Göttsche wrote:\n\nWhen you submit a re-roll to the git ML please use the \"--in-reply-to\"\noption to format-patch, or equivalent.\n\nI see the original thread is at\nhttps://lore.kernel.org/git/20221205190019.52829-1-cgzones@googlemail.com/,\nbut this is now detached from it.\n\n> Commit 57f5d52a942 (\"common-main: call sanitize_stdfds()\") added the\n> sanitization for standard file descriptors (stdin, stdout, stderr) to\n> all binaries.  The lead to all binaries unconditionally opening\n> /dev/null with the flag O_RDWR (read and write).  Most of the time the\n> standard file descriptors should be set up properly and the sanitization\n> ends up doing nothing.\n>\n> There are many git operations, like `git status` or `git stash list`,\n> which might be called by a parent to gather information about the\n> repository and should work on a read-only repository.  That parent might\n> run under a seccomp filter to avoid accidental modification or unwanted\n> command execution on memory corruptions.  As part of that seccomp filter\n> open(2) and openat(2) might be only allowed in read-only mode\n> (O_RDONLY), thus preventing git's sanitation and stopping the\n> application.\n>\n> Check the need of sanitization with a file descriptor in read-only mode,\n> keep it as replacement for stdin and open replacement file descriptors\n> for stdout and stderr in write-only mode.\n>\n> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>\n> ---\n>\n> v2:\n>   - switch to xopen(\"/dev/null\", O_RDONLY) to stay at 2 syscalls in the\n>     common case and use O_WRONLY for stdout and stderr, as suggested\n>     by René Scharfe\n> ---\n>  setup.c | 10 +++++++++-\n>  1 file changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/setup.c b/setup.c\n> index cefd5f6..c57582b 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1669,7 +1669,15 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)\n>  /* if any standard file descriptor is missing open it to /dev/null */\n>  void sanitize_stdfds(void)\n>  {\n> -\tint fd = xopen(\"/dev/null\", O_RDWR);\n> +\tint fd;\n\nAside from the meaningful part of this change, please change this to\navoid refactoring the \"int fd = open(...)\" part of this.\n\nIn your v1 you needed to do that, as we had code between the \"int fd\"\nand \"xopen\".\n\nBut in your v2 here that code has gone away, but you kept the\nnow-unnecessary refactoring.\n\n> +\tfd = xopen(\"/dev/null\", O_RDONLY);\n> +\tif (fd > 0)\n> +\t\tclose(fd);\n> +\tif (fd > 2)\n> +\t\treturn;\n> +\n> +\tfd = xopen(\"/dev/null\", O_WRONLY);\n>  \twhile (fd < 2)\n>  \t\tfd = xdup(fd);\n>  \tif (fd > 2)\n\nI don't really get this, if it's safe under seccomp to open this\nO_RDONLY wasn't it always redundant to use O_WRONLY, and this change\nshould just be switching to O_RDONLY?\n\nIf I just make this function \"return;\" (and do nothing else) I get test\nfailures in t6500-gc.sh, which matches the description of 1d999ddd1da\n(daemon/shell: refactor redirection of 0/1/2 from /dev/null,\n2013-07-16).\n\nThis will have the tests pass:\n\t\n\tdiff --git a/setup.c b/setup.c\n\tindex cefd5f63c46..8a218961cb1 100644\n\t--- a/setup.c\n\t+++ b/setup.c\n\t@@ -1669,7 +1669,7 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)\n\t /* if any standard file descriptor is missing open it to /dev/null */\n\t void sanitize_stdfds(void)\n\t {\n\t-\tint fd = xopen(\"/dev/null\", O_RDWR);\n\t+\tint fd = xopen(\"/dev/null\", O_RDONLY);\n\t \twhile (fd < 2)\n\t \t\tfd = xdup(fd);\n\t \tif (fd > 2)\n\t\nNow, I'm not saying that's safe, we may just have missing test coverage,\nbut if it isn't safe and results in some subtle issue isn't that the\nissue you're going to be exposing here?\n\nAnother thing that passes tests for me is:\n\t\n\tdiff --git a/common-main.c b/common-main.c\n\tindex 0a22861f1ce..7b6059e4ee0 100644\n\t--- a/common-main.c\n\t+++ b/common-main.c\n\t@@ -30,12 +30,6 @@ int main(int argc, const char **argv)\n\t \n\t \ttrace2_initialize_clock();\n\t \n\t-\t/*\n\t-\t * Always open file descriptors 0/1/2 to avoid clobbering files\n\t-\t * in die().  It also avoids messing up when the pipes are dup'ed\n\t-\t * onto stdin/stdout/stderr in the child processes we spawn.\n\t-\t */\n\t-\tsanitize_stdfds();\n\t \trestore_sigpipe_to_default();\n\t \n\t \tgit_resolve_executable_dir(argv[0]);\n\tdiff --git a/setup.c b/setup.c\n\tindex cefd5f63c46..27828f6f076 100644\n\t--- a/setup.c\n\t+++ b/setup.c\n\t@@ -1695,6 +1695,12 @@ int daemonize(void)\n\t \tclose(0);\n\t \tclose(1);\n\t \tclose(2);\n\t+\n\t+\t/*\n\t+\t * Always open file descriptors 0/1/2 to avoid clobbering files\n\t+\t * in die().  It also avoids messing up when the pipes are dup'ed\n\t+\t * onto stdin/stdout/stderr in the child processes we spawn.\n\t+\t */\n\t \tsanitize_stdfds();\n\t \treturn 0;\n\t #endif\n\nWhich would avoid changing the behavior away from O_RDWR, but mov it to\ndaemonize().\n\nWhich again, may be horribly broken, I don't know, I'm just noting these\nfor discussion, as it seems to me that your proposed change is\nimplicitly suggesting that doing some version of of these is safe.\n"},{"id":"469563","messageId":"9df41c56-40bf-f232-c484-9aedbb22aa2b@web.de","threadId":"59007","inReplyTo":"221227.86lemsvqmb.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH v2] setup: avoid unconditional open syscall with write flags","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-27T20:36:26Z","receivedAt":"2022-12-27T21:05:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.12.22 um 15:40 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, Dec 27 2022, Christian Göttsche wrote:\n>\n> When you submit a re-roll to the git ML please use the \"--in-reply-to\"\n> option to format-patch, or equivalent.\n>\n> I see the original thread is at\n> https://lore.kernel.org/git/20221205190019.52829-1-cgzones@googlemail.com/,\n> but this is now detached from it.\n>\n>> Commit 57f5d52a942 (\"common-main: call sanitize_stdfds()\") added the\n>> sanitization for standard file descriptors (stdin, stdout, stderr) to\n>> all binaries.  The lead to all binaries unconditionally opening\n>> /dev/null with the flag O_RDWR (read and write).  Most of the time the\n>> standard file descriptors should be set up properly and the sanitization\n>> ends up doing nothing.\n>>\n>> There are many git operations, like `git status` or `git stash list`,\n>> which might be called by a parent to gather information about the\n>> repository and should work on a read-only repository.  That parent might\n>> run under a seccomp filter to avoid accidental modification or unwanted\n>> command execution on memory corruptions.  As part of that seccomp filter\n>> open(2) and openat(2) might be only allowed in read-only mode\n>> (O_RDONLY), thus preventing git's sanitation and stopping the\n>> application.\n>>\n>> Check the need of sanitization with a file descriptor in read-only mode,\n>> keep it as replacement for stdin and open replacement file descriptors\n>> for stdout and stderr in write-only mode.\n>>\n>> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>\n>> ---\n>>\n>> v2:\n>>   - switch to xopen(\"/dev/null\", O_RDONLY) to stay at 2 syscalls in the\n>>     common case and use O_WRONLY for stdout and stderr, as suggested\n>>     by René Scharfe\n>> ---\n>>  setup.c | 10 +++++++++-\n>>  1 file changed, 9 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/setup.c b/setup.c\n>> index cefd5f6..c57582b 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -1669,7 +1669,15 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)\n>>  /* if any standard file descriptor is missing open it to /dev/null */\n>>  void sanitize_stdfds(void)\n>>  {\n>> -\tint fd = xopen(\"/dev/null\", O_RDWR);\n>> +\tint fd;\n>\n> Aside from the meaningful part of this change, please change this to\n> avoid refactoring the \"int fd = open(...)\" part of this.\n>\n> In your v1 you needed to do that, as we had code between the \"int fd\"\n> and \"xopen\".\n>\n> But in your v2 here that code has gone away, but you kept the\n> now-unnecessary refactoring.\n>\n>> +\tfd = xopen(\"/dev/null\", O_RDONLY);\n>> +\tif (fd > 0)\n>> +\t\tclose(fd);\n>> +\tif (fd > 2)\n>> +\t\treturn;\n>> +\n>> +\tfd = xopen(\"/dev/null\", O_WRONLY);\n>>  \twhile (fd < 2)\n>>  \t\tfd = xdup(fd);\n>>  \tif (fd > 2)\n>\n> I don't really get this, if it's safe under seccomp to open this\n> O_RDONLY wasn't it always redundant to use O_WRONLY, and this change\n> should just be switching to O_RDONLY?\n\nFile descriptor 0 (stdin) should only be read.  FDs 1 and 2 (stdout and\nstderr) should only be written to.  The old code called open(2) once for\nall three, thus needed O_RDWR.  The new code uses minimal file access\nmodes.\n\n> If I just make this function \"return;\" (and do nothing else) I get test\n> failures in t6500-gc.sh, which matches the description of 1d999ddd1da\n> (daemon/shell: refactor redirection of 0/1/2 from /dev/null,\n> 2013-07-16).\n>\n> This will have the tests pass:\n>\n> \tdiff --git a/setup.c b/setup.c\n> \tindex cefd5f63c46..8a218961cb1 100644\n> \t--- a/setup.c\n> \t+++ b/setup.c\n> \t@@ -1669,7 +1669,7 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)\n> \t /* if any standard file descriptor is missing open it to /dev/null */\n> \t void sanitize_stdfds(void)\n> \t {\n> \t-\tint fd = xopen(\"/dev/null\", O_RDWR);\n> \t+\tint fd = xopen(\"/dev/null\", O_RDONLY);\n> \t \twhile (fd < 2)\n> \t \t\tfd = xdup(fd);\n> \t \tif (fd > 2)\n>\n> Now, I'm not saying that's safe, we may just have missing test coverage,\n> but if it isn't safe and results in some subtle issue isn't that the\n> issue you're going to be exposing here?\n\nNo, sanitized fds 1 and 2 must be writable, and the proposed change uses\nO_WRONLY for them.\n\n> Another thing that passes tests for me is:\n>\n> \tdiff --git a/common-main.c b/common-main.c\n> \tindex 0a22861f1ce..7b6059e4ee0 100644\n> \t--- a/common-main.c\n> \t+++ b/common-main.c\n> \t@@ -30,12 +30,6 @@ int main(int argc, const char **argv)\n>\n> \t \ttrace2_initialize_clock();\n>\n> \t-\t/*\n> \t-\t * Always open file descriptors 0/1/2 to avoid clobbering files\n> \t-\t * in die().  It also avoids messing up when the pipes are dup'ed\n> \t-\t * onto stdin/stdout/stderr in the child processes we spawn.\n> \t-\t */\n> \t-\tsanitize_stdfds();\n> \t \trestore_sigpipe_to_default();\n>\n> \t \tgit_resolve_executable_dir(argv[0]);\n> \tdiff --git a/setup.c b/setup.c\n> \tindex cefd5f63c46..27828f6f076 100644\n> \t--- a/setup.c\n> \t+++ b/setup.c\n> \t@@ -1695,6 +1695,12 @@ int daemonize(void)\n> \t \tclose(0);\n> \t \tclose(1);\n> \t \tclose(2);\n> \t+\n> \t+\t/*\n> \t+\t * Always open file descriptors 0/1/2 to avoid clobbering files\n> \t+\t * in die().  It also avoids messing up when the pipes are dup'ed\n> \t+\t * onto stdin/stdout/stderr in the child processes we spawn.\n> \t+\t */\n> \t \tsanitize_stdfds();\n> \t \treturn 0;\n> \t #endif\n>\n> Which would avoid changing the behavior away from O_RDWR, but mov it to\n> daemonize().\n>\n> Which again, may be horribly broken, I don't know, I'm just noting these\n> for discussion, as it seems to me that your proposed change is\n> implicitly suggesting that doing some version of of these is safe.\n\nNot sanitizing the standard file descriptors of commands risks the\nconsequences described in the moved comment.\n\nHow could the proposed change lead to one or more of the standard\nfile descriptors not being sanitized?\n\nRené\n\n"},{"id":"469567","messageId":"xmqqa638sb3o.fsf@gitster.g","threadId":"59007","inReplyTo":"221227.86lemsvqmb.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH v2] setup: avoid unconditional open syscall with write flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-27T23:03:39Z","receivedAt":"2022-12-27T23:03:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> +\tfd = xopen(\"/dev/null\", O_RDONLY);\n>> +\tif (fd > 0)\n>> +\t\tclose(fd);\n>> +\tif (fd > 2)\n>> +\t\treturn;\n>> +\n>> +\tfd = xopen(\"/dev/null\", O_WRONLY);\n>>  \twhile (fd < 2)\n>>  \t\tfd = xdup(fd);\n>>  \tif (fd > 2)\n>\n> I don't really get this, if it's safe under seccomp to open this\n> O_RDONLY wasn't it always redundant to use O_WRONLY, and this change\n> should just be switching to O_RDONLY?\n\nThe first \"if we successfully opened /dev/null and also found that\nstdin is already connected to something above 0, then close the fd\"\nmay be tricky to reason about.  There are four cases.\n\n (1) The first xopen(/dev/null, O_RDONLY) fails and returns -1.\n     In this case, we go down the original codepath.\n\n (2) It returns 0 (upon startup, stdin was closed).  We keep the fd\n     to use as the standard input, and go down the original\n     codepath, but now standard input is connected, the original\n     codepath needs to deal with only stdout and stderr, so we can\n     safely use O_WRONLY and dup it into stdout and stderr.\n\n (3) It returns 1 or 2 (upon startup, stdin was open, stdin or\n     stderr was closed).  The fd is closed, and we go to the\n     original codepath.  Again, the original codepath does not have\n     to touch stdin, so we can afford to use O_WRONLY.\n\n (4) It returns 3 or above, indicating that the three fd's we care\n     about was already open.  We return without making further\n     damage after closing the fd.\n\nThe updated code does a single system call to probe for the lowest\nunused file descriptor and returns in the common case.  And that\nsingle system call is an extra overhead for the \"oops some of the\nlow FD were not open\" case compared to the original code, but the\nread-only FD may get reused when we need to.\n\nSo, I think the updated code does what it explains in its proposed\nlog message.  I.e.\n\n    Check the need of sanitization with a file descriptor in read-only mode,\n    keep it as replacement for stdin and open replacement file descriptors\n    for stdout and stderr in write-only mode.\n\n    v2:\n      - switch to xopen(\"/dev/null\", O_RDONLY) to stay at 2 syscalls in the\n        common case and use O_WRONLY for stdout and stderr, as suggested\n        by René Scharfe\n\n"}]}