[pulseaudio-discuss] [PATCH v2] core-util: Fail if XDG_RUNTIME_DIR belongs to someone else

[Rémi Denis-Courmont]

Le 2014-09-08 14:32, David Henningsson a écrit :
> Usually, PA will use the PULSE_SERVER X11 property instead of using
> XDG_RUNTIME_DIR,
> so this environment variable does not matter.
>
> If this property is not available, or if one is using the pacmd cli 
> protocol,
> the client will go ahead and call pa_make_secure_dir on
> XDG_RUNTIME_DIR/pulse.
> This will either fail (if you're another regular user), or succeed
> (if you're root).
> Both scenarios are bad - failing will cause the connection to fail,
> and succeeding
> is even worse, as it can cause *other* connections to fail (as the 
> directory
> ownership has changed).
>
> Instead fail and complain loudly.
>
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=83007
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> ---
>  src/pulsecore/core-util.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> v2: Don't blame systemd anymore. Make error message translatable.
>
> diff --git a/src/pulsecore/core-util.c b/src/pulsecore/core-util.c
> index d7a95d6..6bb6317 100644
> --- a/src/pulsecore/core-util.c
> +++ b/src/pulsecore/core-util.c
> @@ -1816,6 +1816,14 @@ char *pa_get_runtime_dir(void) {
>      /* Use the XDG standard for the runtime directory. */
>      d = getenv("XDG_RUNTIME_DIR");
>      if (d) {
> +        struct stat st;
> +        if (stat(d, &st) == 0 && st.st_uid != getuid()) {

This looks like a case of ToCToU to me.

In principles, you should probably use open() then fstat(), and then 
openat to create or access files within the directory.

> +            pa_log(_("XDG_RUNTIME_DIR (%s) is not owned by us (uid
> %d), but by uid %d!\n"
> +                   "(This could e g happen if you try to connect to
> a non-root PulseAudio as a root user, over the native protocol. Don't
> do that.)"),
> +                   d, getuid(), st.st_uid);
> +            goto fail;
> +        }
> +
>          k = pa_sprintf_malloc("%s" PA_PATH_SEP "pulse", d);
>
>          if (pa_make_secure_dir(k, m, (uid_t) -1, (gid_t) -1, true) < 
> 0) {

-- 
Rémi Denis-Courmont