git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment
Date: Mon, 9 May 2022 23:39:15 +0200	[thread overview]
Message-ID: <20220509213915.GA2043@szeder.dev> (raw)
In-Reply-To: <xmqqee1il09v.fsf@gitster.g>

On Wed, Apr 27, 2022 at 01:49:32PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like
> > $PATH does), that would have been absolutely necessary to document
> > how it works, but GIT_CONFIG_* is merely an implementation detail of
> > how "git -c var=val" works and I am not sure if it is even a good
> > idea to hardcode how they happen to work like these tests.  The only
> > thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are
> > used internally by the implementation and they should not muck with
> > it, no?
> 
> I misremembered.  GIT_CONFIG_COUNT and stuff are usable by end user
> scripts, but then ...
> 
> > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
> > index 6d764fe0cc..ae0e2e3bdb 100644
> > --- a/Documentation/config/safe.txt
> > +++ b/Documentation/config/safe.txt
> > @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a
> >  `safe.directory` entry with an empty value.
> >  +
> >  This config setting is only respected when specified in a system or global
> > -config, not when it is specified in a repository config or via the command
> > -line option `-c safe.directory=<path>`.
> > +config, not when it is specified in a repository config, via the command
> > +line option `-c safe.directory=<path>`, or in environment variables.
> 
> ... this part must clarify what environment variables it is talking
> about.
> 
>     ... via the command line option `-c safe.directory=<path>`, or
>     via the GIT_CONFIG_{KEY,VALUE} mechanism.

Well, the proposed phrasing covers all environment variables that
affect the configuration.

It doesn't feel right to explicitly list all those variables,
including GIT_CONFIG_PARAMETERS, because that vairable is such an
implementation detail that it isn't even mentioned elsewhere in the
documentation at all.  OTOH, listing only some of the ignored
variables but omitting others doesn't feel quite right either, hence
the general "in environment variables".

> or something, perhaps.  I actually do think it is a useful addition
> to have GIT_SAFE_DIRECTORIES environment variable that should NOT be
> ignored.

Is it safe to have such an environment variable?

So, it's quite clear why 'safe.directory' is ignored in the repository
config: it can be under control of someone else, and thus a malicious
repositoy could trivially safe-list itself.  However, it's unclear (to
me) why 'git -c safe.directory=...' is ignored: 8959555cee
(setup_git_directory(): add an owner check for the top-level
directory, 2022-03-02) only states that it's ignored, but doesn't
justify why.  Now, let's suppose that there was a compelling reason to
do so, e.g. in some way that I don't readily see it can be spoofed and
thus could be abused by an attacker to safe-list a malicious
repository.  If that were indeed the case, then couldn't
'GIT_SAFE_DIRECTORIES=/path/... git cmd' could be spoofed and abused
just as easily?

Or to approach it from the opposite direction: if such a
GIT_SAFE_DIRECTORIES environment variable is safe, then why should we
ignore 'safe.directory' in the command line, or in the environment
for that matter?


  parent reply	other threads:[~2022-05-09 21:40 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-13 15:32 [PATCH 0/3] Updates to the safe.directory config option Derrick Stolee via GitGitGadget
2022-04-13 15:32 ` [PATCH 1/3] t0033: add tests for safe.directory Derrick Stolee via GitGitGadget
2022-04-13 16:24   ` Junio C Hamano
2022-04-13 16:29     ` Derrick Stolee
2022-04-13 19:16   ` Ævar Arnfjörð Bjarmason
2022-04-13 19:46     ` Junio C Hamano
2022-04-13 19:52     ` Derrick Stolee
2022-04-13 15:32 ` [PATCH 2/3] setup: fix safe.directory key not being checked Matheus Valadares via GitGitGadget
2022-04-13 16:34   ` Junio C Hamano
2022-04-13 15:32 ` [PATCH 3/3] setup: opt-out of check with safe.directory=* Derrick Stolee via GitGitGadget
2022-04-13 16:40   ` Junio C Hamano
2022-04-13 16:15 ` [PATCH 0/3] Updates to the safe.directory config option Junio C Hamano
2022-04-13 16:25   ` Derrick Stolee
2022-04-13 16:44     ` Junio C Hamano
2022-04-13 20:27       ` Junio C Hamano
2022-04-13 21:25         ` Taylor Blau
2022-04-13 21:45           ` Junio C Hamano
2022-04-27 17:06 ` [PATCH 0/3] t0033-safe-directory: test improvements and a documentation clarification SZEDER Gábor
2022-04-27 17:06   ` [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir SZEDER Gábor
2022-05-09 22:27     ` Taylor Blau
2022-05-10  6:04     ` Carlo Marcelo Arenas Belón
2022-04-27 17:06   ` [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored SZEDER Gábor
2022-04-27 20:37     ` Junio C Hamano
2022-04-29 16:12       ` Derrick Stolee
2022-04-29 17:57         ` Junio C Hamano
2022-04-29 19:06           ` SZEDER Gábor
2022-04-29 19:19             ` Derrick Stolee
2022-05-10 18:33               ` SZEDER Gábor
2022-05-10 19:55                 ` Taylor Blau
2022-05-10 20:06                   ` Carlo Marcelo Arenas Belón
2022-05-10 20:11                     ` Taylor Blau
2022-05-10 20:18                   ` Eric Sunshine
2022-04-27 17:06   ` [PATCH 3/3] safe.directory: document and check that it's ignored in the environment SZEDER Gábor
2022-04-27 20:42     ` Junio C Hamano
2022-04-27 20:49       ` Junio C Hamano
2022-04-29 16:13         ` Derrick Stolee
2022-05-09 21:39         ` SZEDER Gábor [this message]
2022-05-09 21:45           ` Junio C Hamano
2022-05-10 18:48             ` SZEDER Gábor

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220509213915.GA2043@szeder.dev \
    --to=szeder.dev@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).