Deny audio recording for all snap applications

Hey Luke, could you work with Simon to help landing that to yakkety and probably SRU to xenial (needed for snappy)?

This bug was fixed in the package pulseaudio - 1:8.0-2ubuntu2

---------------
pulseaudio (1:8.0-2ubuntu2) yakkety; urgency=medium

  [ Simon Fels ]
  * debian/patches/0700-modules-add-snappy-policy-module.patch:
    - Add initial support for a snappy specific policy manager
      which will deny all audio recording from snaps for now
      until real integration with the trust-store is available. (LP: #1583057)
  * debian/rules:
    - Build with snappy support
  * debian/pulseaudio.install:
    - Include new snappy policy module

 -- Luke Yelavich <email address hidden> Wed, 01 Jun 2016 12:04:14 +1000

@Luke: How did you test the change before landing it?

I was under the impression that this had been tested given its waiting to be landed in overlays etc.

If it has not beentested, I am happy to back it out, since I am not in a position to test this right now.

Thanks for working on this! Per the snappy team, this will also need a SRU for xenial.

Adding xenial task and marking triaged since a fix is available in yakkety. Who will be providing this update?

Ping, who will be providing this update to xenial?

@Jamie: Attached is a debdiff to update the pulse pacakge with snappy support.

Can we please try to upstream this patch? This will help with making other distributions share the security features and advantages of snaps.

@Simon, thanks, I'll work on sponsoring this.

@Zygmunt, I'm not sure this is the patch to upstream-- it is the phase 1 approach and the phase 2 approach is pulseaudion/trust-store/snappy interfaces which we will be discussing this week.

@Simon, couple of small things:
* you should use 8.0-0ubuntu3.1 as the version instead of 8.0-0ubuntu4
* the changelog has a date of 'Tue, 17 May 2016 17:59:58 +0200' which is quite old, yet the diff was only recently uploaded. You can use 'dch -r' to update the date

More importantly:
 * the patch introduces compiler warnings. These should be cleaned up:
+modules/module-snappy-policy.c: In function ‘connect_record_hook’:
+modules/module-snappy-policy.c:57:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
+ char *label = NULL;
+ ^
+modules/module-snappy-policy.c:65:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
+ pa_hook_result_t decision = PA_HOOK_OK;
+ ^

* configure shows:
+ Enable Snappy support: no
+ Enable Apparmor: yes

Can you comment on why snappy support was added to configure in this patch if you aren't going to use it? It seems that snappy support is compiled though, cause I see '-DHAVE_SNAPPY=1' in the build logs and see usr/lib/pulse-8.0/modules/module-snappy-policy.so is now shipped.

The functionality does not work as expected and I am able to record when running parecord under an apparmor profile that starts with 'snap.' (see attached).

I should mention that when testing this installed test packages then logged out of my session, killed my user's pulseaudio then logged back in. I suppose I could have also done 'killall pulseaudio' and have it restart automatically instead.

@Simon, finally, in reading the patch this will affect both strict and devmode and so the patch should "if startswith 'snap.' and process is in enforce mode ; then block recording".

This will be needed for the phase 2 implementation as well, so it is not wasted effort. I've asked the apparmor devs to comment in the trello card if there is an apparmor API for this. Be cognizant of TOUCTOU here but also understand that a snap is not able to change its enforcement mode so a TOCTOU is not security relevant for this particular change.

@Simon, per the SRU process, I've done the paperwork to pursue the SRU but leaving this as 'In Progress' due to my comments. Please attach an updated debdiff and I'll review and adjust the bug as appropriate.

@Jamie: Works fine here for me. Using a simple snap

name: pulseaudio-clients
version: 8.0-1
summary: Clients for PulseAudio
description: |
 Contains PulseAudio client utilities

apps:
  pactl:
    command: usr/bin/pactl
    plugs: [pulseaudio]
  paplay:
    command: usr/bin/paplay
    plugs: [pulseaudio]
  parec:
    command: usr/bin/parec
    plugs: [pulseaudio]

parts:
  packages:
    plugin: nil
    stage-packages:
      - pulseaudio

to test.

Make sure that the module is loaded:

simon@nirvana ~/Work/ubuntu/snappy/paplay-snap $ pactl list modules | grep snappy
 Name: module-snappy-policy

If not you can load it with

$ pactl load-module module-snappy-policy

If you now install the snap from above and run

$ pulseaudio-clients.parec

you will see the client hangs. If you unload the module

$ pactl unload-module module-snappy-policy

you will see the client can record now and doesn't hang any longer.

The only thing which might have went wrong with the package debdiff is that the module for the snappy policy wasn't added to /etc/pulse/daemon.pa. Will check that now.

Updated the patch. We now add snappy-policy always to default.pa. I couldn't figure out what I have to change to make the ifelse statement work properly to add the snappy policy module only conditionally to default.pa

Also fixed the compiler warnings and other small things.

@Simon, thanks for the updates. It looks I did not have the module-snappy-policy module loaded and appreciate the update to default.pa and the updated patch that addresses the other issues.

The only remaining issue is making sure that recording continues to work in devmode. I think you will want to use aa_getcon() for this. Please discuss with Tyler before implementing however.