fs/binfmt: close symlink TOCTOU and harden setuid/setgid exec hygiene

binfmt/binfmt_execmodule.c

@@ -45,6 +45,7 @@
 #include <nuttx/binfmt/binfmt.h>
 
 #include "binfmt.h"
+#include "environ/environ.h"
 
 #ifndef CONFIG_BINFMT_DISABLE
 
@@ -301,14 +302,29 @@ int exec_module(FAR struct binary_s *binp,
 #endif
 
 #ifdef CONFIG_SCHED_USER_IDENTITY
-  if (binp->mode & S_ISUID)
-    {
-      tcb->group->tg_euid = binp->uid;
-    }
+  /* Apply set-user-ID / set-group-ID credentials for the new image. */
 
-  if (binp->mode & S_ISGID)
+  if ((binp->mode & (S_ISUID | S_ISGID)) != 0)
     {
-      tcb->group->tg_egid = binp->gid;
+      FAR struct task_group_s *group = tcb->group;
+
+      if ((binp->mode & S_ISUID) != 0)
+        {
+          group->tg_euid = binp->uid;
+          group->tg_suid = binp->uid;
+        }
+
+      if ((binp->mode & S_ISGID) != 0)
+        {
+          group->tg_egid = binp->gid;
+          group->tg_sgid = binp->gid;
+        }
+
+      group->tg_flags |= GROUP_FLAG_SECURE_EXEC;
+      group->tg_flags &= ~(GROUP_FLAG_FD_BACKTRACE | GROUP_FLAG_DUMPABLE);
+#ifndef CONFIG_DISABLE_ENVIRON
+      env_sanitize_secure(group);
+#endif
     }
 #endif
 

fs/inode/fs_inoderemove.c

@@ -79,6 +79,18 @@ static FAR struct inode *inode_unlink(FAR const char *path)
       inode = desc.node;
       DEBUGASSERT(inode != NULL);
 
+#ifdef CONFIG_FS_PERMISSION
+      if (desc.parent != NULL)
+        {
+          ret = inode_checkperm(desc.parent, W_OK);
+          if (ret < 0)
+            {
+              inode = NULL;
+              goto errout;
+            }
+        }
+#endif
+
       /* If peer is non-null, then remove the node from the right of
        * of that peer node.
        */

fs/inode/fs_inodereserve.c

@@ -226,6 +226,17 @@ int inode_reserve(FAR const char *path,
   left   = desc.peer;
   parent = desc.parent;
 
+#ifdef CONFIG_FS_PERMISSION
+  if (parent != NULL)
+    {
+      ret = inode_checkperm(parent, W_OK | X_OK);
+      if (ret < 0)
+        {
+          goto errout_with_search;
+        }
+    }
+#endif
+
   for (; ; )
     {
       FAR struct inode *node;

fs/inode/inode.h

@@ -374,6 +374,7 @@ void inode_root_reserve(void);
  *
  *   EINVAL - 'path' is invalid for this operation
  *   EEXIST - An inode already exists at 'path'
+ *   EACCES - Caller lacks permission on the parent directory
  *   ENOMEM - Failed to allocate in-memory resources for the operation
  *
  ****************************************************************************/
@@ -390,6 +391,10 @@ int inode_reserve(FAR const char *path,
  *   inode is in-use, then it will be unlinked, but will not be freed until
  *   the last reference to the inode is released.
  *
+ * Returned Value:
+ *   Zero (OK) on success; a negated errno value on failure, including
+ *   -EACCES if the caller lacks write permission on the parent directory.
+ *
  * Assumptions/Limitations:
  *   The caller must hold the inode semaphore
  *

fs/vfs/fs_mkdir.c

@@ -137,19 +137,11 @@ int mkdir(const char *pathname, mode_t mode)
 
   else
     {
-      /* Verify write+search permission on the parent directory before
-       * adding a new name to the pseudo-filesystem tree.  POSIX requires
-       * both W_OK and X_OK to create a directory entry.
-       */
-
-      ret = inode_checkperm(desc.parent, W_OK | X_OK);
-      if (ret < 0)
-        {
-          errcode = -ret;
-          goto errout_with_search;
-        }
-
       /* Create an inode in the pseudo-filesystem at this path.
+       * inode_reserve() resolves the path and checks parent-directory
+       * permissions while the inode tree lock is held, closing symlink
+       * TOCTOU windows.
+       *
        * NOTE that the new inode will be created with a reference
        * count of zero.
        */

fs/vfs/fs_open.c

@@ -172,7 +172,19 @@ static int file_vopen(FAR struct file *filep, FAR const char *path,
 
   /* Validate operation support and pseudo-filesystem permissions */
 
-  ret = inode_checkopenperm(inode, oflags);
+#ifndef CONFIG_DISABLE_MOUNTPOINT
+  if (INODE_IS_MOUNTPT(inode))
+    {
+      ret = inode_checkopenperm(inode, oflags);
+    }
+  else
+#endif
+    {
+      inode_rlock();
+      ret = inode_checkopenperm(inode, oflags);
+      inode_runlock();
+    }
+
   if (ret < 0)
     {
       goto errout_with_inode;

fs/vfs/fs_rename.c

@@ -71,32 +71,34 @@ static int pseudorename(FAR const char *oldpath, FAR struct inode *oldinode,
                         FAR const char *newpath)
 {
   struct inode_search_s newdesc;
-  struct inode_search_s pardesc;
+  struct inode_search_s olddesc;
   FAR struct inode *newinode;
-  FAR struct inode *parnode;
   FAR char *subdir = NULL;
 #ifdef CONFIG_FS_NOTIFY
   bool isdir = INODE_IS_PSEUDODIR(oldinode);
 #endif
   int ret;
 
-  /* SETUP_SEARCH early so RELEASE_SEARCH at errout is safe. */
+  /* Hold the inode tree lock across resolve / permission checks / mutate
+   * so symbolic links cannot be swapped between check and use.
+   * Destination parent permissions are enforced by inode_reserve().
+   */
 
-  SETUP_SEARCH(&newdesc, newpath, true);
+  inode_lock();
 
-  /* Verify source parent write permission. */
+  SETUP_SEARCH(&newdesc, newpath, true);
 
   ret = inode_checkperm(oldparent, W_OK);
   if (ret < 0)
     {
-      goto errout;
+      goto errout_with_lock;
     }
 
   /* According to POSIX, any new inode at this path should be removed
    * first, provided that it is not a directory.
    */
 
-  ret = inode_find(&newdesc);
+  ret = inode_search(&newdesc);
   if (ret >= 0)
     {
       /* We found it.  Get the search results */
@@ -110,19 +112,17 @@ static int pseudorename(FAR const char *oldpath, FAR struct inode *oldinode,
 
       if (oldinode == newinode)
         {
-          inode_release(newinode);
           ret = OK;
-          goto errout; /* Same name, this is not an error case. */
+          goto errout_with_lock;
         }
 
 #ifndef CONFIG_DISABLE_MOUNTPOINT
       /* Make sure that the old path does not lie on a mounted volume. */
 
       if (INODE_IS_MOUNTPT(newinode))
         {
-          inode_release(newinode);
           ret = -EXDEV;
-          goto errout;
+          goto errout_with_lock;
         }
 #endif
 
@@ -145,7 +145,7 @@ static int pseudorename(FAR const char *oldpath, FAR struct inode *oldinode,
             {
               subdir = NULL;
               ret = -ENOMEM;
-              goto errout;
+              goto errout_with_lock;
             }
 
           newpath = subdir;
@@ -162,57 +162,43 @@ static int pseudorename(FAR const char *oldpath, FAR struct inode *oldinode,
            * won't really be removed until we call inode_release();
            */
 
-          inode_remove(newpath);
+          ret = inode_remove(newpath);
+          if (ret < 0 && ret != -EBUSY)
+            {
+              goto errout_with_lock;
+            }
+
 #ifdef CONFIG_FS_NOTIFY
           notify_unlink(newpath);
 #endif
         }
-
-      inode_release(newinode);
-    }
-
-  /* Re-resolve the final destination parent after path rewrite. */
-
-  SETUP_SEARCH(&pardesc, newpath, true);
-  inode_find(&pardesc);   /* pardesc.parent valid even if node not found */
-  parnode = pardesc.node;
-
-  ret = inode_checkperm(pardesc.parent, W_OK);
-
-  /* inode_find() holds a reference on parnode; RELEASE_SEARCH() only
-   * frees pardesc.buffer.
-   */
-
-  if (parnode != NULL)
-    {
-      inode_release(parnode);
-    }
-
-  RELEASE_SEARCH(&pardesc);
-
-  if (ret < 0)
-    {
-      goto errout;
     }
 
   /* Create a new, empty inode at the destination location.
    * NOTE that the new inode will be created with a reference count
    * of  zero.
    */
 
-  inode_lock();
   ret = inode_reserve(newpath, 0777, &newinode);
   if (ret < 0)
     {
-      /* It is an error if a node at newpath already exists in the tree
-       * OR if we fail to allocate memory for the new inode (and possibly
-       * any new intermediate path segments).
-       */
+      goto errout_with_lock;
+    }
 
-      ret = -EEXIST;
+  /* Re-resolve the source under the same lock before unlinking it. */
+
+  SETUP_SEARCH(&olddesc, oldpath, true);
+  ret = inode_search(&olddesc);
+  if (ret < 0 || olddesc.node != oldinode)
+    {
+      inode_remove(newpath);
+      ret = -ENOENT;
+      RELEASE_SEARCH(&olddesc);
       goto errout_with_lock;
     }
 
+  RELEASE_SEARCH(&olddesc);
+
   /* Copy the inode state from the old inode to the newly allocated inode */
 
   newinode->i_child   = oldinode->i_child;   /* Link to lower level inode */
@@ -265,6 +251,7 @@ static int pseudorename(FAR const char *oldpath, FAR struct inode *oldinode,
   ret = OK;
 
 errout_with_lock:
+  RELEASE_SEARCH(&newdesc);
   inode_unlock();
 
 #ifdef CONFIG_FS_NOTIFY
@@ -274,8 +261,6 @@ static int pseudorename(FAR const char *oldpath, FAR struct inode *oldinode,
     }
 #endif
 
-errout:
-  RELEASE_SEARCH(&newdesc);
   if (subdir != NULL)
     {
       fs_heap_free(subdir);

fs/vfs/fs_unlink.c

@@ -121,14 +121,6 @@ int nx_unlink(FAR const char *pathname)
           goto errout_with_inode;
         }
 
-      /* Verify parent-directory write permission before unlink. */
-
-      ret = inode_checkperm(desc.parent, W_OK);
-      if (ret < 0)
-        {
-          goto errout_with_inode;
-        }
-
       /* Notify the driver that it has been unlinked.  If there are no
        * open references to the driver instance, then the driver should
        * release all resources because it is no longer accessible.

include/nuttx/sched.h

@@ -118,7 +118,9 @@
 #define GROUP_FLAG_DELETED         (1 << 2)                      /* Bit 2: Group has been deleted but not yet freed */
 #define GROUP_FLAG_EXITING         (1 << 3)                      /* Bit 3: Group exit is in progress */
 #define GROUP_FLAG_FD_BACKTRACE    (1 << 4)                      /* Bit 4: Enable FD backtrace for the group */
-                                                                 /* Bits 5-7: Available */
+#define GROUP_FLAG_SECURE_EXEC     (1 << 5)                      /* Bit 5: Secure (setuid/setgid) executable */
+#define GROUP_FLAG_DUMPABLE        (1 << 6)                      /* Bit 6: Process may be traced / coredumped */
+                                                                 /* Bit 7: Available */
 
 /* Values for struct child_status_s ch_flags */
 

include/stdlib.h

@@ -173,6 +173,7 @@ uint32_t  arc4random(void);
 
 FAR char **get_environ_ptr(void);
 FAR char *getenv(FAR const char *name);
+FAR char *secure_getenv(FAR const char *name);
 int       putenv(FAR const char *string);
 int       clearenv(void);
 int       setenv(FAR const char *name, FAR const char *value, int overwrite);

include/unistd.h

@@ -488,6 +488,7 @@ gid_t   getgid(void);
 
 int     seteuid(uid_t uid);
 uid_t   geteuid(void);
+int     issetugid(void);
 int     setegid(gid_t gid);
 gid_t   getegid(void);
 

libs/libc/unistd/CMakeLists.txt

@@ -89,7 +89,8 @@ if(NOT CONFIG_SCHED_USER_IDENTITY)
     lib_setreuid.c
     lib_setregid.c
     lib_getresuid.c
-    lib_getresgid.c)
+    lib_getresgid.c
+    lib_issetugid.c)
 endif()
 
 if(NOT CONFIG_DISABLE_ENVIRON)

libs/libc/unistd/Make.defs

@@ -40,6 +40,7 @@ ifneq ($(CONFIG_SCHED_USER_IDENTITY),y)
 CSRCS += lib_setuid.c lib_setgid.c lib_getuid.c lib_getgid.c
 CSRCS += lib_seteuid.c lib_setegid.c lib_geteuid.c lib_getegid.c
 CSRCS += lib_setreuid.c lib_setregid.c lib_getresuid.c lib_getresgid.c
+CSRCS += lib_issetugid.c
 endif
 
 ifneq ($(CONFIG_DISABLE_ENVIRON),y)