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