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

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

[Jonathan Word]

All,

I recently discovered that `git config` does not respect read-only files.

This caused unexpected difficulty in managing the global .gitconfig
for a system account shared by a large team. A team member was able to
execute a `git config --global` command without any notice or warning
that the underlying config file had been marked read-only in an
attempt to prevent unintentional changes. If instead git had raised a
warning saying that the "gitconfig is read-only" this would have
prevented that team member from accidentally breaking our git config.


Bug detail:

Due to the implementation strategy of
config::git_config_set_multivar_in_file_gently (
https://github.com/git/git/blob/5b33cb1fd733f581da07ae8afa7e9547eafd248e/config.c#L2074
) the file permissions of the target .gitconfig file are not
respected.


Proposal:

Part 1) Add a .gitconfig variable to respect a read-only gitconfig
file and optional "--force" override option for the `git config`
command

Such a gitconfig variable could be defined as:
config.respectFileMode: [ "never", "allow-override", "always" ]

Where:
* never - read-only file mode of config files are ignored (aka:
existing behavior)
* allow-override - read-only file mode of config files is respected
unless the user provides a "--force" option to `git config`
* always - read-only file mode of config files is respected (and the
"--force" option does not work)

Part 2) Change config::git_config_set_multivar_in_file_gently (
https://github.com/git/git/blob/5b33cb1fd733f581da07ae8afa7e9547eafd248e/config.c#L2077
) to verify write permissions on the destination depending on the
specified config.respectFileMode variable and "--force" option.



I think that this is a reasonably sized change that enables users to
opt-in to a 'strict mode' while preserving current behavior.


Thoughts?


Tested with:
OS: Linux
Version: 2.9.0 (issue exists in current master branch)

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

[Markus Hitter]

Am 08.11.2016 um 16:22 schrieb Jonathan Word:
> Proposal:
> 
> Part 1) Add a .gitconfig variable to respect a read-only gitconfig
> file and optional "--force" override option for the `git config`
> command
> 
> Such a gitconfig variable could be defined as:
> config.respectFileMode: [ "never", "allow-override", "always" ]
> [...]
> Thoughts?

I'd consider disrespecting file permissions to be a bug. Only very few tools allow to do so ('rm' is the only other one coming to mind right now), for good reason. If they do, only with additional parameters or by additional user interaction. Git should follow this strategy.

Which means: respect file permissions, no additional config variable and only if there's very substantial reason, add a --force. KISS.

That said, disrespecting permissions requires additional code, so it'd be interesting to know why this code was added. The relevant commit in the git.git repo should tell.


Markus

-- 
- - - - - - - - - - - - - - - - - - -
Dipl. Ing. (FH) Markus Hitter
http://www.jump-ing.de/

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

[Jonathan Word]

I proposed a variant that would be fully backwards-compatible (don't
know who might rely on the functionality http://xkcd.com/1172/ )
however I'd be happy to see the change without additional config +1
... that's a call for this list as maintainers.

The root of the issue is that tempfile::rename_tempfile (
https://github.com/git/git/blob/35f6318d44379452d8d33e880d8df0267b4a0cd0/tempfile.c#L288
) relies on http://man7.org/linux/man-pages/man2/rename.2.html which,
only requires directory write permissions - not file write
permissions. As you point out 'rm' is another example of this paradigm
and it works exactly the same way.

The point of confusion to users ( / my team) is that `git config`
gives the appearance of editing / modifying the .gitconfig file
in-place (where file permissions would be respected) however the
actual implementation performs the equivalent of a rm+mv which only
respects directory permissions.

The `git config` command is only one of many that leverage that
rename_tempfile function, if opting to respect file-level permissions
across the board then the desired change is probably at that level
rather than in config::git_config_set_multivar_in_file_gently which
would only add respect for file-level permissions to the one command.

Cheeers,


On Tue, Nov 8, 2016 at 11:49 AM, Markus Hitter <mah@jump-ing.de> wrote:
> Am 08.11.2016 um 16:22 schrieb Jonathan Word:
>> Proposal:
>>
>> Part 1) Add a .gitconfig variable to respect a read-only gitconfig
>> file and optional "--force" override option for the `git config`
>> command
>>
>> Such a gitconfig variable could be defined as:
>> config.respectFileMode: [ "never", "allow-override", "always" ]
>> [...]
>> Thoughts?
>
> I'd consider disrespecting file permissions to be a bug. Only very few tools allow to do so ('rm' is the only other one coming to mind right now), for good reason. If they do, only with additional parameters or by additional user interaction. Git should follow this strategy.
>
> Which means: respect file permissions, no additional config variable and only if there's very substantial reason, add a --force. KISS.
>
> That said, disrespecting permissions requires additional code, so it'd be interesting to know why this code was added. The relevant commit in the git.git repo should tell.
>
>
> Markus
>
> --
> - - - - - - - - - - - - - - - - - - -
> Dipl. Ing. (FH) Markus Hitter
> http://www.jump-ing.de/

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

[Jeff King]

On Tue, Nov 08, 2016 at 12:18:22PM -0500, Jonathan Word wrote:

> The point of confusion to users ( / my team) is that `git config`
> gives the appearance of editing / modifying the .gitconfig file
> in-place (where file permissions would be respected) however the
> actual implementation performs the equivalent of a rm+mv which only
> respects directory permissions.

The reason for the tmpfile/rename is that git-config actually takes a
dot-lock on the file while writing it. Simultaneous writers are blocked,
and simultaneous readers see an atomic view of the file (either the
state before or after the write, but never a half-written file).  Most
of git's file-writes are done this way.

> The `git config` command is only one of many that leverage that
> rename_tempfile function, if opting to respect file-level permissions
> across the board then the desired change is probably at that level
> rather than in config::git_config_set_multivar_in_file_gently which
> would only add respect for file-level permissions to the one command.

I am not convinced this is a code problem and not simply a documentation
issue, but if you wanted to add an option to try to respect file
permissions, then yes, I agree it should be done across the board.
Probably converting "rename(from, to)" to first check "access(to,
W_OK)". That's racy, but it's the best we could do.

-Peff

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

[Junio C Hamano]

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.

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

[Jeff King]

On Tue, Nov 08, 2016 at 05:22:52PM -0800, Junio C Hamano wrote:

> 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?

Yeah, that is a lot friendlier, as it prevents the caller from doing
work (which may even involve the user typing things!) when it is clear
that we would fail the final step anyway.

-Peff

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

[Jonathan Word]

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