Race in load-module snap policy check in classic confinement

[lawl]

SUMMARY
=========
When running a snap in classic confinement, that needs access to PA_COMMAND_LOAD_MODULE and PA_COMMAND_UNLOAD_MODULE. These sometimes succeed and sometimes fail with "Access denied".

After running "pacmd unload-module module-snap-policy" and unloading the snap policy module, these work reliably.

I have verified this in a fresh install of Ubuntu 20.04 in a VM.

STEPS TO REPRODUCE
=========
a) Either build a snap with classic confinement that sends these commands on the pulseaudio native protocol socket. (This is how I found the bug)
b) Or, what I did here to easier reproduce, abuse the sandbox of a random classic snap:

Download the attached bug.tgz with a minimal reproducer. It contains the source code for a program that sends load and unload commands to pulse. Unfortunately `pacmd` has a pid-file check that fails inside the sandbox and doesn't work. The reproducer does essentially the same as "pacmd load/unload-module" though.

(a pre-compiled x64 binary is also included in case you don't have a go compiler and dare to run an untrusted binary in a VM)

Unpack the tgz, build it, if necessary with "go mod download && go build"

Grab a random classic mode snap to use its sandbox as a test bed:

    $ sudo snap install atom --classic
    atom 1.48.0 from Snapcrafters installed

Open a shell in its sandbox:

    snap run --shell atom

Navigate to the compiled binary and execute it a few times:

    user@user-Standard-PC-Q35-ICH9-2009:~/bug$ ./bug
    2020/07/08 18:46:10 PulseAudio connection created successfully
    2020/07/08 18:46:10 Couldn't load module, error message: PulseAudio error: commandLoadModule -> Access denied
    user@user-Standard-PC-Q35-ICH9-2009:~/bug$ ./bug
    2020/07/08 18:46:11 PulseAudio connection created successfully
    Loaded Module sucessfully at index: 40
    user@user-Standard-PC-Q35-ICH9-2009:~/bug$ ./bug
    2020/07/08 18:46:12 PulseAudio connection created successfully
    Loaded Module sucessfully at index: 41
    user@user-Standard-PC-Q35-ICH9-2009:~/bug$ ./bug
    2020/07/08 18:46:12 PulseAudio connection created successfully
    2020/07/08 18:46:12 Couldn't load module, error message: PulseAudio error: commandLoadModule -> Access denied
    user@user-Standard-PC-Q35-ICH9-2009:~/bug$ ./bug
    2020/07/08 18:46:14 PulseAudio connection created successfully
    2020/07/08 18:46:14 Couldn't load module, error message: PulseAudio error: commandLoadModule -> Access denied
    user@user-Standard-PC-Q35-ICH9-2009:~/bug$ ./bug
    2020/07/08 18:46:14 PulseAudio connection created successfully
    2020/07/08 18:46:14 Couldn't load module, error message: PulseAudio error: commandLoadModule -> Access denied
    user@user-Standard-PC-Q35-ICH9-2009:~/bug$ ./bug
    2020/07/08 18:46:15 PulseAudio connection created successfully
    2020/07/08 18:46:15 Couldn't load module, error message: PulseAudio error: commandLoadModule -> Access denied

Succeeds and fails apparently at random.

Now from a non-sandboxed shell, run

    pacmd unload-module module-snap-policy

to unload the snap-policy module from pulseaudio, now run ./bug a few more times. It now succeeds reliably, every time.

Side note, with the real program on my actual machine, the race seems to behave slightly differently. It seems not to work the first time an application is started, but closing it and reopening it seems to make it work pretty reliably afterwards. Restarting "snapd", causes the following run the snap to fail again.

EXPECTED BEHAVIOUR
==================

The pulseaudio snap policy module should correctly determine and enforce it's policy.

ACTUAL BEHAVIOUR
================

The pulseaudio snap policy module seemingly at random denies access when the snap has the permissions to do an operation.

ADDITIONAL INFORMATION
======================

$ lsb_release -rd
Description: Ubuntu 20.04 LTS
Release: 20.04

$ apt-cache policy pulseaudio
pulseaudio:
  Installed: 1:13.99.1-1ubuntu3.3
  Candidate: 1:13.99.1-1ubuntu3.3
  Version table:
 *** 1:13.99.1-1ubuntu3.3 500
        500 http://ch.archive.ubuntu.com/ubuntu focal-updates/main amd64 Packages
        100 /var/lib/dpkg/status
     1:13.99.1-1ubuntu3.2 500
        500 http://security.ubuntu.com/ubuntu focal-security/main amd64 Packages
     1:13.99.1-1ubuntu3 500
        500 http://ch.archive.ubuntu.com/ubuntu focal/main amd64 Packages

[James Henstridge (jamesh)]

I think there's two issues at play here.

The hooks we added for module loading/unloading as part of USN-4355-1 simply check if the client has an AppArmor label that looks like it belongs to a snap and denies access if found. This will also deny access to classic snaps, which is probably a mistake.

The race condition you've encountered is probably a case of "policy module not in effect" vs. "policy module in effect" rather than a race in the behaviour of the policy module itself. This probably indicates that Pulse is servicing client requests before it has completely started.

For the first issue, we can make the hook request info about the snap and allow access to classic snaps. For the second, I think we just need to load module-snap-policy earlier during start up.

[James Henstridge (jamesh)]

I think I need to dig into this further. The fact you're seeing a few successful module loads with different module indexes would indicate it is the same Pulse Audio instance.

[lawl (lawl123)]

Yes, I do believe it's the same PA instance, I had not noticed it restarting (e.g. pavucontrol losing the connection) when testing around this bug.

Also, I'm not sure if it helps, but the module likes to spam a ton of

    pulseaudio[11451]: E: [pulseaudio] module-snap-policy.c: AppArmor profile could not be retrieved.
    pulseaudio[11451]: E: [pulseaudio] module-snap-policy.c: AppArmor profile could not be retrieved.
    pulseaudio[11451]: E: [pulseaudio] module-snap-policy.c: AppArmor profile could not be retrieved.
    pulseaudio[11451]: E: [pulseaudio] module-snap-policy.c: AppArmor profile could not be retrieved.

into the log even when *not* running as a snap. I don't know if this is related as I've found it to be harmless to the functionality of my programm when running outside snaps.

[James Henstridge (jamesh)]

Sorry for taking so long to get back to you. I now understand the non-deterministic behaviour you're seeing.

I'm working on a fix for the server side to allow classic snaps to access these commands. It will require a small change to your Pulse Audio client library to fix the non-determinism though. I'll share more information soon.

[James Henstridge (jamesh)]

This is the in-progress fix I've been working on. It does not quite work right though: switching to an async hook for these commands is resulting in the daemon killing the client on a protocol error.

This might be a problem with the hooks patch set itself. I need to investigate a bit further.

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "allow-classic.diff" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

[Launchpad Janitor (janitor)]

This bug was fixed in the package pulseaudio - 1:13.99.2-1ubuntu2.1

---------------
pulseaudio (1:13.99.2-1ubuntu2.1) groovy-security; urgency=medium

  * SECURITY UPDATE: don't rely on SCM_CREDENTIALS to detect snap confined
    clients (LP: #1895928)
    - d/p/0409-pa-client-peer-credentials.patch: drop patch
    - d/p/0409-fix-arg-parsing-after-async-hook.patch: remains of old 0409
      patch not related to pa_creds.
    - d/p/0410-pa-client-peer-apparmor-label.patch: new patch, records
      AppArmor label in pa_client struct for native connections using
      aa_getpeercon.
    - d/p/0702-add-snappy-policy-module.patch: use the AppArmor
      label in the pa_client rather than looking it up via the process ID
      from SCM_CREDENTIALS.
    - CVE-2020-16123
   * Don't block classic snaps from module loading/unloading (LP: #1886854)
    - d/p/0702-add-snappy-policy-module.patch: replace
      deny_to_snaps_hook with a version that allows classic snaps.

 -- James Henstridge <email address hidden> Thu, 05 Nov 2020 16:46:59 -0500

[Launchpad Janitor (janitor)]

This bug was fixed in the package pulseaudio - 1:13.99.1-1ubuntu3.8

---------------
pulseaudio (1:13.99.1-1ubuntu3.8) focal-security; urgency=medium

  * SECURITY UPDATE: don't rely on SCM_CREDENTIALS to detect snap confined
    clients (LP: #1895928)
    - d/p/0409-pa-client-peer-credentials.patch: drop patch
    - d/p/0409-fix-arg-parsing-after-async-hook.patch: remains of old 0409
      patch not related to pa_creds.
    - d/p/0410-pa-client-peer-apparmor-label.patch: new patch, records
      AppArmor label in pa_client struct for native connections using
      aa_getpeercon.
    - d/p/0702-add-snappy-policy-module.patch: use the AppArmor
      label in the pa_client rather than looking it up via the process ID
      from SCM_CREDENTIALS.
    - CVE-2020-16123
   * Don't block classic snaps from module loading/unloading (LP: #1886854)
    - d/p/0702-add-snappy-policy-module.patch: replace
      deny_to_snaps_hook with a version that allows classic snaps.

 -- James Henstridge <email address hidden> Thu, 15 Oct 2020 17:23:31 -0400

[Launchpad Janitor (janitor)]

This bug was fixed in the package pulseaudio - 1:11.1-1ubuntu7.11

---------------
pulseaudio (1:11.1-1ubuntu7.11) bionic-security; urgency=medium

  * SECURITY UPDATE: don't rely on SCM_CREDENTIALS to detect snap confined
    clients (LP: #1895928)
    - d/p/0409-pa-client-peer-credentials.patch: drop patch
    - d/p/0409-fix-arg-parsing-after-async-hook.patch: remains of old 0409
      patch not related to pa_creds.
    - d/p/0410-pa-client-peer-apparmor-label.patch: new patch, records
      AppArmor label in pa_client struct for native connections using
      aa_getpeercon.
    - d/p/0702-add-snappy-policy-module.patch: use the AppArmor
      label in the pa_client rather than looking it up via the process ID
      from SCM_CREDENTIALS.
    - CVE-2020-16123
   * Don't block classic snaps from module loading/unloading (LP: #1886854)
    - d/p/0702-add-snappy-policy-module.patch: replace
      deny_to_snaps_hook with a version that allows classic snaps.

 -- James Henstridge <email address hidden> Tue, 22 Sep 2020 12:30:20 +0800