git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Derrick Stolee <derrickstolee@github.com>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>, git@vger.kernel.org
Cc: philipoakley@iee.email, me@ttaylorr.com,
	"Guy Maurel" <guy.j@maurel.de>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Randall Becker" <rsbecker@nexbridge.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Subject: Re: [RFC PATCH] git-compat-util: avoid failing dir ownership checks if running priviledged
Date: Tue, 26 Apr 2022 15:48:33 -0400	[thread overview]
Message-ID: <9658dea7-d421-b238-113d-c7b83eca4569@github.com> (raw)
In-Reply-To: <20220426183105.99779-1-carenas@gmail.com>

On 4/26/2022 2:31 PM, Carlo Marcelo Arenas Belón wrote:
> bdc77d1d685 (Add a function to determine whether a path is owned by the
> current user, 2022-03-02) checks for the effective uid of the running
> process using geteuid() but didn't account for cases where that uid was
> root (because git was invoked through sudo or doas) and the effetive id

s/effetive/effective

> that repositiry trusted for its config was different, therefore failing

s/repositiry/repository

> the following common call:
> 
>   guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty
>   [sudo] password for guy:
>   fatal: unsafe repository ('/home/guy/Software/uncrustify' is owned by someone else)
> 
> Attempt to detect those cases by using the environment variables that
> those tools create to keep track of the original user id, and do the
> ownership check using those instead.
> 
> This assumes the environment the user is running with after going
> priviledged can't be tampered with, and also does the check only for

s/priviledged/privileged 

> root to keep the most common case less complicated, but as a side effect
> will miss cases where sudo (or an equivalent) was used to change to
> another unpriviledged user.

s/unpriviledged/unpriviledged 

> Sent as an RFC as it has been only lightly tested and because some of
> the assumptions I have to make, made me unconfortable.

 
> Ex, in order to make the atoi() calls safe, I was originally doing
> is_digit(), but that would require this function to move further down
> to work.
>
> It is also now big enough that would make sense for it to move into
> its own compat file and outside for git-compat-util.h, but if that is
> done we might not keep the "root uid is not always 0" bits that seem
> useful to have for the future.
>
> getent() is not thread safe, so it might be worth to use an alternative
> but that would require a bigger change.

These points make sense. It would be worth taking our time and
doing some refactoring in advance of this functional change.
 
> IMHO it should have a test added, but not sure where it would fit.

I wonder how we could test a multi-user scenario. The tests I
added in e47363e5a (t0033: add tests for safe.directory, 2022-04-13)
purposefully avoid the user-id functionality.

> Original discussion in :
> 
>   https://lore.kernel.org/git/4ef9287b-6260-9538-7c89-cffb611520ee@maurel.de/

I agree that the idea behind this change is a good one. The escalation
of privilege isn't a huge concern when the "real" user is the same.
The only way to trick the root user here is to set an environment
variable, in which case they might as well modify PATH and be done with
it.

