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

[Liliana Marie Prikler]

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...