Bug 45656 - lacks handling of (not-so-)special cases in pa_make_secure_dir()
Summary: lacks handling of (not-so-)special cases in pa_make_secure_dir()
Status: RESOLVED MOVED
Alias: None
Product: PulseAudio
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: All Linux (All)
: low normal
Assignee: pulseaudio-bugs
QA Contact: pulseaudio-bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-05 13:16 UTC by Michael Shigorin
Modified: 2018-07-30 10:09 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
strace output (59.44 KB, text/plain)
2012-02-05 13:16 UTC, Michael Shigorin
Details
Proposed (partial) fix to chown problem (623 bytes, patch)
2014-04-23 18:58 UTC, Bradley Broom
Details | Splinter Review

Description Michael Shigorin 2012-02-05 13:16:31 UTC
Created attachment 56630 [details]
strace output

As of pulseaudio-1.1, both src/daemon/main.c::change_user() and src/pulsecore/core-util.c::pa_make_secure_dir() are pretty ignorant of target directory being already there with proper permissions and rush to mkdir()/fchown()/fchmod() for no good reason.

My original problem occurs on r/o NFSv3 Linux 2.6.32 thin client root filesystem while trying to run pulseaudio --system (used to work like charm with 0.9.5):

# pulseaudio --system
W: [pulseaudio] main.c: Running in system mode, but --disallow-exit not set!
W: [pulseaudio] main.c: Running in system mode, but --disallow-module-loading not set!
N: [pulseaudio] main.c: Running in system mode, forcibly disabling SHM mode!
N: [pulseaudio] main.c: Running in system mode, forcibly disabling exit idle time!
E: [pulseaudio] core-util.c: Failed to create secure directory: Operation not permitted

Here's localhost test re-run with r/w ext4 rootfs:

# pulseaudio --system -v -v
W: [pulseaudio] main.c: Running in system mode, but --disallow-exit not set!
W: [pulseaudio] main.c: Running in system mode, but --disallow-module-loading not set!
N: [pulseaudio] main.c: Running in system mode, forcibly disabling SHM mode!
N: [pulseaudio] main.c: Running in system mode, forcibly disabling exit idle time!
D: [pulseaudio] core-rtclock.c: Timer slack is set to 50 us.
D: [pulseaudio] core-util.c: setpriority() worked.
I: [pulseaudio] core-util.c: Successfully gained nice level -11.
I: [pulseaudio] main.c: Found user 'pulse' (UID 144) and group 'pulse' (GID 56).
I: [pulseaudio] main.c: Successfully dropped root privileges.
I: [pulseaudio] main.c: This is PulseAudio 1.1
D: [pulseaudio] main.c: Compilation host: i586-alt-linux-gnu
D: [pulseaudio] main.c: Compilation CFLAGS: -pipe -Wall -g -O2 -march=i586 -mtune=i686 -W -Wextra -Wno-long-long -Wvla -Wno-overlength-strings -Wunsafe-loop-optimizations -Wundef -Wformat=2 -Wlogical-op -Wsign-compare -Wformat-security -Wmissing-include-dirs -Wformat-nonliteral -Wpointer-arith -Winit-self -Wdeclaration-after-statement -Wfloat-equal -Wmissing-prototypes -Wredundant-decls -Wmissing-declarations -Wmissing-noreturn -Wshadow -Wendif-labels -Wcast-align -Wstrict-aliasing -Wwrite-strings -Wno-unused-parameter -ffast-math -Wp,-D_FORTIFY_SOURCE=2 -fno-common -fdiagnostics-show-option
D: [pulseaudio] main.c: Running on host: Linux i686 3.2.2-std-pae-alt1 #1 SMP Wed Feb 1 06:39:46 UTC 2012
D: [pulseaudio] main.c: Found 2 CPUs.
I: [pulseaudio] main.c: Page size is 4096 bytes
D: [pulseaudio] main.c: Compiled with Valgrind support: no
D: [pulseaudio] main.c: Running in valgrind mode: no
D: [pulseaudio] main.c: Running in VM: no
D: [pulseaudio] main.c: Optimized build: yes
D: [pulseaudio] main.c: All asserts enabled.
I: [pulseaudio] main.c: Machine ID is afe81388ef56429371ec614748402b92.
E: [pulseaudio] core-util.c: Failed to create secure directory: Operation not permitted
# getent passwd pulse
pulse:x:144:56:Pulseaudio daemon:/var/run/pulse:/dev/null
# ls -ld /var/run/pulse      
drwxrwx--x 2 root pulse 4096 Nov 17 15:46 /var/run/pulse

