{"thread":{"id":"58898","subject":"[PATCH] setup: avoid unconditional open with write flags","startedAt":"2022-12-05T19:00:32Z","lastAt":"2022-12-06T23:39:44Z","messageCount":10,"participants":["Christian Göttsche","brian m. carlson","Taylor Blau","Junio C Hamano","Jeff King","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"468541","messageId":"20221205190019.52829-1-cgzones@googlemail.com","threadId":"58898","inReplyTo":null,"subject":"[PATCH] setup: avoid unconditional open with write flags","fromName":"Christian Göttsche","fromEmail":"cgzones@googlemail.com","sentAt":"2022-12-05T19:00:19Z","receivedAt":"2022-12-05T19:00:32Z","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 non modifying git operations, like `git status` or `git\nstash list`, which might be called by a parent to gather information\nabout the repository. That parent might run under a seccomp filter to\navoid accidental modification or unwanted command execution on memory\ncorruptions. As part of that seccomp filter open(2) and openat(2) might\nbe only allowed in read-only mode (O_RDONLY), thus preventing git's\nsanitation and stopping the application.\n\nCheck before opening /dev/null to populate a possible non-present\nstandard file descriptor if actually any is missing.\n\nSigned-off-by: Christian Göttsche \n---\nAlternatively one could add a command line argument\n(`--no-stdfd-sanitization`).\n---\n setup.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex cefd5f6..2af7170 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1669,7 +1669,14 @@ 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+\tif (fcntl(0, F_GETFD) != -1 &&\n+\t fcntl(1, F_GETFD) != -1 &&\n+\t fcntl(2, F_GETFD) != -1)\n+\t\treturn;\n+\n+\tfd = xopen(\"/dev/null\", O_RDWR);\n \twhile (fd < 2)\n \t\tfd = xdup(fd);\n \tif (fd > 2)\n-- \n2.38.1\n\n"},{"id":"468549","messageId":"Y45tGLaJwg3Lt5uJ@tapette.crustytoothpaste.net","threadId":"58898","inReplyTo":"20221205190019.52829-1-cgzones@googlemail.com","subject":"Re: [PATCH] setup: avoid unconditional open with write flags","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-12-05T22:13:44Z","receivedAt":"2022-12-05T22:13:50Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-12-05 at 19:00:19, Christian Göttsche wrote:\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 non modifying git operations, like `git status` or `git\n> stash list`, which might be called by a parent to gather information\n> about the repository. That parent might run under a seccomp filter to\n> avoid accidental modification or unwanted command execution on memory\n> corruptions. As part of that seccomp filter open(2) and openat(2) might\n> be only allowed in read-only mode (O_RDONLY), thus preventing git's\n> sanitation and stopping the application.\n> \n> Check before opening /dev/null to populate a possible non-present\n> standard file descriptor if actually any is missing.\n\nI don't think this patch makes anything worse, and so I think it should\nbe fine as it is.\n\n_However_, I will say that `git status` is not a read-only command\nbecause it can write the index, and we aren't, in general, going to be\nable to promise that any portion of Git will work with only O_RDONLY\nfile descriptors. I suspect such a sandbox is going to result in a\nhighly broken Git in general, and so it wouldn't be a good idea.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"468551","messageId":"Y453yiYAifv/oV5n@nand.local","threadId":"58898","inReplyTo":"20221205190019.52829-1-cgzones@googlemail.com","subject":"Re: [PATCH] setup: avoid unconditional open with write flags","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-05T22:59:28Z","receivedAt":"2022-12-05T22:59:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Dec 05, 2022 at 08:00:19PM +0100, Christian Göttsche wrote:\n> @@ -1669,7 +1669,14 @@ 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\nSo before we would do one syscall here unconditionally. Then in the\ncommon case, we'll do another syscall to close the descriptor we just\nopened. IOW, ordinarily we expect this function to execute two syscalls.\n\n> +\tint fd;\n> +\n> +\tif (fcntl(0, F_GETFD) != -1 &&\n> +\t fcntl(1, F_GETFD) != -1 &&\n> +\t fcntl(2, F_GETFD) != -1)\n> +\t\treturn;\n\nBut under the same circumstances (i.e., where all three descriptors are\nalready valid), we now have to make three syscalls to determine the same\nfact.\n\n...So I'm not sure that I agree with brian's \"this isn't making anything\nworse\" statement earlier in the thread.\n\nIn practice, however, it appears to be basically undetectable. Here,\n'git.old' is the pre-image of this patch, and 'git.new' is the\npost-image. I figure that running 'git -h' is about the fastest thing I\ncould do:\n\n $ hyperfine -N -L V old,new './git.{V} -h'\n Benchmark 1: ./git.old -h\n Time (mean ± σ): 1.6 ms ± 0.2 ms [User: 1.1 ms, System: 0.4 ms]\n Range (min … max): 0.8 ms … 2.2 ms 1589 runs\n\n Benchmark 2: ./git.new -h\n Time (mean ± σ): 1.6 ms ± 0.2 ms [User: 1.1 ms, System: 0.4 ms]\n Range (min … max): 0.9 ms … 2.2 ms 1746 runs\n\n Summary\n './git.old -h' ran\n 1.00 ± 0.14 times faster than './git.new -h'\n\nSo it appears that the old version is ever-so-slightly faster than the\nnew one. But it's so noisy, and the regression is so small that it's\nhard to notice it at all.\n\nSo I wouldn't strongly oppose the patch based on those numbers, but in\nprinciple it seems flawed.\n\nThanks,\nTaylor\n"},{"id":"468563","messageId":"xmqq7cz5pf1w.fsf@gitster.g","threadId":"58898","inReplyTo":"Y453yiYAifv/oV5n@nand.local","subject":"Re: [PATCH] setup: avoid unconditional open with write flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-06T00:10:51Z","receivedAt":"2022-12-06T00:10:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> So it appears that the old version is ever-so-slightly faster than the\n> new one. But it's so noisy, and the regression is so small that it's\n> hard to notice it at all.\n>\n> So I wouldn't strongly oppose the patch based on those numbers, but in\n> principle it seems flawed.\n\nThanks for writing and reviewing.\n\nAs long as we were touching the function, I suspect that\nthe logic should become more like\n\n if (fd #0 is not open)\n\topen /dev/null read-only and give it to fd #0\n if (fd #1 is not open)\n\topen /dev/null write-only and give it to fd #1\n if (fd #2 is not open)\n\topen /dev/null write-only and give it to fd #2\n\nwith opening of /dev/null optimized not to happen when not needed.\n"},{"id":"468566","messageId":"Y46NaUjc+eqzSExR@nand.local","threadId":"58898","inReplyTo":"xmqq7cz5pf1w.fsf@gitster.g","subject":"Re: [PATCH] setup: avoid unconditional open with write flags","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-06T00:31:37Z","receivedAt":"2022-12-06T00:31:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Dec 06, 2022 at 09:10:51AM +0900, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > So it appears that the old version is ever-so-slightly faster than the\n> > new one. But it's so noisy, and the regression is so small that it's\n> > hard to notice it at all.\n> >\n> > So I wouldn't strongly oppose the patch based on those numbers, but in\n> > principle it seems flawed.\n>\n> Thanks for writing and reviewing.\n>\n> As long as we were touching the function, I suspect that\n> the logic should become more like\n>\n> if (fd #0 is not open)\n> \topen /dev/null read-only and give it to fd #0\n> if (fd #1 is not open)\n> \topen /dev/null write-only and give it to fd #1\n> if (fd #2 is not open)\n> \topen /dev/null write-only and give it to fd #2\n>\n> with opening of /dev/null optimized not to happen when not needed.\n\nYeah, that would work, and it has the added benefit of not opening fd #0\nwith O_RDWR (though I kind of doubt that such a thing matters in\npractice).\n\nBut it's still no better than the patch here in the happy case, since we\nstill have to perform three fcntl() checks to figure out that all three\ndescriptors are initialized as-expected (versus just one open() and\nclose()).\n\nSo I think your version is a slight improvement on Christian's, but I\nwould just as soon stick with what we have.\n\nThanks,\nTaylor\n"},{"id":"468568","messageId":"xmqq359tpdoy.fsf@gitster.g","threadId":"58898","inReplyTo":"Y46NaUjc+eqzSExR@nand.local","subject":"Re: [PATCH] setup: avoid unconditional open with write flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-06T00:40:13Z","receivedAt":"2022-12-06T00:41:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> But it's still no better than the patch here in the happy case, since we\n> still have to perform three fcntl() checks to figure out that all three\n> descriptors are initialized as-expected (versus just one open() and\n> close()).\n>\n> So I think your version is a slight improvement on Christian's, but I\n> would just as soon stick with what we have.\n\nI am OK as long as there is a workaround available to Christian's\nuse case without changing \"git\" at all. If Christian can tighten\nthe environment into somewhat unnatural \"opening writable FD is a\nfailure\" way, I suspect such a jail can be augmented to further to\nallow opening /dev/null and other \"selected\" files writable, so I\nwouldn't worry too much if we dropped this patch entirely.\n\nThanks.\n\n"},{"id":"468574","messageId":"Y46dHWVaU00WFL+f@coredump.intra.peff.net","threadId":"58898","inReplyTo":"Y45tGLaJwg3Lt5uJ@tapette.crustytoothpaste.net","subject":"Re: [PATCH] setup: avoid unconditional open with write flags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-06T01:38:37Z","receivedAt":"2022-12-06T01:39:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 05, 2022 at 10:13:44PM +0000, brian m. carlson wrote:\n\n> _However_, I will say that `git status` is not a read-only command\n> because it can write the index, and we aren't, in general, going to be\n> able to promise that any portion of Git will work with only O_RDONLY\n> file descriptors. I suspect such a sandbox is going to result in a\n> highly broken Git in general, and so it wouldn't be a good idea.\n\nI wonder if \"git status\" might work OK in a sandbox, because it should\nquietly skip the on-disk index refresh if something fails. That is, it's\nsupposed to work in a read-only repository. As long as the sandbox just\nreturns an error when opening the lockfile (and not, say, killing the\nprocess), it would look the same to Git.\n\n-Peff\n"},{"id":"468627","messageId":"CAJ2a_DeNQ0KfkKLzhsxA0UtixmLvNxgzR=KC0UjMwVx1WXJMEg@mail.gmail.com","threadId":"58898","inReplyTo":"Y46dHWVaU00WFL+f@coredump.intra.peff.net","subject":"Re: [PATCH] setup: avoid unconditional open with write flags","fromName":"Christian Göttsche","fromEmail":"cgzones@googlemail.com","sentAt":"2022-12-06T16:15:03Z","receivedAt":"2022-12-06T16:18:38Z","isPatch":true,"sender":{"key":"cgzones@googlemail.com","avatar":null},"body":"> But it's still no better than the patch here in the happy case, since we\n> still have to perform three fcntl() checks to figure out that all three\n> descriptors are initialized as-expected (versus just one open() and\n> close()).\n\nAn alternative to performing three syscalls fot the check one could call\nopen(2) with O_RDONLY (O_PATH would also work, but seems not\nyet to be used in the git source) on a common path (\"/\", \"/dev/null\", ...)\nand skip the sanitization if the returned descriptor is greater than 2.\nThis would lead to two (open + close) syscalls in the common case,\nsame as current.\n\n> If Christian can tighten\n> the environment into somewhat unnatural \"opening writable FD is a\n> failure\" way, I suspect such a jail can be augmented to further to\n> allow opening /dev/null and other \"selected\" files writable, so I\n> wouldn't worry too much if we dropped this patch entirely.\n\nThe seccomp filter only gets the address of the memory where the path\nis stored, so simple allow-listing paths is not possible. And even on\ninspection of the path one would need to avoid toctou attacks (the\nfilter seeing a different memory content at check time than the kernel\nat use time).\n"},{"id":"468634","messageId":"f49b915e-46d1-7e53-ae4f-b01c2028b70d@web.de","threadId":"58898","inReplyTo":"20221205190019.52829-1-cgzones@googlemail.com","subject":"Re: [PATCH] setup: avoid unconditional open with write flags","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-06T19:47:09Z","receivedAt":"2022-12-06T19:47:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.12.22 um 20:00 schrieb Christian Göttsche:\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 non modifying git operations, like `git status` or `git\n> stash list`, which might be called by a parent to gather information\n> about the repository. That parent might run under a seccomp filter to\n> avoid accidental modification or unwanted command execution on memory\n> corruptions. As part of that seccomp filter open(2) and openat(2) might\n> be only allowed in read-only mode (O_RDONLY), thus preventing git's\n> sanitation and stopping the application.\n>\n> Check before opening /dev/null to populate a possible non-present\n> standard file descriptor if actually any is missing.\n>\n> Signed-off-by: Christian Göttsche \n> ---\n> Alternatively one could add a command line argument\n> (`--no-stdfd-sanitization`).\n> ---\n> setup.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/setup.c b/setup.c\n> index cefd5f6..2af7170 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1669,7 +1669,14 @@ 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> +\tif (fcntl(0, F_GETFD) != -1 &&\n> +\t fcntl(1, F_GETFD) != -1 &&\n> +\t fcntl(2, F_GETFD) != -1)\n> +\t\treturn;\n> +\n> +\tfd = xopen(\"/dev/null\", O_RDWR);\n> \twhile (fd < 2)\n> \t\tfd = xdup(fd);\n> \tif (fd > 2)\n\nIf read-only access is allowed, how about this?\n\ndiff --git a/setup.c b/setup.c\nindex cefd5f63c4..0f52c51759 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1669,7 +1669,12 @@ 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 = xopen(\"/dev/null\", O_RDONLY);\n+\tif (fd > 0)\n+\t\tclose(fd);\n+\tif (fd > 2)\n+\t\treturn;\n+\tfd = xopen(\"/dev/null\", O_WRONLY);\n \twhile (fd < 2)\n \t\tfd = xdup(fd);\n \tif (fd > 2)\n\nRequires an extra open/close pair if fd 0 is already open, but no extra\nsyscalls if 0, 1 and 2 are all open.\n\nCan opening /dev/null (or NUL on Windows) multiple times instead of dup'ing\ncause issues? Can we e.g. lock ourselves out?\n\nRené\n\n"},{"id":"468649","messageId":"xmqqr0xc85l4.fsf@gitster.g","threadId":"58898","inReplyTo":"f49b915e-46d1-7e53-ae4f-b01c2028b70d@web.de","subject":"Re: [PATCH] setup: avoid unconditional open with write flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-06T23:39:35Z","receivedAt":"2022-12-06T23:39:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Can opening /dev/null (or NUL on Windows) multiple times instead of dup'ing\n> cause issues? Can we e.g. lock ourselves out?\n\n258e93a1 (daemon: if one of the standard fds is missing open it to\n/dev/null, 2006-07-13) unfortunately does not explain why it was\ndone this way.\n\nGiven that \"command /dev/null 2>/dev/null\" should work fine,\nI suspect that the \"once we open a throw-away descriptor, instead of\nopening the same file again, dup the fd and reuse\" was done not for\ncorrectness but for performance, under the assumption that open() is\nmore expensive?\n"}]}