[apparmor] [PATCH 2/2] Set cache file tstamp to the mtime of most recent policy file tstamps

[Steve Beattie]

On Mon, Jun 08, 2015 at 04:07:44PM -0700, John Johansen wrote:
> On 06/08/2015 03:27 PM, Steve Beattie wrote:
> > On Fri, Jun 05, 2015 at 03:24:23PM -0700, John Johansen wrote:
> >> Currently the cache file has its mtime set to its creation time, but this
> >> can lead to cache issues when a policy file is updated separately from
> >> the cache file so that is possible a policy file is newer than the
> >> what the cache file was generated from but still fails the comparison
> >> because the generated cache file has a newer timestamp.
> >>
> >> Signed-off-by: John Johansen <john.johansen at canonical.com>
> > 
> > So I think this is still problematic in Ubuntu given how the
> > /etc/apparmor.d/local/ includes are implemented; they are generated
> yes timestamps are still problematic
> 
> > by dh_apparmor at package postinst time iff the local/ include file
> > doesn't already exist. This means the generated include gets an mtime
> > of the package install, and therefore so will the cache file.
> > 
> yes
> 
> > Consider the sequence of events for package foo:
> > 
> >   (1) foo version n+1 is built with a policy update for foo
> >   (2) foo version n is installed
> >       + /etc/apparmor.d/local/foo gets an mtime of (2)
> >       + therefore /etc/apparmor.d/local/foo also gets an mtime of (2)
> >   (3) foo is upgraded to version n+1
> >       + mtime of foo's cache file (2) is still newer than the policy
> >         mtime for foo (1), so the cache blob is used (incorrectly).
> >       + even if we required the mtime to be exactly the same, the cache
> > 	blob would still be (incorrectly) used (unless we recorded
> > 	the mtime for all files), because the local/foo file is the
> > 	most recently updated policy element for foo.
> > 
> yep this will fail with this
> 
> > But it's possible I might be confused.
> > 
> The case it closes is
> 1) n+1 is built
> 2) n is installed, and doesn't generate new files
> 3) upgrade to n+1
> 
> in this case the cache with have n's timestamp, the only way to fix the
> case you bring up is hashing

Alright, I looked more into how things are actually done in Ubuntu. Both
the dh_apparmor generated postinst bits and the apparmor package
postinst force the cache files to be ignored or flushed outright. So
this should be safe there.

While I too would like to go to a hashing approach, this seems like
an adequate improvement for now.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150609/c9e38fc3/attachment.pgp>