On Fri, May 6, 2022 at 2:00 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Carlo Arenas <carenas@xxxxxxxxx> writes: > > > On Fri, May 6, 2022 at 1:00 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > >> > >> Carlo Arenas <carenas@xxxxxxxxx> writes: > >> > >> > which is also why we can't use it, any possibly bogus or suspicious > >> > value we get from SUDO_UID MUST be ignored. > >> > >> I do not think I agree. If we have a strange value in SUDO_UID, it > >> would be much better and safer to err on the safe side. > > > > ignoring it is the safe side; for example if we replace the current > > function with the proposed one then some user lucky enough to have > > access to the latest linux supercomputer that has been patched to have > > a 64-bit uid_t (because who makes 32-bit supercomputers nowadays) > > would get root[1] access by simply faking his SUDO_UID to be UINT_MAX > > + 1. > > Since we do not pay attention to SUDO_UID unless euid is root, > anybody who can attack by faking SUDO_UID to affect what Git does > can already become root on the box. Normally yes, but they just might be too clever and found instead a way to fool sudo into generating a bogus SUDO_UID with the hopes that whoever implemented this forgot to check for bogus values and used atoi() and therefore grants them uid = 0 for whatever nefarious objective they might have. While we can't protect against a buggy sudo, we can't do the next best thing and that is to NOT let a would-be attacker still get their 5 min of fame by reporting a vulnerability in git, because they just made it crash through sudo. Especially when the impact of just ignoring them and going our merry way is that (with the current code) we will use the euid of the user that started sudo and "do the right thing" (tm). > In any case, if we notice that SUDO_UID is not a valid number and > die(), we deny the access anyway, so there is no need to write more > code to ignore. not sure which additional code you are referring to here, but currently the only code to ignore is embedded in the one that is needed to read the value, so it is not additional, except maybe for the (uid_t)-1 check that was discussed before[1] and that is what protects us from overflows when sizeof(long) > sizeof(uid_t) && UID_T_MIN >= 0, which are likely most of the systems we are expected to run on. if you are referring to the whole function that could be replaced by using instead git_env_ulong() as dscho suggested, additional code would be needed anyway to make sure that we truncate it appropriately as what we really need is a git_env_uint32() which I am really glad to add if you would rather go that way, but is definitely going to require more code than what we have right now. Note also that we will need to add a flag to it, so it doesn't try to be helpful and allow SUDO_UID=0M to sneak in. Carlo [1] https://lore.kernel.org/git/CAPUEsphFb-=BcV-mxS5RZpJQ8UVq23ni0Lo8tQ4J3TP04B4KQg@xxxxxxxxxxxxxx/
