bug#40837: core-updates: webkitgtk web process sandbox incomplete

[Marius Bakke <mbakke@fastmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2561 bytes --]

Hello Jack,

Thanks a lot for this work.

Jack Hill <jackhill@jackhill.us> writes:

> Some additional observations:
>
> With my patched webkitgtk, if I set:
>
> PULSE_CLIENTCONFIG=/gnu/store/zc4dsmvdabi00nvisrjhi9w00ff4igs7-client.conf
>
> it does work, which is an improvement compared to without the patch.

Great.  I have attached a patch for Guix that stops using /etc for these
variables.

> I notice that Nix [0] has a similar patch:
>
> """
> diff -ru old/webkitgtk-2.26.0/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp webkitgtk-2.26.0/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> --- old/webkitgtk-2.26.0/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp	2019-09-09 04:47:07.000000000 -0400
> +++ webkitgtk-2.26.0/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp	2019-09-20 21:14:10.537921173 -0400
> @@ -585,7 +585,7 @@
>           { SCMP_SYS(keyctl), nullptr },
>           { SCMP_SYS(request_key), nullptr },
>
> -        // Scary VM/NUMA ops 
> +        // Scary VM/NUMA ops
>           { SCMP_SYS(move_pages), nullptr },
>           { SCMP_SYS(mbind), nullptr },
>           { SCMP_SYS(get_mempolicy), nullptr },
> @@ -724,6 +724,10 @@
>           "--ro-bind-try", "/usr/local/lib64", "/usr/local/lib64",
>
>           "--ro-bind-try", PKGLIBEXECDIR, PKGLIBEXECDIR,
> +
> +        // Nix Directories
> +        "--ro-bind", "@storeDir@", "@storeDir@",
> +        "--ro-bind", "/run/current-system", "/run/current-system",
>       };
>       // We would have to parse ld config files for more info.
>       bindPathVar(sandboxArgs, "LD_LIBRARY_PATH");
> """
>
> [0] https://github.com/NixOS/nixpkgs/blob/465566948393cf533e3617704d1c4ccc34cf3753/pkgs/development/libraries/webkitgtk/fix-bubblewrap-paths.patch
>
> so I wonder if I didn't do the mounts in the right place and or if it is 
> becasue I missed /run/current-system.
>
> I'm going to try to adapt the Nix patch to see if that helps.

Were you able to verify whether /run/current-system is required inside
the sandbox?

I cleaned up your patch a bit and rebased it on the latest master
branch, available as patch 2/2 below.  Currently building it on
'core-updates' to verify that it works.  It takes a while on my dinky
quad-core server though.  :-)

It does not bind /run/current-system, and I think we should avoid it if
possible.  Ideally we would only mount the store paths required by the
consumers instead of all of /gnu/store, but not sure how to achieve
that.


[-- Attachment #1.2: 0001-services-Do-not-use-symbolic-links-in-PulseAudio-var.patch --]
[-- Type: text/x-patch, Size: 3195 bytes --]

From a2607c8246456460a6bbed62144daf7196a5c9bd Mon Sep 17 00:00:00 2001
From: Marius Bakke <mbakke@fastmail.com>
Date: Wed, 6 May 2020 17:48:42 +0200
Subject: [PATCH 1/2] services: Do not use symbolic links in PulseAudio
 variables.

This addresses <https://bugs.gnu.org/40837> by making these configuration
files more easily accessible within the WebKitGTK+ sandbox.

* gnu/services/sound.scm (pulseaudio-environment): Move below
PULSEAUDIO-CONF-ENTRY.  Create PULSE_CONFIG and PULSE_CLIENTCONFIG entries
directly instead of referring to /etc/pulse.
(pulseaudio-etc): Do not create /etc/pulse/client.conf and /etc/pulse/daemon.conf.
---
 gnu/services/sound.scm | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index a1c928222a..bdf819b422 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2018, 2020 Oleg Pykhalov <go.wigust@gmail.com>
 ;;; Copyright © 2020 Leo Prikler <leo.prikler@student.tugraz.at>
+;;; Copyright © 2020 Marius Bakke <mbakke@fastmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -127,11 +128,6 @@ ctl.!default {
                       (default
                         (file-append pulseaudio "/etc/pulse/system.pa"))))
 
