[apparmor] [PATCH 7/6] libapparmor: Strip a trailing newline character in aa_splitcon(3)

[Tyler Hicks]

On 2015-05-19 12:21:17, John Johansen wrote:
> On 05/19/2015 07:32 AM, Tyler Hicks wrote:
> > Adjust the internal splitcon() function to strip a single trailing
> > newline character when the bool strip_newline argument is true.
> > 
> > aa_getprocattr_raw(2) needs to set strip_newline to true since the
> > kernel appends a newline character to the end of the AppArmor contexts
> > read from /proc/>PID>/attr/current.
> > 
> > aa_splitcon(3) also sets strip_newline to true since it is unknown
> > whether the context is originated from a location that appends a newline
> > or not.
> > 
> > aa_getpeercon_raw(2) does not set strip_newline to true since it is
> > unexpected for the kernel to append a newline to the the buffer returned
> > from getsockopt(2).
> > 
> > This patch also creates tests specifically for splitcon() and updates
> > the aa_splitcon(3) man page.
> > 
> > Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> > ---
> >  libraries/libapparmor/doc/aa_splitcon.pod |   7 ++
> >  libraries/libapparmor/src/kernel.c        |  50 ++++++----
> >  libraries/libapparmor/src/tst_kernel.c    | 161 ++++++++++++++++++++++++------
> >  3 files changed, 166 insertions(+), 52 deletions(-)
> > 
> > diff --git a/libraries/libapparmor/doc/aa_splitcon.pod b/libraries/libapparmor/doc/aa_splitcon.pod
> > index a85b619..2a46dd2 100644
> > --- a/libraries/libapparmor/doc/aa_splitcon.pod
> > +++ b/libraries/libapparmor/doc/aa_splitcon.pod
> > @@ -40,6 +40,11 @@ terminated. The enforcement mode is also NUL terminated and the parenthesis
> >  surrounding the mode are removed. If @mode is non-NULL, it will point to the
> >  first character in the enforcement mode string on success.
> >  
> > +The Linux kernel's /proc/<PID>/attr/current interface appends a trailing
> > +newline character to AppArmor contexts that are read from that file. If @con
> > +contains a single trailing newline character, it will be stripped by
> > +aa_splitcon() prior to all other processing.
> > +
> >  =head1 RETURN VALUE
> >  
> >  Returns a pointer to the first character in the label string. NULL is returned
> > @@ -50,7 +55,9 @@ on error.
> >   Context                        Label               Mode 
> >   -----------------------------  ------------------  -------
> >   unconfined                     unconfined          NULL
> > + unconfined\n                   unconfined          NULL
> >   /bin/ping (enforce)            /bin/ping           enforce
> > + /bin/ping (enforce)\n          /bin/ping           enforce
> >   /usr/sbin/rsyslogd (complain)  /usr/sbin/rsyslogd  complain
> > 
> 
> so this has got me thinking as to what behavior we want to show here. First
> what of for processes that have a profile in the unconfined mode
> 
>   /bin/ping (unconfined)           /bin/ping            unconfined?
> 
> or do we want to be hiding that info from the kernel? Generally I think it is useful
> for introspection purposes to know that the profile name separate from the mode.
> If we have
>   /bin/ping (unconfined)
>   /foo/bar  (unconfined)
> 
> they both get treated as unconfined for enforcement purposes but as soon as /bin/ping
> is replaced with an enforcing profile, we get
>   /bin/ping (enforce)
>   /foo/bar  (unconfined)
> 
> from an introspection pov, it is nice to know that when /bin/ping is replaced it will
> get enforced. It will be difficult for users if they see all the processes with
> /bin/ping and /foo/bar lumped together as one confinement (unconfined) and then, when
> replacing one of those profiles get half of them split back out.
> 
> 
> with that said we still have the unconfined profile (which has a mode of unconfined that
> is not reported), do we leave it as
>   unconfined		unconfined		null
> or do
>   unconfined		unconfined		unconfined

John and I spoke about the above topic in #apparmor and I wanted to be
sure to record it on the list. We came to the conclusion that the kernel
should start injecting " (unconfined)" onto the end of profiles in the
unconfined mode when displaying the AppArmor context.

No changes to libapparmor are expected to be needed. Some of the utils
will likely need changes. All known "trusted helpers" (userspace code
that makes decisions based on AppArmor contexts) will need to be audited
to make sure that they treat the unconfined profile correctly.

> So the other problem is stacked profiles
>   /bin/foo//&/bin/bar	(EE)			EE?   or just enforce?
>   /bin/A//&/bin/C	(EC)			EC?
> 
> kernel side currently we report an abbreviated single character mode for each profile
> in the stack, but we could report a single mode if they are all the same like in the EE
> case.

We didn't discuss this in IRC.

I need to think about this one some more. My first thought is that the
current aa_get*con() API isn't going to work with stacked profiles.
We'll at least need an iterator function...

Tyler
-------------- 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/20150519/46f63b44/attachment.pgp>