acassis commented on code in PR #19900:
URL: https://github.com/apache/nuttx/pull/19900#discussion_r3812892016


##########
Documentation/applications/nsh/commands.rst:
##########
@@ -215,6 +215,44 @@ Also sets the previous working directory environment 
variable
 ``cd ..``              sets the current working directory to the parent 
directory.
 ==================  =====================================
 
+.. _cmdchroot:
+
+``chroot`` Change Root Directory
+================================
+
+**Command Syntax**::
+
+  chroot <newroot> [<command> [args...]]
+
+**Synopsis**. Change the filesystem root of the current task group so
+absolute path lookups start at ``<newroot>``. Requires
+``CONFIG_FS_CHROOT``. This is a filesystem jail, not a container.
+
+The command performs ``chdir(newroot)``, ``chroot(".")``, then
+``chdir("/")``. With no extra arguments the current NSH session stays
+jailed (``pwd`` shows ``/``). An optional command is executed with
+``execvp()`` after the jail is in place.
+
+When ``CONFIG_SCHED_USER_IDENTITY`` is enabled, ``chroot()`` requires
+effective UID 0. Drop extra privilege after jailing so a later
+``chroot()`` cannot be used to escape.
+
+File descriptors opened before ``chroot()`` are not retroactively
+contained. The ``chroot <newroot> <command>`` form closes non-stdio
+descriptors that are not already ``O_CLOEXEC`` before ``execvp()``.
+The no-command form leaves the current session's existing descriptors
+usable, including any that point outside the jail.
+
+**Example**::
+
+  nsh> mkdir /tmp/jail
+  nsh> echo hello > /tmp/jail/marker
+  nsh> chroot /tmp/jail
+  nsh> pwd
+  /

Review Comment:
   @Abhishekmishra2808 add here after pwd the "nsh> ls /" to show that there is 
not /dev, /proc available.



##########
fs/inode/fs_inodesearch.c:
##########
@@ -193,6 +199,120 @@ static int _inode_linktarget(FAR struct inode *inode,
 }
 #endif
 
+#ifdef CONFIG_FS_CHROOT
+/****************************************************************************
+ * Name: inode_get_chroot
+ ****************************************************************************/
+
+static FAR struct inode *inode_get_chroot(FAR const char **relpath)
+{
+  FAR struct tcb_s *tcb = nxsched_self();
+
+  if (relpath != NULL)
+    {
+      *relpath = NULL;
+    }
+
+  if (tcb != NULL && tcb->group != NULL && tcb->group->tg_root != NULL)
+    {
+      if (relpath != NULL)
+        {
+          *relpath = tcb->group->tg_rootrel;
+        }
+
+      return tcb->group->tg_root;
+    }
+
+  return g_root_inode;
+}
+
+/****************************************************************************
+ * Name: inode_normalize_abs
+ *
+ * Description:
+ *   Normalize an absolute path: drop empty and "." segments, and clamp
+ *   ".." at the search root.  Needed even before a jail is installed:
+ *   chroot(".") becomes "$PWD/.", and if PWD sits under a mountpoint
+ *   (tmpfs /tmp) the leftover "." is passed to the filesystem as
+ *   relpath and fails with ENOENT.
+ *
+ ****************************************************************************/

Review Comment:
   Please include the Input parameters and the return



##########
Documentation/implementation/user_identity.rst:
##########
@@ -152,6 +152,19 @@ Configuration
   See :ref:`file-permission` for the VFS helpers, mount-crossing
   traverse rules, and testing notes.
 
+Flat Build Trust Boundary
+=========================
+
+This credential model is a DAC layer for cooperating tasks, not a

Review Comment:
   What is DAC? Digital Analog Converter? Please explain what is DAC in this 
context



##########
Documentation/implementation/chroot.rst:
##########
@@ -0,0 +1,88 @@
+.. _chroot:
+
+======
+chroot

Review Comment:
   I think this "implementation" is misleading: you didn't explain how it was 
implemented, what was necessary, why did you spend an week implementing it to 
explain what was the difficult part of the implementation