-(define (pulseaudio-environment config)
-  `(;; Define these variables, so that pulseaudio honors /etc.
-    ("PULSE_CONFIG" . "/etc/pulse/daemon.conf")
-    ("PULSE_CLIENTCONFIG" . "/etc/pulse/client.conf")))
-
 (define (pulseaudio-conf-entry arg)
   (match arg
     ((key . value)
@@ -139,21 +135,22 @@ ctl.!default {
     ((? string? _)
      (string-append arg "\n"))))
 
+(define pulseaudio-environment
+  (match-lambda
+    (($ <pulseaudio-configuration> client-conf daemon-conf default-script-file)
+     `(("PULSE_CONFIG" . ,(apply mixed-text-file "daemon.conf"
+                                 "default-script-file = " default-script-file "\n"
+                                 (map pulseaudio-conf-entry daemon-conf)))
+       ("PULSE_CLIENTCONFIG" . ,(apply mixed-text-file "client.conf"
+                                       (map pulseaudio-conf-entry client-conf)))))))
+
 (define pulseaudio-etc
   (match-lambda
-    (($ <pulseaudio-configuration> client-conf daemon-conf
-                                   default-script-file system-script-file)
+    (($ <pulseaudio-configuration> _ _ default-script-file system-script-file)
      `(("pulse"
         ,(file-union
           "pulse"
-          `(("client.conf"
-             ,(apply mixed-text-file "client.conf"
-                     (map pulseaudio-conf-entry client-conf)))
-            ("daemon.conf"
-             ,(apply mixed-text-file "daemon.conf"
-                     "default-script-file = " default-script-file "\n"
-                     (map pulseaudio-conf-entry daemon-conf)))
-            ("default.pa" ,default-script-file)
+          `(("default.pa" ,default-script-file)
             ("system.pa" ,system-script-file))))))))
 
 (define pulseaudio-service-type
-- 
2.26.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-gnu-webkitgtk-Patch-to-share-store-via-Bubblewrap.patch --]
[-- Type: text/x-patch, Size: 4134 bytes --]

From 3864b54f4aadefc600433d3654b0a1a73ab6fa98 Mon Sep 17 00:00:00 2001
From: Jack Hill <jackhill@jackhill.us>
Date: Sat, 25 Apr 2020 22:03:48 -0400
Subject: [PATCH 2/2] gnu: webkitgtk: Patch to share store via Bubblewrap.

Fixes <https://bugs.gnu.org/40837>.

* gnu/packages/patches/webkitgtk-share-store.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/webkit.scm (webkitgtk)[source](patches): Use it.

Co-authored-by: Marius Bakke <mbakke@fastmail.com>
---
 gnu/local.mk                                  |  1 +
 .../patches/webkitgtk-share-store.patch       | 20 +++++++++++++++++++
 gnu/packages/webkit.scm                       | 12 ++++++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)
 create mode 100644 gnu/packages/patches/webkitgtk-share-store.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 62eeb39ece..5c06415205 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1542,6 +1542,7 @@ dist_patch_DATA =						\
   %D%/packages/patches/vte-CVE-2012-2738-pt2.patch			\
   %D%/packages/patches/warsow-qfusion-fix-bool-return-type.patch	\
   %D%/packages/patches/weasyprint-library-paths.patch		\
+  %D%/packages/patches/webkitgtk-share-store.patch		\
   %D%/packages/patches/websocketpp-fix-for-boost-1.70.patch	\
   %D%/packages/patches/wicd-bitrate-none-fix.patch		\
   %D%/packages/patches/wicd-get-selected-profile-fix.patch	\
diff --git a/gnu/packages/patches/webkitgtk-share-store.patch b/gnu/packages/patches/webkitgtk-share-store.patch
new file mode 100644
index 0000000000..4174e73b6c
--- /dev/null
+++ b/gnu/packages/patches/webkitgtk-share-store.patch
@@ -0,0 +1,20 @@
+Author: Jack Hill <jackhill@jackhill.us>
+Tell bubblewrap to share the store.
+
+See <https://bugs.gnu.org/40837>.
+
+---
+diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+index ad301ab2..d53b680e 100644
+--- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
++++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+@@ -737,6 +737,9 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
+         "--ro-bind-try", "/usr/local/share", "/usr/local/share",
+         "--ro-bind-try", DATADIR, DATADIR,
+ 
++       // Bind mount the store inside the WebKitGTK sandbox.
++       "--ro-bind", "@storedir@", "@storedir@",
++
+         // We only grant access to the libdirs webkit is built with and
+         // guess system libdirs. This will always have some edge cases.
+         "--ro-bind-try", "/lib", "/lib",
diff --git a/gnu/packages/webkit.scm b/gnu/packages/webkit.scm
index e52536c279..6035d6c59d 100644
--- a/gnu/packages/webkit.scm
+++ b/gnu/packages/webkit.scm
@@ -128,7 +128,8 @@ engine that uses Wayland for graphics output.")
                                   "webkitgtk-" version ".tar.xz"))
               (sha256
                (base32
-                "1g9hik3bprki5s9d7y5288q5irwckbzajr6rnlvjrlnqrwjkblmr"))))
+                "1g9hik3bprki5s9d7y5288q5irwckbzajr6rnlvjrlnqrwjkblmr"))
+              (patches (search-patches "webkitgtk-share-store.patch"))))
     (build-system cmake-build-system)
     (outputs '("out" "doc"))
     (arguments
@@ -156,6 +157,15 @@ engine that uses Wayland for graphics output.")
                           "-DUSE_WOFF2=OFF")
        #:phases
        (modify-phases %standard-phases
+         (add-after 'unpack 'configure-bubblewrap-store-directory
+           (lambda _
+             ;; This phase is a corollary to 'webkitgtk-share-store.patch' to
+             ;; avoid hard coding /gnu/store, for users with other prefixes.
+             (let ((store-directory (%store-directory)))
+               (substitute*
+                   "Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp"
+                 (("@storedir@") store-directory))
+               #t)))
          (add-after 'unpack 'patch-gtk-doc-scan
            (lambda* (#:key inputs #:allow-other-keys)
              (for-each (lambda (file)
-- 
2.26.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]