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

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

[LTPCGO | George]

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/

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

[Chris Torek]

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

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

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

[Junio C Hamano]

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

> 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).

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.