diff mbox

[bug#53676,0/5] *** PulseAudio service improvements ***

Message ID 87zgn11lll.fsf@gmail.com
State Accepted
Headers show

Commit Message

Maxim Cournoyer Feb. 8, 2022, 2:25 p.m. UTC 
Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Hi,
>
> Am Montag, dem 07.02.2022 um 17:29 -0500 schrieb Maxim Cournoyer:
>> Thanks for this!  I wasn't aware of the history; I tried it and it
>> failed the same.  The following fix I attempted in webkitgtk did not
>> seem to do anything:
>> 
>> --8<---------------cut here---------------start------------->8---
>> modified  
>> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
>> @@ -24,6 +24,7 @@
>>  #include <fcntl.h>
>>  #include <glib.h>
>>  #include <seccomp.h>
>> +#include <string.h>
>>  #include <sys/ioctl.h>
>>  #include <sys/mman.h>
>>  #include <unistd.h>
>> @@ -337,7 +338,16 @@ static void bindIfExists(Vector<CString>& args,
>> const char* path, BindFlags bind
>>          bindType = "--ro-bind-try";
>>      else
>>          bindType = "--bind-try";
>> -    args.appendVector(Vector<CString>({ bindType, path, path }));
>> +
>> +    // Canonicalize the source path, otherwise a symbolic link could
>> +    // point to a location outside of the namespace.
>> +    char canonicalPath[PATH_MAX];
>> +    if (!realpath(path, canonicalPath)) {
>> +        if (strlen(path) + 1 > PATH_MAX)
>> +            return;                  // too long of a path
>> +        strcpy(path, canonicalPath); // no-op
>> +    }
>> +    args.appendVector(Vector<CString>({ bindType, canonicalPath,
>> path }));
>>  }
> Apart from raw char arrays and string.h looking funny (and wrong) in
> C++, what is strcpy supposed to do here?  Would it work if we mapped
> canonicalPath to path (i.e. `ls path' in the container would be `ls
> canonicalPath' under the hood)?

I first went the C++ solution, which is std::filesystem::canonical, but
was suggested in #webkitgtk (on the GNOME IRC server) to use the POSIX
realpath, already in use in that file, upon finding out that their build
system is configured to disallow the use of exceptions
(-fno-exceptions).  I refined the experiment as:

--8<---------------cut here---------------start------------->8---
 --8<---------------cut here---------------end--------------->8---

Which produced the intended bwrap arguments, but unfortunately that'd
still fail.  The issue seems to be related to attempt to bind
/etc/pulse/client.conf over something already existing there; it can be
simply reproduced with:

--8<---------------cut here---------------start------------->8---
$ guix shell bubblewrap -- bwrap --ro-bind /gnu /gnu \
    --ro-bind /etc /etc \
    --ro-bind /etc/pulse/client.conf /etc/pulse/client.conf \
    /gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-5.1.8/bin/bash
bwrap: Can't create file at /etc/pulse/client.conf: No such file or directory
--8<---------------cut here---------------end--------------->8---

One thing to try would be to not bind mount client.conf; /etc/ is
already bind mounted as a whole.  If the resolved paths are all bind
mounted (which they are since we share the whole of /gnu), we should be
OK.

Alternatively we could try to bind only the resolved paths, and rewrite
the environment variables such as PULSE_CLIENTCONFIG at run time in
webkitgtk such that it points to the resolved destination.

To be continued...

Maxim

Comments

Liliana Marie Prikler Feb. 8, 2022, 7:31 p.m. UTC | #1 
Hi,

Am Dienstag, dem 08.02.2022 um 09:25 -0500 schrieb Maxim Cournoyer:
> Hi Liliana,
> 
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > Hi,
> > 
> > Am Montag, dem 07.02.2022 um 17:29 -0500 schrieb Maxim Cournoyer:
> > > Thanks for this!  I wasn't aware of the history; I tried it and
> > > it
> > > failed the same.  The following fix I attempted in webkitgtk did
> > > not
> > > seem to do anything:
> > > 
> > > --8<---------------cut here---------------start------------->8---
> > > modified  
> > > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> > > @@ -24,6 +24,7 @@
> > >  #include <fcntl.h>
> > >  #include <glib.h>
> > >  #include <seccomp.h>
> > > +#include <string.h>
> > >  #include <sys/ioctl.h>
> > >  #include <sys/mman.h>
> > >  #include <unistd.h>
> > > @@ -337,7 +338,16 @@ static void bindIfExists(Vector<CString>&
> > > args,
> > > const char* path, BindFlags bind
> > >          bindType = "--ro-bind-try";
> > >      else
> > >          bindType = "--bind-try";
> > > -    args.appendVector(Vector<CString>({ bindType, path, path
> > > }));
> > > +
> > > +    // Canonicalize the source path, otherwise a symbolic link
> > > could
> > > +    // point to a location outside of the namespace.
> > > +    char canonicalPath[PATH_MAX];
> > > +    if (!realpath(path, canonicalPath)) {
> > > +        if (strlen(path) + 1 > PATH_MAX)
> > > +            return;                  // too long of a path
> > > +        strcpy(path, canonicalPath); // no-op
> > > +    }
> > > +    args.appendVector(Vector<CString>({ bindType, canonicalPath,
> > > path }));
> > >  }
> > Apart from raw char arrays and string.h looking funny (and wrong)
> > in
> > C++, what is strcpy supposed to do here?  Would it work if we
> > mapped
> > canonicalPath to path (i.e. `ls path' in the container would be `ls
> > canonicalPath' under the hood)?
> 
> I first went the C++ solution, which is std::filesystem::canonical,
> but was suggested in #webkitgtk (on the GNOME IRC server) to use the
> POSIX realpath, already in use in that file, upon finding out that
> their build system is configured to disallow the use of exceptions
> (-fno-exceptions).  I refined the experiment as:
> 
> --8<---------------cut here---------------start------------->8---
> diff --git
> a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> index 0d5dd4f6986d..1512b73a985d 100644
> --- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> +++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> @@ -325,6 +325,18 @@ enum class BindFlags {
>      Device,
>  };
>  
> +static void bindSymlinksRealPath(Vector<CString>& args, const char*
> path,
> +                                 const char* bindOption = "--ro-
> bind")
> +{
> +    char realPath[PATH_MAX];
> +
> +    if (realpath(path, realPath) && strcmp(path, realPath)) {
> +        args.appendVector(Vector<CString>({
> +            bindOption, realPath, realPath,
> +        }));
> +    }
> +}
> +
>  static void bindIfExists(Vector<CString>& args, const char* path,
> BindFlags bindFlags = BindFlags::ReadOnly)
>  {
>      if (!path || path[0] == '\0')
> @@ -337,6 +349,10 @@ static void bindIfExists(Vector<CString>& args,
> const char* path, BindFlags bind
>          bindType = "--ro-bind-try";
>      else
>          bindType = "--bind-try";
> +
> +    // Canonicalize the source path, otherwise a symbolic link could
> +    // point to a location outside of the namespace.
> +    bindSymlinksRealPath(args, path, bindType);
>      args.appendVector(Vector<CString>({ bindType, path, path }));
>  }
>  
> @@ -615,17 +631,6 @@ static void bindV4l(Vector<CString>& args)
>      }));
>  }
>  
> -static void bindSymlinksRealPath(Vector<CString>& args, const char*
> path)
> -{
> -    char realPath[PATH_MAX];
> -
> -    if (realpath(path, realPath) && strcmp(path, realPath)) {
> -        args.appendVector(Vector<CString>({
> -            "--ro-bind", realPath, realPath,
> -        }));
> -    }
> -}
> -
>  // Translate a libseccomp error code into an error message.
> libseccomp
>  // mostly returns negative errno values such as -ENOMEM, but some
>  // standard errno values are used for non-standard purposes where
> their
>  --8<---------------cut here---------------end--------------->8---
Note that Webkit has a FileSystem namespace with a realPath function
that does std::filesystem::canonical already.

> Which produced the intended bwrap arguments, but unfortunately that'd
> still fail.  The issue seems to be related to attempt to bind
> /etc/pulse/client.conf over something already existing there; it can
> be simply reproduced with:
> 
> --8<---------------cut here---------------start------------->8---
> $ guix shell bubblewrap -- bwrap --ro-bind /gnu /gnu \
>     --ro-bind /etc /etc \
>     --ro-bind /etc/pulse/client.conf /etc/pulse/client.conf \
>     /gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-
> 5.1.8/bin/bash
> bwrap: Can't create file at /etc/pulse/client.conf: No such file or
> directory
> --8<---------------cut here---------------end--------------->8---
> 
> One thing to try would be to not bind mount client.conf; /etc/ is
> already bind mounted as a whole.  If the resolved paths are all bind
> mounted (which they are since we share the whole of /gnu), we should
> be OK.
Do we really need to bind all of /etc?  I think it'd make sense to try
and shrink that.

Not bind-mounting it is not a solution IIUC.  At least it appears that
is the standard quo in which the symlink is unresolved.  I think
bubblewrap simply ignores symlinks due to their inherent TOCTOU
characteristics and other "fun" things one could do with them.

> [...]
> 
> To be continued...
diff mbox

Patch

diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
index 0d5dd4f6986d..1512b73a985d 100644
--- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
@@ -325,6 +325,18 @@  enum class BindFlags {
     Device,
 };
 
+static void bindSymlinksRealPath(Vector<CString>& args, const char* path,
+                                 const char* bindOption = "--ro-bind")
+{
+    char realPath[PATH_MAX];
+
+    if (realpath(path, realPath) && strcmp(path, realPath)) {
+        args.appendVector(Vector<CString>({
+            bindOption, realPath, realPath,
+        }));
+    }
+}
+
 static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bindFlags = BindFlags::ReadOnly)
 {
     if (!path || path[0] == '\0')
@@ -337,6 +349,10 @@  static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bind
         bindType = "--ro-bind-try";
     else
         bindType = "--bind-try";
+
+    // Canonicalize the source path, otherwise a symbolic link could
+    // point to a location outside of the namespace.
+    bindSymlinksRealPath(args, path, bindType);
     args.appendVector(Vector<CString>({ bindType, path, path }));
 }
 
@@ -615,17 +631,6 @@  static void bindV4l(Vector<CString>& args)
     }));
 }
 
-static void bindSymlinksRealPath(Vector<CString>& args, const char* path)
-{
-    char realPath[PATH_MAX];
-
-    if (realpath(path, realPath) && strcmp(path, realPath)) {
-        args.appendVector(Vector<CString>({
-            "--ro-bind", realPath, realPath,
-        }));
-    }
-}
-
 // Translate a libseccomp error code into an error message. libseccomp
 // mostly returns negative errno values such as -ENOMEM, but some
 // standard errno values are used for non-standard purposes where their