> +	euid = geteuid();
> +	if (euid == ROOT_UID) {
> +		/* we might have raised our priviledges with sudo or doas */

Similar spelling error here.

> +		const char *real_uid = getenv("SUDO_UID");
> +		if (real_uid && *real_uid)
> +			euid = atoi(real_uid);
> +		else {
> +			real_uid = getenv("DOAS_UID");
> +			if (real_uid && *real_uid)
> +				euid = atoi(real_uid);
> +		}

I imagine that something else could be added here to help Windows
users who have elevated to administrator privileges. It will use a
completely different mechanism, though, if needed at all. We can
delay that for now.

Thanks,
-Stolee

  reply	other threads:[~2022-04-26 19:48 UTC|newest]

Thread overview: 170+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-26 18:31 Carlo Marcelo Arenas Belón
2022-04-26 19:48 ` Derrick Stolee [this message]
2022-04-26 19:56   ` Junio C Hamano
2022-04-26 20:10     ` rsbecker
2022-04-26 20:45       ` Carlo Arenas
2022-04-26 21:10         ` Junio C Hamano
2022-04-26 20:12     ` Carlo Arenas
2022-04-26 20:26   ` Carlo Arenas
2022-04-29 16:16   ` Derrick Stolee
2022-04-27  0:05 ` [PATCH] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-04-27  9:33   ` Phillip Wood
2022-04-27 12:30     ` Phillip Wood
2022-04-27 14:15       ` rsbecker
2022-04-27 15:58       ` Carlo Arenas
2022-04-27 16:14         ` Phillip Wood
2022-04-27 18:54           ` Junio C Hamano
2022-04-27 20:59             ` Carlo Arenas
2022-04-27 21:09               ` rsbecker
2022-04-27 21:25               ` Junio C Hamano
2022-04-28 17:56             ` Phillip Wood
2022-04-27 15:38     ` Carlo Arenas
2022-04-27 15:50       ` rsbecker
2022-04-27 16:19       ` Junio C Hamano
2022-04-27 16:45         ` Carlo Arenas
2022-04-27 17:22         ` Phillip Wood
2022-04-27 17:49           ` rsbecker
2022-04-27 17:54             ` Carlo Arenas
2022-04-27 18:05               ` rsbecker
2022-04-27 18:11                 ` Carlo Arenas
2022-04-27 18:16                   ` rsbecker
2022-04-27 16:31       ` Phillip Wood
2022-04-27 16:54         ` Carlo Arenas
2022-04-27 17:28           ` Phillip Wood
2022-04-27 17:49             ` Carlo Arenas
2022-04-27 22:26   ` [RFC PATCH v2] " Carlo Marcelo Arenas Belón
2022-04-27 22:33     ` Junio C Hamano
2022-04-28  3:35     ` [PATCH 0/2] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-04-28  3:35       ` [PATCH 1/2] Documentation: explain how safe.directory works when running under sudo Carlo Marcelo Arenas Belón
2022-04-28  5:17         ` Junio C Hamano
2022-04-28  5:58           ` Carlo Arenas
2022-04-28  6:41             ` Junio C Hamano
2022-04-28  3:35       ` [PATCH 2/2] t: add tests for safe.directory when running with sudo Carlo Marcelo Arenas Belón
2022-04-28  5:34         ` Junio C Hamano
2022-04-28  4:57       ` [PATCH 0/2] fix `sudo make install` regression in maint Junio C Hamano
2022-04-28 10:58       ` [PATCH v2 0/3] " Carlo Marcelo Arenas Belón
2022-04-28 10:58         ` [PATCH v2 1/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-04-28 18:02           ` Phillip Wood
2022-04-28 18:57             ` Carlo Arenas
2022-04-28 10:58         ` [PATCH v2 2/3] Documentation: explain how safe.directory works when running under sudo Carlo Marcelo Arenas Belón
2022-04-30  6:17           ` Bagas Sanjaya
2022-04-30  6:39             ` Junio C Hamano
2022-04-30 14:15             ` Carlo Marcelo Arenas Belón
2022-04-28 10:58         ` [PATCH v2 3/3] t: add tests for safe.directory when running with sudo Carlo Marcelo Arenas Belón
2022-04-28 16:55           ` Junio C Hamano
2022-04-28 18:08             ` Phillip Wood
2022-04-28 18:12               ` Junio C Hamano
2022-05-06 17:50                 ` Carlo Arenas
2022-05-06 21:43                   ` Junio C Hamano
2022-05-06 22:57                     ` Carlo Arenas
2022-05-06 23:55                       ` Junio C Hamano
2022-05-07 11:57                         ` Carlo Marcelo Arenas Belón
2022-04-28 19:53             ` rsbecker
2022-04-28 20:22               ` Carlo Arenas
2022-04-28 20:43                 ` rsbecker
2022-04-28 20:51                   ` Junio C Hamano
2022-04-28 20:56                   ` Carlo Arenas
2022-04-28 21:55                     ` rsbecker
2022-04-28 22:21                       ` Junio C Hamano
2022-04-28 22:45                         ` rsbecker
2022-04-28 20:46                 ` Junio C Hamano
2022-04-28 20:32               ` Junio C Hamano
2022-04-28 20:40                 ` rsbecker
2022-04-28 20:48                 ` Carlo Arenas
2022-04-28 21:02             ` Carlo Arenas
2022-04-28 21:07               ` Junio C Hamano
2022-04-29  1:24                 ` Carlo Marcelo Arenas Belón
2022-04-29 18:50                   ` Junio C Hamano
2022-04-29 20:05                     ` Carlo Marcelo Arenas Belón
2022-05-02 18:39         ` [RFC PATCH v3 0/3] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-02 18:39           ` [RFC PATCH v3 1/3] t: document regression git safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-02 21:35             ` Junio C Hamano
2022-05-02 23:07               ` Carlo Arenas
2022-05-02 18:39           ` [RFC PATCH v3 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-02 18:39           ` [RFC PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo Carlo Marcelo Arenas Belón
2022-05-02 22:10             ` Junio C Hamano
2022-05-03  0:00               ` Carlo Arenas
2022-05-03  6:54         ` [PATCH v3 0/3] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-03  6:54           ` [PATCH v3 1/3] t: document regression git safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-03 14:03             ` Phillip Wood
2022-05-03 15:56               ` Carlo Marcelo Arenas Belón
2022-05-04 11:15                 ` Phillip Wood
2022-05-04 13:02                   ` Carlo Arenas
2022-05-04 14:11                     ` Phillip Wood
2022-05-05 13:44             ` Johannes Schindelin
2022-05-05 14:34               ` Phillip Wood
2022-05-05 15:50               ` Junio C Hamano
2022-05-05 18:33               ` Junio C Hamano
2022-05-05 19:39                 ` Junio C Hamano
2022-05-06 21:03                   ` Carlo Arenas
2022-05-09  8:21                 ` Phillip Wood
2022-05-09 14:51                   ` Carlo Arenas
2022-05-09 15:18                     ` Phillip Wood
2022-05-09 16:01                   ` Junio C Hamano
2022-05-09 16:21                     ` Carlo Arenas
2022-05-06 17:39               ` Carlo Arenas
2022-05-03  6:54           ` [PATCH v3 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-05 14:01             ` Johannes Schindelin
2022-05-05 14:32               ` Phillip Wood
2022-05-06 19:15                 ` Carlo Arenas
2022-05-06 20:00                   ` Junio C Hamano
2022-05-06 20:22                     ` Carlo Arenas
2022-05-06 20:59                       ` Junio C Hamano
2022-05-06 21:40                         ` Carlo Arenas
2022-05-06 21:07                       ` rsbecker
2022-05-05 16:09               ` Junio C Hamano
2022-05-06 20:02               ` Carlo Arenas
2022-05-03  6:54           ` [PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo Carlo Marcelo Arenas Belón
2022-05-03 14:12             ` Phillip Wood
2022-05-03 15:27               ` Junio C Hamano
2022-05-06 16:54               ` Carlo Arenas
2022-05-07 16:35           ` [RFC PATCH v4 0/3] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-07 16:35             ` [RFC PATCH v4 1/3] t: regression git needs safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-07 16:35             ` [RFC PATCH v4 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-07 17:34               ` Junio C Hamano
2022-05-07 18:56                 ` Carlo Marcelo Arenas Belón
2022-05-09 16:54                   ` Junio C Hamano
2022-05-09 17:36                     ` rsbecker
2022-05-09 18:48                     ` Carlo Arenas
2022-05-09 19:16                       ` rsbecker
2022-05-09 19:41                       ` Junio C Hamano
2022-05-07 16:35             ` [RFC PATCH v4 3/3] t0034: add negative tests and allow git init to mostly work under sudo Carlo Marcelo Arenas Belón
2022-05-10 14:17             ` [RFC PATCH v4 0/3] fix `sudo make install` regression in maint Phillip Wood
2022-05-10 15:47               ` Carlo Arenas
2022-05-10 17:46             ` [PATCH " Carlo Marcelo Arenas Belón
2022-05-10 17:46               ` [PATCH v4 1/3] t: regression git needs safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-10 22:10                 ` Junio C Hamano
2022-05-10 23:11                   ` Carlo Arenas
2022-05-10 23:44                     ` Junio C Hamano
2022-05-11  0:56                       ` Carlo Arenas
2022-05-11  1:11                         ` Junio C Hamano
2022-05-10 17:46               ` [PATCH v4 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-10 22:57                 ` Junio C Hamano
2022-05-11  7:34                   ` Carlo Arenas
2022-05-11 14:58                     ` Junio C Hamano
2022-05-10 17:46               ` [PATCH v4 3/3] t0034: add negative tests and allow git init to mostly work under sudo Carlo Marcelo Arenas Belón
2022-05-10 23:11                 ` Junio C Hamano
2022-05-10 23:25                   ` Junio C Hamano
2022-05-11 14:04                   ` Carlo Arenas
2022-05-11 15:29                     ` Junio C Hamano
2022-05-13  1:00               ` [PATCH v5 0/4] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-13  1:00                 ` [PATCH v5 1/4] t: regression git needs safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-06-03 12:12                   ` SZEDER Gábor
2022-05-13  1:00                 ` [PATCH v5 2/4] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-06-03 11:05                   ` SZEDER Gábor
2022-06-03 16:54                     ` Junio C Hamano
2022-06-03 17:34                       ` SZEDER Gábor
2022-05-13  1:00                 ` [PATCH v5 3/4] t0034: add negative tests and allow git init to mostly work under sudo Carlo Marcelo Arenas Belón
2022-05-13  1:20                   ` Junio C Hamano
2022-05-14 14:36                     ` Carlo Arenas
2022-05-15 16:54                       ` Junio C Hamano
2022-05-15 19:21                         ` Carlo Arenas
2022-05-16  5:27                           ` Junio C Hamano
2022-05-16 13:07                             ` Carlo Marcelo Arenas Belón
2022-05-16 16:25                               ` Junio C Hamano
2022-05-13  1:00                 ` [PATCH v5 4/4] git-compat-util: allow root to access both SUDO_UID and root owned Carlo Marcelo Arenas Belón
2022-06-15 14:02                   ` Johannes Schindelin
2022-06-17 14:26                     ` Carlo Arenas
2022-06-17 16:00                       ` Junio C Hamano
2022-06-17 20:23                   ` [PATCH v6] " Carlo Marcelo Arenas Belón
2022-06-17 21:02                     ` Junio C Hamano

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=9658dea7-d421-b238-113d-c7b83eca4569@github.com \
    --to=derrickstolee@github.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=guy.j@maurel.de \
    --cc=me@ttaylorr.com \
    --cc=philipoakley@iee.email \
    --cc=rsbecker@nexbridge.com \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [RFC PATCH] git-compat-util: avoid failing dir ownership checks if running priviledged' \
    /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

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