##########
Documentation/implementation/chroot.rst:
##########
@@ -0,0 +1,88 @@
+.. _chroot:
+
+======
+chroot
+======
+
+``chroot()`` is a kernel-enforced filesystem jail.  When
+``CONFIG_FS_CHROOT`` is enabled, each task group may pin a directory as
+its root.  Absolute path lookup starts there, so the group cannot see
+files outside that tree.
+
+This is **not** a container.  NuttX does not provide PID, mount, or
+network namespaces.  ``chroot()`` only changes where pathname lookup
+begins.
+
+Configuration
+=============
+
+Enable ``CONFIG_FS_CHROOT`` in the filesystem configuration.  The
+syscall is then available from ``unistd.h``.
+
+When ``CONFIG_SCHED_USER_IDENTITY`` is also enabled, ``chroot()``
+requires effective UID 0 and returns ``-1`` with ``errno`` set to
+``EPERM`` otherwise.  Without user identity every task is treated as
+root, so ``chroot()`` remains allowed.  On ``CONFIG_BUILD_FLAT`` this
+``euid == 0`` gate is the same class of check as credential DAC, not a
+process-isolation boundary; see the trust-boundary note in
+:ref:`user-identity`.
+
+Semantics
+=========
+
+* ``chroot(path)`` resolves ``path`` relative to the caller's current
+  root (so a nested ``chroot()`` cannot walk back to the host tree).
+* ``path`` must name a directory (``ENOTDIR`` otherwise).
+* ``chroot("/")`` from the global root is a no-op.  From inside a jail,
+  ``/`` is the jail root, so it cannot be used to escape.
+* The jail is stored on the task group (``tg_root``, and ``tg_rootrel``
+  when the jail is a subdirectory of a mount such as tmpfs).  Child
+  tasks inherit it.  Kernel threads do not.
+* After a successful ``chroot()``, ``PWD`` is rewritten so relative
+  lookups stay inside the jail:
+
+  * If ``PWD`` is exactly the new root, it becomes ``/``.
+  * If ``PWD`` is under the new root, the prefix is stripped.
+  * Otherwise ``PWD`` is set to ``/``.  Unlike Linux, NuttX does not
+    keep a current directory outside the new tree.
+
+NSH
+===
+
+The NSH ``chroot`` command performs the usual Unix dance::
+
+  chdir(newroot);
+  chroot(".");
+  chdir("/");
+
+With no extra arguments the current NSH session stays jailed (``pwd``
+shows ``/``, ``ls /`` lists the jail tree).  An optional command is
+executed with ``execvp()`` after the jail is in place; NSH closes
+non-stdio, non-``O_CLOEXEC`` descriptors first (see below).
+
+When ``CONFIG_SCHED_USER_IDENTITY`` is enabled, drop extra privilege
+after the jail is in place (for example ``setuid()`` to a non-root
+user) so a later ``chroot()`` cannot be used to escape.
+
+Open file descriptors
+=====================
+
+File descriptors opened before ``chroot()`` are not retroactively
+contained.  POSIX allows this; NuttX does not close them.  A jailed
+task that inherits a host descriptor can read and write that file
+without going through pathname lookup, so the jail does not apply.
+This is the most common way ``chroot()`` is misused as a security
+tool.  Do not treat it as a sandbox against a process that already
+holds host file descriptors.
+
+The NSH ``chroot <newroot> <command>`` form closes every open
+descriptor above stderr that is not already marked ``O_CLOEXEC``
+before ``execvp()``.  Stdio (fds 0--2) is left intact.  The
+no-command form leaves the current NSH session jailed with its
+existing descriptors, including any that point outside the tree.
+
+Out of scope
+============
+
+Bind-mounts or unionfs to populate ``/dev`` inside a jail, mount/PID/
+network namespaces, and ``pivot_root()`` are not provided.

Review Comment:
   Please explain the challenges here, why wasn't it implemented?



##########
Documentation/implementation/chroot.rst:
##########
@@ -0,0 +1,88 @@
+.. _chroot:
+
+======
+chroot
+======
+
+``chroot()`` is a kernel-enforced filesystem jail.  When
+``CONFIG_FS_CHROOT`` is enabled, each task group may pin a directory as
+its root.  Absolute path lookup starts there, so the group cannot see
+files outside that tree.
+
+This is **not** a container.  NuttX does not provide PID, mount, or
+network namespaces.  ``chroot()`` only changes where pathname lookup
+begins.

Review Comment:
   Maybe instead of saying "NuttX does not provide PID, mount, or network 
namespaces" that some people will take as true (and of course NuttX has it), I 
suggest adding:
   Limitations: currently chroot doesn't support PID, mount, network namespaces 
and explain the next start as a TODO. What need to be done to support it



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to