On 27/04/2022 19:54, Junio C Hamano wrote:
[ I've trimmed your discussion for brevity but thanks for taking the
time to write it out, it was helpful in clarifying what we're trying to
protect ourselves against]
So, in short, I think it is reasonable if we did:
- do the extra "if SUDO_UID exists, then..." only when we are root.
- use strtol to ensure SUDO_UID is in good shape (all digits) and
read into long; use errno=0 trick to notice overflows. err on
the safe side and do not allow if the value cannot be read
correctly.
- do the comparison between st.st_uid and strtol() result by
casting both to (long). Without this, st.st_uid has a value that
needs wider than long, we cannot allow the owner of such a
repository in (and I somehow feel that is unlikely to happen).
- or omit the third one --- tough luck for those whose UID is
beyond what a long can represent (and I somehow feel that this is
probably OK).
I think that unsigned long should be wide enough. On x86 linux uid_t is
uint32_t for both 32 & 64 bit processors. That means that we could
truncate the value we read from SUDO_UID when long is 64 bits if it is
cast to uid_t but we're supposed to be able to trust that SUDO_UID is a
valid id so that shouldn't be a problem. If an attacker can change
what's in SUDO_UID we're doomed anyway.
Best Wishes
Phillip
