Issue with insufficient permission for adding an object to repository database .git/objects

See discussion here https://groups.google.com/forum/?fromgroups#!topic/git-users/35HBb76oA6c and https://github.com/trapexit/mergerfs/issues/626

On certain file systems, there are issues with staging a commit because a call to open a file stored under .git/objects fails. It has been brought up in this discussion group previously.

I have attached a fix below, but it would be much better to fix in the code. I am curious first, before proposing a fix in the code (although I can't find the specific call in the source at https://github.com/git/git ), what the reasoning is for the current permissions check on the call rather than checking the contents of the opened tmp file.

cc -Wall -O3 -D_GNU_SOURCE -fPIC -c -o githack.o githack.c; gcc -o githack.so -nostartfiles -shared githack.o -ldl;

LD_PRELOAD=./githack.so git commit -a -m "Some new commit"

The code is below:

#include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/stat.h> #include <sys/types.h> //#define openat ignorethisopen #define open ignorethisopen #define open64 ignorethisopen64 #include <fcntl.h> //#undef openat #undef open #undef open64 #include <dlfcn.h>

/*
        'strace git ...' will show git fail on an openat() command
        this is probably implemented as open64() on your system
        you can confirm this by use of 'ltrace git ...'
        you may also need to adjust the oflag comparison of 194
*/

/*static int (*___openat)(int, char *, int, mode_t);*/ static int (*___open)(const char *, int, mode_t); static int (*___open64)(const char *, int, mode_t);

static void* dlwrap(const char *fn)
{
        const char *e;
        void *p = dlsym(RTLD_NEXT, fn);
        if ((e=dlerror())!=0)  fprintf(stderr, "dlsym(RTLD_NEXT,'%s'): 
%s\r\n", fn, e);
        return p;
}

void _init(void)
{
        ___open = dlwrap("open");
        ___open64 = dlwrap("open64");
}

/*int openat(int dirfd, const char *pathname, int oflag, mode_t mode)*/
int open(const char *pathname, int oflag, mode_t mode)
{
        if (oflag && oflag == 194)
               return ___open(pathname, oflag, S_IRWXU);
        return ___open(pathname, oflag, mode);
}

int open64(const char *pathname, int oflag, mode_t mode)
{
        if (oflag && oflag == 194)
               return ___open64(pathname, oflag, S_IRWXU);
        return ___open64(pathname, oflag, mode);
}

🖳 LTPCGO 🖳 Next-level security Dr. George O’Neill +44 7717 318 220 george@ltpcgo.com https://www.ltpcgo.com/

On Thu, Jul 16, 2020 at 7:30 AM LTPCGO | George <george@ltpcgo.com> wrote:

This is not in fact a bug in Git (which assumes POSIX semantics). Git is not doing its own permissions checking here. Rather, it is a problem with the way the NFS software you are using attempts, but fails, to emulate the POSIX requirements.

What Git does is this:

 * form the name of a file that we expect not to exist
 * use an open() system call in this way:

     fd = open(path, O_CREAT | O_EXCL | O_RDWR, 0444)

Note that this is a single, atomic system call that asks the OS to:

 1. make sure the path does not exist currently -- if it
    does, return an error;
 2. create that path and open the resulting file for
    reading and writing, but make sure that no one else
    may write to that path.

On a local file system, this really is a single atomic operation: either the path does not exist *and* you are allowed to create it *and* the creation succeeds *and* you now have a writable file-handle for a read-only file; or, any one of the "and"s above has failed (file already existed, you aren't allowed to create here, etc).

The underlying file is, in effect, write-once: *one* user may write to the file, once, then close it.

Some NFS implementations, however, simply don't support this operation as a single atomic operation. In a best-effort attempt to provide it anyway, they will do:

 1. test to see if the file exists, perhaps by doing an
    open with O_CREAT|O_EXCL themselves, but in an internal
    way that fails to obtain the file *handle*;
 2. chmod the underlying file to the desired mode;
 3. open the file again, this time to get a file handle.

In this case, the second open fails *because* the file is set up to be "write-once": you cannot get a writable file handle after the chmod. But *Git* did not do the chmod.

Git could perhaps open these temporary files with mode 0644 instead of mode 0444, relying on the O_EXCL part and self-coordination, then follow up later with an fchmod() to set the mode to 0444. But this opens a small window for badly-behaved programs to obtain a file handle that could be used to write to the Git object.

(Note that O_EXCL does not work well in general on NFS: some systems do support it, but you're somewhat at the mercy of your implementation. See also https://stackoverflow.com/questions/3406712/open-o-creat-o-excl-on-nfs-in-linux and other reports you'll find by searching for "NFS O_EXCL" on google.)

Chris

Chris Torek <chris.torek@gmail.com> writes:

That's a very nice and clealy written analysis. What you outlined above is exactly why we use these flags and create the file read-only from the get go.

Having said that, most of these "create atomically" that are coming via the tempfile API could use a looser 0644, as the primary reason why we are careful is not to protect against bad actors but to protect against ourselves (another process or another thread) racing to create the same object---in other words, we are happy as long as O_CREAT|O_EXCL is honored, and "link into the final place and then unlink the tempfile" or "rename into the final place" patterns that are used to conclude the temporary file obtained via the tempfile API are reliable on the target filesystem.

The resulting file being read-only with 0444 (limited further by umask) is not all that important. It is certainly possible to destroy an owner-writable file by redirecting into it, and making the files read-only may prevent such an accident from happening, but immediate parent directories of these files have to be at least owner-writable, so it is just as easy to destroy such a read-only file by unlinking it from its parent directory.

I may be forgetting important reasons why we insist read-only files in $GIT_DIR for objects, packs and commit-graph files (there may be others), but otherwise I do not have a strong objection to a patch with a well written log message that loosens these 0444 modes that are (eventually) given to git_mkstemp_mode() to 0644.

Thanks.