On Sat, May 07, 2022 at 10:34:44AM -0700, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> writes:
>
> > + /*
> > + * env_id could underflow/overflow in the previous call
> > + * and if it will still fit in a long it will not report
> > + * it as error with ERANGE, instead silently using an
> > + * equivalent positive number that might be bogus.
> > + * if uid_t is narrower than long, it might not fit,
> > + * hence why we need to check it against the maximum
> > + * possible uid_t value before accepting it.
> > + */
> > + if (!*endptr && !errno && env_id <= (uid_t)-1)
> > + *id = env_id;
>
> Thanks for very clearly spelling out why you care. It makes it much
> easier to explain why I disagree with the line of reasoning ;-)
Funny enough the comment doesn't even scratch the surface on the genius
of that (uid_t)-1 and how it also prevents people with a signed uid_t
to misuse this code as well, while still allowing uid_t > INT_MAX.
As mentioned in the cover letter, the code without this is indeed broken
and unsafe to use in that case, so we will need to do something else (as
spelled there as well), I we were to proceed with it, which as I said
before I am totally OK with (granted we do that something else, and
promess not to misuse this somewhere later and open us to a potentially
embarrasing bug.
> This code may be exercised by a potential attacker, but we know that
> the codepath is entered only when euid==ROOT_UID. The attacker may
> or may not have used 'sudo', and we cannot trust the value of
> SUDO_UID at all. But that is OK. If the attacker already is root
> on the box, they do not have to use "git" or exercise this new code
> in order to attack anybody on the box already. This requires us to
> exclude social engineering attack to tell a victim to run "sudo",
> set SUDO_UID to a specific value, and run something, but at last I
> have been excluding that from the beginning. There are easier
> things you can tell the potential victim to cause harm while being
> root.
Agree, but even if I put a scary looking warning and tried to make this
code harder to use than it needs to be, is fairly visible and there had
been already suggestions of removing that restriction.
Which is why I said this is more a defensive programming solution than
real protection under the current constrains.
> Now the whole point of adding this new code to _weaken_ the existing
> check is to help legitimate users who are authorised to become root
> via "sudo" on the box. Making it easier for them to use "git" while
> tentatively gaining root priviledge so that they can do "make
> install" in a repository they own.
>
> We know that this code is meant to be exercised after a potential
> victim gained euid==ROOT_UID via 'sudo', and SUDO_UID is exported by
> the command for the original user. If uid_t is narrower than ulong
> (e.g. 16-bit uid_t vs 64-bit ulong), and if it is unsigned, the only
> effect the extra check is doing is to exclude the unfortunate user
> with uid==65535 from using "sudo git describe".
not quite, the check does "<=" so excludes no legitimate users, what
is excludes is bit multiplers of those valid users ids to work, so
if an obviously impossible to get legitimately and therefore bogus if
uid_t is 16bit value of 65536 even gets here we will not assume it was
root instead, which I would find personally embarrasing.
> In exchange, the only attack scenario the code prevents is this,
> IIUC.
>
> * You, the aspiring cracker, are a user not allowed to run "sudo" on
> the box, and you know your uid is 1000
>
> * You look for another user, a potential victim, whose uid is 1000
> modulo 65536 (if your uid_t is 16-bit) and who can run "sudo" on
> the box.
>
> * You prepare a malicious repository, invite that user there and
> ask them to run "sudo something" there.
>
> I'd say such an attack vector is not likely, and a user with maximum
> allowed uid_t value is equally not that likely, so I do not care too
> deeply either way---and in such a case, I do prefer a simpler code.
as I do as well, but this is only 4 characters long and getting rid of
it means we now have to do either of :
* hope nobody has a signed uid_t as this code is broken in that case
and will misbehave widly, more importantly we also lost our only
way to find them and warn them ahead of time that they need to patch
it.
* add some aditional way to detect it and avoid this code and maybe
even provide an alternative.
and also :
* move back to strtol and potentially break the feature for half the
valid users if uid_t is 32-bit unsigned as it is most likely to be
in 32-bit systems.
* move to an even wider integer so it will be always wider than uid_t
which means we have to move a lot more code around at least.
Carlo
