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

[John Johansen]

On 05/19/2015 02:41 PM, Tyler Hicks wrote:
> 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...
> 
No it works
it shows up just as shown above, ie. you get profile1 with a separator and
then the intro prefix.

  A//&B//&C  (ABC)

where A, B, C in the mode are the single char representative of the mode
for the corresponding profile.

you can also ask the kernel for permissions on the query interface for
  A//&B//&C

it it will happily oblige

what the query interface won't currently do is create a new label that
doesn't currently exist.  So if A//&B//&C does not exist on some object
yet it will respond with -EEXIST.  It would be a trivial change to allow
the query interface to allow querying combinations that don't exist,
but I haven't seen the need to allow that yet.

Btw this will also work across namespaces
  A//&:ns://B

but the answer is currently unreliable do to issues around cross namespace
permissions. But should work in the future.

Now it may be that we may want to add iterator support to the library
for tasks that want to separate the profiles in a stack, and look at things
individually but I think the current interface works for all uses atm.