{"thread":{"id":"53866","subject":"Issue with insufficient permission for adding an object to repository database .git/objects","startedAt":"2020-07-16T14:25:54Z","lastAt":"2020-07-17T19:53:26Z","messageCount":3,"participants":["LTPCGO | George","Chris Torek","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"401635","messageId":"d6bbe71e47dcfd7ef31213537eeb3f05@ltpcgo.com","threadId":"53866","inReplyTo":null,"subject":"Issue with insufficient permission for adding an object to repository database .git/objects","fromName":"LTPCGO | George","fromEmail":"george@ltpcgo.com","sentAt":"2020-07-16T14:25:45Z","receivedAt":"2020-07-16T14:25:54Z","isPatch":false,"sender":{"key":"george@ltpcgo.com","avatar":null},"body":"See discussion here \nhttps://groups.google.com/forum/?fromgroups#!topic/git-users/35HBb76oA6c \nand https://github.com/trapexit/mergerfs/issues/626\n\nOn certain file systems, there are issues with staging a commit because \na call to open a file stored under .git/objects fails.  It has been \nbrought up in this discussion group previously.\n\nI have attached a fix below, but it would be much better to fix in the \ncode.  I am curious first, before proposing a fix in the code (although \nI can't find the specific call in the source at \nhttps://github.com/git/git ), what the reasoning is for the current \npermissions check on the call rather than checking the contents of the \nopened tmp file.\n\ncc -Wall -O3 -D_GNU_SOURCE -fPIC -c -o githack.o githack.c; gcc -o \ngithack.so -nostartfiles -shared githack.o -ldl;\n\n\n\nLD_PRELOAD=./githack.so git commit -a -m \"Some new commit\"\n\n\n\nThe code is below:\n\n#include <stdio.h>\n#include <stdlib.h>\n#include <string.h>\n#include <sys/stat.h>\n#include <sys/types.h>\n//#define openat ignorethisopen\n#define open ignorethisopen\n#define open64 ignorethisopen64\n#include <fcntl.h>\n//#undef openat\n#undef open\n#undef open64\n#include <dlfcn.h>\n\n/*\n        'strace git ...' will show git fail on an openat() command\n        this is probably implemented as open64() on your system\n        you can confirm this by use of 'ltrace git ...'\n        you may also need to adjust the oflag comparison of 194\n*/\n\n/*static int (*___openat)(int, char *, int, mode_t);*/\nstatic int (*___open)(const char *, int, mode_t);\nstatic int (*___open64)(const char *, int, mode_t);\n\nstatic void* dlwrap(const char *fn)\n{\n        const char *e;\n        void *p = dlsym(RTLD_NEXT, fn);\n        if ((e=dlerror())!=0)  fprintf(stderr, \"dlsym(RTLD_NEXT,'%s'): \n%s\\r\\n\", fn, e);\n        return p;\n}\n\nvoid _init(void)\n{\n        ___open = dlwrap(\"open\");\n        ___open64 = dlwrap(\"open64\");\n}\n\n/*int openat(int dirfd, const char *pathname, int oflag, mode_t mode)*/\nint open(const char *pathname, int oflag, mode_t mode)\n{\n        if (oflag && oflag == 194)\n               return ___open(pathname, oflag, S_IRWXU);\n        return ___open(pathname, oflag, mode);\n}\n\nint open64(const char *pathname, int oflag, mode_t mode)\n{\n        if (oflag && oflag == 194)\n               return ___open64(pathname, oflag, S_IRWXU);\n        return ___open64(pathname, oflag, mode);\n}\n\n🖳 LTPCGO 🖳\nNext-level security\nDr. George O’Neill\n+44 7717 318 220\ngeorge@ltpcgo.com\nhttps://www.ltpcgo.com/\n"},{"id":"401638","messageId":"CAPx1GvfKxu8gwbp0Gn2dBf9th874skKjD-echeAFr7_77o8FYw@mail.gmail.com","threadId":"53866","inReplyTo":"d6bbe71e47dcfd7ef31213537eeb3f05@ltpcgo.com","subject":"Re: Issue with insufficient permission for adding an object to repository database .git/objects","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-07-16T15:41:40Z","receivedAt":"2020-07-16T15:41:54Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Thu, Jul 16, 2020 at 7:30 AM LTPCGO | George <george@ltpcgo.com> wrote:\n> I have attached a fix below, but it would be much better to fix in the\n> code.  I am curious first, before proposing a fix in the code (although\n> I can't find the specific call in the source at\n> https://github.com/git/git ), what the reasoning is for the current\n> permissions check on the call rather than checking the contents of the\n> opened tmp file.\n\nThis is not in fact a bug in Git (which assumes POSIX semantics).\nGit is not doing its own permissions checking here.  Rather, it is\na problem with the way the NFS software you are using attempts,\nbut fails, to emulate the POSIX requirements.\n\nWhat Git does is this:\n\n * form the name of a file that we expect not to exist\n * use an open() system call in this way:\n\n     fd = open(path, O_CREAT | O_EXCL | O_RDWR, 0444)\n\nNote that this is a single, atomic system call that asks\nthe OS to:\n\n 1. make sure the path does not exist currently -- if it\n    does, return an error;\n 2. create that path and open the resulting file for\n    reading and writing, but make sure that no one else\n    may write to that path.\n\nOn a local file system, this really is a single atomic\noperation: either the path does not exist *and* you are\nallowed to create it *and* the creation succeeds *and*\nyou now have a writable file-handle for a read-only file;\nor, any one of the \"and\"s above has failed (file already\nexisted, you aren't allowed to create here, etc).\n\nThe underlying file is, in effect, write-once: *one* user\nmay write to the file, once, then close it.\n\nSome NFS implementations, however, simply don't support this\noperation as a single atomic operation.  In a best-effort attempt\nto provide it anyway, they will do:\n\n 1. test to see if the file exists, perhaps by doing an\n    open with O_CREAT|O_EXCL themselves, but in an internal\n    way that fails to obtain the file *handle*;\n 2. chmod the underlying file to the desired mode;\n 3. open the file again, this time to get a file handle.\n\nIn this case, the second open fails *because* the file is\nset up to be \"write-once\": you cannot get a writable file\nhandle after the chmod.  But *Git* did not do the chmod.\n\nGit could perhaps open these temporary files with mode\n0644 instead of mode 0444, relying on the O_EXCL part and\nself-coordination, then follow up later with an fchmod()\nto set the mode to 0444.  But this opens a small window\nfor badly-behaved programs to obtain a file handle that\ncould be used to write to the Git object.\n\n(Note that O_EXCL does not work well in general on NFS:\nsome systems do support it, but you're somewhat at the\nmercy of your implementation.  See also\nhttps://stackoverflow.com/questions/3406712/open-o-creat-o-excl-on-nfs-in-linux\nand other reports you'll find by searching for \"NFS O_EXCL\"\non google.)\n\nChris\n"},{"id":"401710","messageId":"xmqq365pex5b.fsf@gitster.c.googlers.com","threadId":"53866","inReplyTo":"CAPx1GvfKxu8gwbp0Gn2dBf9th874skKjD-echeAFr7_77o8FYw@mail.gmail.com","subject":"Re: Issue with insufficient permission for adding an object to repository database .git/objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-17T19:53:20Z","receivedAt":"2020-07-17T19:53:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Torek <chris.torek@gmail.com> writes:\n\n> What Git does is this:\n>\n>  * form the name of a file that we expect not to exist\n>  * use an open() system call in this way:\n>\n>      fd = open(path, O_CREAT | O_EXCL | O_RDWR, 0444)\n>\n> Note that this is a single, atomic system call that asks\n> the OS to:\n>\n>  1. make sure the path does not exist currently -- if it\n>     does, return an error;\n>  2. create that path and open the resulting file for\n>     reading and writing, but make sure that no one else\n>     may write to that path.\n>\n> On a local file system, this really is a single atomic\n> operation: either the path does not exist *and* you are\n> allowed to create it *and* the creation succeeds *and*\n> you now have a writable file-handle for a read-only file;\n> or, any one of the \"and\"s above has failed (file already\n> existed, you aren't allowed to create here, etc).\n\nThat's a very nice and clealy written analysis.  What you outlined\nabove is exactly why we use these flags and create the file\nread-only from the get go.\n\nHaving said that, most of these \"create atomically\" that are coming\nvia the tempfile API could use a looser 0644, as the primary reason\nwhy we are careful is not to protect against bad actors but to\nprotect against ourselves (another process or another thread) racing\nto create the same object---in other words, we are happy as long as\nO_CREAT|O_EXCL is honored, and \"link into the final place and then\nunlink the tempfile\" or \"rename into the final place\" patterns that\nare used to conclude the temporary file obtained via the tempfile\nAPI are reliable on the target filesystem.\n\nThe resulting file being read-only with 0444 (limited further by\numask) is not all that important.  It is certainly possible to\ndestroy an owner-writable file by redirecting into it, and making\nthe files read-only may prevent such an accident from happening, but\nimmediate parent directories of these files have to be at least\nowner-writable, so it is just as easy to destroy such a read-only\nfile by unlinking it from its parent directory.\n\nI may be forgetting important reasons why we insist read-only files\nin $GIT_DIR for objects, packs and commit-graph files (there may be\nothers), but otherwise I do not have a strong objection to a patch\nwith a well written log message that loosens these 0444 modes that\nare (eventually) given to git_mkstemp_mode() to 0644.\n\nThanks.\n"}]}