/var/run/pulse (0771,root,pulse) and the user/group prepared by ALT Linux pulseaudio-system subpackage in both cases.

Syscalls up to the finishing rmdir() (included as a beacon not as a culprit, full strace output attached):

umask(022)                              = 022
mkdir("/var/run/pulse", 0755)           = -1 EEXIST (File exists)
umask(022)                              = 022
open("/var/run/pulse", O_RDONLY|O_NOCTTY|O_LARGEFILE|O_NOFOLLOW|O_CLOEXEC) = 3
fstat64(3, {st_mode=S_IFDIR|0771, st_size=4096, ...}) = 0
getuid32()                              = 144
getgid32()                              = 56
fchown32(3, 144, 56)                    = -1 EPERM (Operation not permitted)
rmdir("/var/run/pulse")                 = -1 EACCES (Permission denied)

See also:
http://pulseaudio.org/ticket/539
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=628033

Seen also:
https://bugzilla.redhat.com/show_bug.cgi?id=508072
https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2009-1299
Comment 1 Michael Shigorin 2012-02-06 10:41:42 UTC
Sorry, for now it's rather up to ALT package differences: http://git.altlinux.org/people/sbolshakov/packages/?p=pulseaudio.git;a=commitdiff;h=b83171ee668c832a750c96b77866827fa9e43130;hp=3cea9f3032a4ae65030e74c4ffdf1547e3f3a2d3

Although it would be nice to check if the directory's fine before rushing to craft it into shape.
Comment 3 Colin Guthrie 2012-03-13 17:52:10 UTC
This should be fairly easy to catch for 2.0
Comment 4 Colin Guthrie 2012-04-10 03:40:09 UTC
OK, so I've just tested this.

I started "pulseaudio --system" as root and it correctly dropped privs to the pulse user.

The runtime dir was set to /var/run/pulse which I had pre-created to be owned by pulse.pulse. Likewise the pulse users homedir was set to be /var/lib/pulse which was also the state dir as per compilation.

When the user was added, the dir had these permissions:
drwxr-xr-x 5 pulse pulse 4096 Apr 10 10:47 /var/lib/pulse/


After launching PA, it changed to these permissions:
drwx------ 5 pulse pulse 4096 Apr 10 10:48 /var/lib/pulse/


The runtime dir was also handled fine. PA ensured the permissions were:
drwxr-xr-x 2 pulse pulse 4096 Apr 10 10:47 /var/run/pulse/


Where I could replicate failure is when /var was a read-only filesystem.

Now this is, in itself, not really something that should be supported. The whole point of /var is that it is variable. It does not make sense to mount /var read-only. /usr yes, but not /var:

/var/lib

    Files that change while the system is running normally.

So I'm not sure we should go out of our way to support this. The runtime dir is even more variable, as /var/run is used for multiple applications to record transient state. These days it is a symlink to /run which is mounted in tmpfs.


That said, I agree that we could try harder to not fail when the perms are correct to begin with. i.e. do a check first and miss out the chown/chmods/mkdir if all is correct already.

I'd happily take a patch for that, but the one on arch is incorrect in this regard. We should still insist on the known-good perms, we just shouldn't fail if they are already like that and we cannot call mkdir.

Anyway, this is a very specific use case (the readonly /var - NFS root systems should use e.g. tmpfs, aufs or unionfs to make these directories r/w even if the changes are ultimately lost - there are various scripts to do this on Fedora and Mageia etc - Personally I run just such a setup for my own media centre), and as such it's something I'm not going to be able to personally look at for v2.0. If someone wants to provide a good patch, I'll happily merge it. Removing from 2.0 blockers.
Comment 5 Felipe Sateler 2014-04-04 20:01:44 UTC
Hi,

I'm piggybacking on this issue to comment that pa_make_secure_dir still doesn't handle the pulse dir being a symlink (as of 5.0). I tested this and pulseaudio errored with "too many levels of symbolic links" when ~/.pulse is a symlink.
Comment 6 Bradley Broom 2014-04-23 18:58:15 UTC
Created attachment 97826 [details] [review]
Proposed (partial) fix to chown problem


The attached patch only attempts to issue the chown if the target does not have the desired uid or gid.

It fixes the 'Failed to create secure directory' error I was receiving on my NFS mounted home directory.

