On 27/04/2022 10:33, Phillip Wood wrote:

Hi Carlo

On 27/04/2022 01:05, Carlo Marcelo Arenas Belón wrote:

[...]

quoted

+{
+    const char *real_uid = getenv(env);
+
+    if (real_uid && *real_uid) {
+        char *error;
+        long extracted_id = strtol(real_uid, &error, 10);
+        if (!*error && LONG_MIN < extracted_id &&
+                extracted_id < LONG_MAX) {

strtol() returns a long so the last two checks are redundant. The 
standard is silent on what happens to error when the value is out of 
range. The way to check that all the string was consumed without 
underflow/overflow is

     errno = 0;
     val = strtol(str, &endp, 10);
     if (errno || !*endp)

Sorry that should be "if (errno || *endp)" to check for an error

 > [...]
 >     #if sizeof(uid_t) == sizeof(int)
 >         if (extracted_id > INT_MAX)
 >             error(...)
 >     #endif

I think we should probably check if uid_t is a short integer as well as

 > +#ifdef __TANDEM
 > +#define ROOT_UID 65535

suggests it may be an unsigned short on NonStop.

Not knowing the size of uid_t or if it is signed or not (as far as I can 
tell posix just says it's an integer) makes the limit checks tricky - 
maybe we make euid a long (or unsigned long) here and it the function 
below rather than casting it to uid_t and possibly truncating it.

 > [...]

quoted

+
  static inline int is_path_owned_by_current_uid(const char *path)
  {
      struct stat st;
+    uid_t euid;
+
      if (lstat(path, &st))
          return 0;
-    return st.st_uid == geteuid();
+
+    euid = geteuid();
+    if (euid == ROOT_UID) {
+        /* we might have raised our privileges with sudo */
+        extract_id_from_env("SUDO_UID", &euid);

You are ignoring any errors when parsing the environment variable - that 
is not a good idea in a security check.

Best Wishes

Phillip

quoted

+    }
+    return st.st_uid == euid;
  }
  #define is_path_owned_by_current_user is_path_owned_by_current_uid