Re: Bug: git config does not respect read-only .gitconfig file

From: Jonathan Word <argoday@argoday.com>
Date: Nov 9, 2016, 13:51 UTC
Message-ID: <CAD9aWCi5m_eJ==yC6_X-O_Xc+nyFczKkiEmqFSEAHKyNt2g-ZQ@mail.gmail.com>
In-Reply-To: <xmqqk2cdbg5v.fsf@gitster.mtv.corp.google.com>

> It is unreasonable to drop the write-enable bit of
> a file in a writable directory and expect it to stay unmodified. The
> W-bit on the file is not usable as a security measure, and we do not
> use it as such.

The point here is not a matter of security - it is of expectations.

When a user drops write access on the global ~/.gitconfig I think a reasonable user would expect future `git config --global` calls to fail by default. The possibility of an override is a different matter, and my initial proposal included the details of enabling direct override. I don't think there is any presumption that this is a security related discussion.

> I do not offhand know how much a new feature "this repository can be
> modified by pushing into and fetching from, but its configuration
> cannot be modified" is a sensible thing to have.

I agree that per-repository files almost never run into an issue with this. Our problem is strictly with the global ~/.gitconfig which in our use case is owned by a shared system account and used implicitly by many developers. Thus any one of those devs can call `git config` without any signal that they are changing something that ought not to be changed and should think carefully.

This would be equivalent to dropping write access to a file that your account owns so that vi / emacs / etc.. will warn that the file is read-only before modifying it (useful for any number of sensitive files). Obviously from a security perspective you have a number of means of potential override, however all require additional steps that surface the initial intention that the file should not change - or should only change rarely after additional confirmation.

> perhaps the lock_file()
> function can have access(path, W_OK) check before it returns a
> tempfile that has been successfully opened?

That sounds ideal

On Tue, Nov 8, 2016 at 8:22 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> Probably converting "rename(from, to)" to first check "access(to,
>> W_OK)". That's racy, but it's the best we could do.
>
> Hmph, if these (possibly problematic) callers are all following the
> usual "lock, write to temp, rename" pattern, perhaps the lock_file()
> function can have access(path, W_OK) check before it returns a
> tempfile that has been successfully opened?
>
> Having said that, I share your assessment that this is not a code or
> design problem.  It is unreasonable to drop the write-enable bit of
> a file in a writable directory and expect it to stay unmodified. The
> W-bit on the file is not usable as a security measure, and we do not
> use it as such.
>
> I do not offhand know how much a new feature "this repository can be
> modified by pushing into and fetching from, but its configuration
> cannot be modified" is a sensible thing to have.  But it is quite
> clear that even if we were to implement such feature, we wouldn't be
> using W-bit on .git/config to signal that.
>