Please test it out.
Comment 7 Tanu Kaskinen 2014-04-25 10:03:10 UTC
(In reply to comment #6)
> Created attachment 97826 [details] [review] [review]
> Proposed (partial) fix to chown problem
> 
> 
> The attached patch only attempts to issue the chown if the target does not
> have the desired uid or gid.
> 
> It fixes the 'Failed to create secure directory' error I was receiving on my
> NFS mounted home directory.
> 
> Please test it out.

Patch applied: http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=5610d41482df995baaf510308e07ccbe04c9e18b
Comment 8 Peter Meerwald 2014-11-17 09:02:59 UTC
this should probably be set to 'resolved'; what is missing?
Comment 9 Michael Shigorin 2014-11-17 10:35:15 UTC
Should be fine, I didn't have a chance to re-test it within that specific NFS-root environment but feel free to close the bug based on Bradley's patch having been applied -- I'll revisit it if still needed.

Thanks everyone involved :)
Comment 10 Felipe Sateler 2014-11-18 13:25:23 UTC
Pulseaudio still fails when ~/.pulse is a symlink.

For some reason pa_make_secure_dir opens with O_NOFOLLOW if it is available. As I do not know the reason I cannot say if simply removing the flag is reasonable, but removing it makes pa work when ~/.pulse is a symlink to a directory.
Comment 11 Tanu Kaskinen 2014-12-19 13:57:58 UTC
Sorry, I closed this bug prematurely. I don't know either if there's some valid reason to use O_NOFOLLOW. Until there's clarity, it makes sense to keep this bug open.
Comment 12 Marc Mengel 2016-05-16 20:28:55 UTC
I'll pile on here, for the moment; there's one other related issue; 
I've recently ended up on a new configured-to-be-ultra-secure(?) 
shared NFS home area where I cannot change group ownership.
(it's actually kerberos nfs4 ACL's, and thus they want the group 
permissions turned off...) 

Now pulseaudio complains that it cannot make a secure directory,
because it tries to chgrp it, and that fails.  The funny thing is,
it makes it mode 0700, so the group ownership is moot anyhow.

So I propose the following:
*** ./src/pulsecore/core-util.c.orig	2016-05-16 15:13:39.281228492 -0500
--- ./src/pulsecore/core-util.c	2016-05-16 14:39:39.193214811 -0500
***************
*** 268,274 ****
  #ifndef OS_IS_WIN32
      if (!S_ISDIR(st.st_mode) ||
          (st.st_uid != uid) ||
!         (st.st_gid != gid) || 
          ((st.st_mode & 0777) != m)) {
          errno = EACCES;
          goto fail;
--- 268,275 ----
  #ifndef OS_IS_WIN32
      if (!S_ISDIR(st.st_mode) ||
          (st.st_uid != uid) ||
!          /* we only care about gid if its being used... */
!         (st.st_gid != gid && st.st_mode & 070 != 0) || 
          ((st.st_mode & 0777) != m)) {
          errno = EACCES;
          goto fail;

This works great for me; basically we don't check the group ownership
if the group bits are turned off in the mode; and voila, lovely audio again.
Comment 13 Arun Raghavan 2017-06-07 11:10:18 UTC
This came up on IRC again, and henrk pointed out this old bug that explains why we might want to keep O_NOFOLLOW:

  https://web.archive.org/web/20101129201521/http://pulseaudio.org/ticket/662

So we have two choices, I think -- continue to not allow symlinks and close this out, or have throw out a message about possibly opening up your PA for access by any user if the directory is a symlink.

Thoughts?
Comment 14 henrk 2017-06-07 11:37:54 UTC
Hello, the use case for the symlinked ~/.config/pulse is following:

I use GNU Stow for managing my dotfiles and I set it up in the following way. I have a git repo in "dotflies_location" where I track "dotflies_location/base/.config/pulse/default.pa" and then I use stow to install it on my machines by "stow -t ~ base". That creates symlink to the pulse folder in ~/.config, if the pulse folder is not there.
Comment 15 Tanu Kaskinen 2017-06-08 23:19:26 UTC
I re-read the discussion in this bug, and Marc's patch in comment 12 seems like a sensible thing to do (sorry, Marc, for ignoring you for so long).

Now, regarding O_NOFOLLOW, I don't quite understand why Lennart was so strongly against symlinks. As far as I can tell, the purpose of O_NOFOLLOW is just to avoid trouble *after* the user has already shot himself in the foot by creating a symlink that others can modify. That's a nice thing to do, but if the extra caution causes trouble, then it's probably not worth it. I don't see any security problems if the symlink is in the user's home directory where others can't write. I'm in favour of removing the O_NOFOLLOW flag. (Maybe it should be removed from pa_lock_file() and pid.c:open_pid_file() too?)
Comment 16 GitLab Migration User 2018-07-30 10:09:17 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/242.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.