From 8f56724e2bc00b7301cb4324a7ace7713ab56dd3 Mon Sep 17 00:00:00 2001
From: Samuli Thomasson <samuli.thomasson@pm.me>
Date: Sun, 1 Jun 2025 09:54:55 +0200
Subject: [PATCH] make sandbox path bind mount read-only on request

Signed-off-by: Samuli Thomasson <samuli.thomasson@pm.me>
---
 src/libstore/include/nix/store/globals.hh     | 19 +++++----
 src/libstore/unix/build/derivation-builder.cc | 42 +++++++++++--------
 .../unix/build/linux-derivation-builder.cc    |  7 +++-
 3 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/src/libstore/include/nix/store/globals.hh b/src/libstore/include/nix/store/globals.hh
index 00d929236..42b5ac2c0 100644
--- a/src/libstore/include/nix/store/globals.hh
+++ b/src/libstore/include/nix/store/globals.hh
@@ -632,13 +632,18 @@ public:
     Setting<PathSet> sandboxPaths{
         this, {}, "sandbox-paths",
         R"(
-          A list of paths bind-mounted into Nix sandbox environments. You can
-          use the syntax `target=source` to mount a path in a different
-          location in the sandbox; for instance, `/bin=/nix-bin` will mount
-          the path `/nix-bin` as `/bin` inside the sandbox. If *source* is
-          followed by `?`, then it is not an error if *source* does not exist;
-          for example, `/dev/nvidiactl?` specifies that `/dev/nvidiactl` will
-          only be mounted in the sandbox if it exists in the host filesystem.
+          A list of paths bind-mounted into Nix sandbox environments. Use the
+          syntax `target[=source][:ro][?]` to control the mount:
+
+          - `=source` will mount a different path at target location; for
+            instance, `/bin=/nix-bin` will mount the path `/nix-bin` as `/bin`
+            inside the sandbox.
+
+          - `:ro` makes the mount read-only (Linux only).
+
+          - `?` makes it not an error if *source* does not exist; for example,
+            `/dev/nvidiactl?` specifies that `/dev/nvidiactl` will only be
+            mounted in the sandbox if it exists in the host filesystem.
 
           If the source is in the Nix store, then its closure will be added to
           the sandbox as well.
diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc
index 1f15cc9e9..89ccf2d2d 100644
--- a/src/libstore/unix/build/derivation-builder.cc
+++ b/src/libstore/unix/build/derivation-builder.cc
@@ -108,8 +108,9 @@ protected:
     struct ChrootPath {
         Path source;
         bool optional;
-        ChrootPath(Path source = "", bool optional = false)
-            : source(source), optional(optional)
+        bool rdonly;
+        ChrootPath(Path source = "", bool optional = false, bool rdonly = false)
+            : source(source), optional(optional), rdonly(rdonly)
         { }
     };
     typedef std::map<Path, ChrootPath> PathsInChroot; // maps target path to source path
@@ -847,20 +848,29 @@ DerivationBuilderImpl::PathsInChroot DerivationBuilderImpl::getPathsInSandbox()
 {
     PathsInChroot pathsInChroot;
 
+    auto addPathWithOptions = [&](std::string s) {
+        if (s.empty()) return;
+        bool optional = false;
+        bool rdonly = false;
+        if (s[s.size() - 1] == '?') {
+            optional = true;
+            s.pop_back();
+        }
+        if (s.size() > 3 && s.substr(s.size() - 3) == ":ro") {
+            rdonly = true;
+            s.resize(s.size() - 3);
+        }
+        size_t p = s.find('=');
+        if (p == std::string::npos)
+            pathsInChroot[s] = {s, optional, rdonly};
+        else
+            pathsInChroot[s.substr(0, p)] = {s.substr(p + 1), optional, rdonly};
+    };
+
     /* Allow a user-configurable set of directories from the
        host file system. */
     for (auto i : settings.sandboxPaths.get()) {
-        if (i.empty()) continue;
-        bool optional = false;
-        if (i[i.size() - 1] == '?') {
-            optional = true;
-            i.pop_back();
-        }
-        size_t p = i.find('=');
-        if (p == std::string::npos)
-            pathsInChroot[i] = {i, optional};
-        else
-            pathsInChroot[i.substr(0, p)] = {i.substr(p + 1), optional};
+        addPathWithOptions(i);
     }
     if (hasPrefix(store.storeDir, tmpDirInSandbox()))
     {
@@ -936,11 +946,7 @@ DerivationBuilderImpl::PathsInChroot DerivationBuilderImpl::getPathsInSandbox()
                 if (line == "") {
                     state = stBegin;
                 } else {
-                    auto p = line.find('=');
-                    if (p == std::string::npos)
-                        pathsInChroot[line] = line;
-                    else
-                        pathsInChroot[line.substr(0, p)] = line.substr(p + 1);
+                    addPathWithOptions(line);
                 }
             }
         }
diff --git a/src/libstore/unix/build/linux-derivation-builder.cc b/src/libstore/unix/build/linux-derivation-builder.cc
index c27b87163..2e3424358 100644
--- a/src/libstore/unix/build/linux-derivation-builder.cc
+++ b/src/libstore/unix/build/linux-derivation-builder.cc
@@ -121,13 +121,18 @@ static void setupSeccomp()
 #  endif
 }
 
-static void doBind(const Path & source, const Path & target, bool optional = false)
+static void doBind(const Path & source, const Path & target, bool optional = false, bool rdonly = false)
 {
     debug("bind mounting '%1%' to '%2%'", source, target);
 
     auto bindMount = [&]() {
         if (mount(source.c_str(), target.c_str(), "", MS_BIND | MS_REC, 0) == -1)
             throw SysError("bind mount from '%1%' to '%2%' failed", source, target);
+
+        if (rdonly)
+            // initial mount wouldn't respect MS_RDONLY, must remount
+            if (mount("", target.c_str(), "", MS_REMOUNT | MS_BIND | MS_RDONLY, 0) == -1)
+                throw (SysError("making bind mount '%s' read-only failed", target));
     };
 
     auto maybeSt = maybeLstat(source);