Re: [libvirt] [PATCH] AppArmor: allow QEMU to set_process_name.

On Mon, 2016-12-05 at 17:30 +0100, Christian Ehrhardt wrote: > > On Mon, Dec 5, 2016 at 12:21 PM, intrigeri <intrigeri+libvirt(a)boum.org&gt; > wrote: > > > > > > > +  @{PROC}/@{pid}/task/@{tid}/comm rw, > > > Hi, > we have used the following for now that we planned to submit soon: > owner @{PROC}/@{pid}/task/[0-9]*/comm rw > > But I like yours more since you are adding the explicit TID instead of a > pattern. > I'm convinced you confirmed your fix working, but I wonder if might want to > consider the "owner" part we had. > Note that @{pid} and @{tid} may be causing some confusion. Today, these are simple pattern matches in anticipation of AppArmor kernel variables. From /etc/apparmor.d/tunables/kernelvars: # until kernel vars are implemented # and until the parser supports nested groupings like #   @{pid}=[1-9]{[0-9]{[0-9]{[0-9]{[0-9]{[0-9],},},},},} # use @{pid}={[1-9],[1-9][0-9],[1-9][0-9][0-9],[1-9][0-9][0-9][0-9],[1-9][0-9][0- 9][0- 9][0-9],[1-9][0-9][0-9][0-9][0-9][0-9]} #same pattern as @{pid} for now @{tid}=@{pid} As such, while intrigeri's use of @{tid} is more correct for the long term, it is not (appreciably) different from Christian's use of [0-9]*. This means that either rule will allow modifying other process' 'comm' value. Since libvirt is *usually* configured to launch VMs as non-root (eg. the 'libvirt-qemu' group on Debian and its derivatives) there is some mitigation with owner match, but it does mean that any libvirt managed qemu process can update the 'comm' value of any other libvirt managed qemu process. For systems that are configured to launch qemu under root, then the rule means a qemu process is able to modify the 'comm' value of any other root process. Either way, this breaks qemu isolation and as a result I think it is wrong to add it to the libvirt-qemu abstraction until kernel variables are available. The best we can do at this point is conditionally add the owner rule per VM or per invocation of the command that runs set_process_name. The safest would be to add the rule when set_process_name is called, then remove it after with a profile reload. I'm unfamiliar with set_process_name and when it is run so I can't say if this is a viable approach or not. If it is not possible to update the profile in this manner, then at a minimum there should be a FIXME comment above this rule discussing all of this.