[apparmor] restrictions on profile names

[John Johansen]

On 05/08/2017 08:23 AM, Tyler Hicks wrote:
> On 04/01/2017 10:51 PM, John Johansen wrote:
>> There has been work upstream to bring generic LSM stacking to the
>> Linux kernel. If this happens it will require changes to apparmor,
>> specifically around the proc/<pid>/attr interfaces that apparmor
>> shares with other lsms. Currently only a single LSM can use these
>> interfaces, however with full lsm stacking there will need to be some
>> form of sharing. The current design has these interfaces being
>> multiplexed by using lsm name tagging
>>
>> eg.
>>
>>  > cat /proc/self/attr/current
>>  lsm1='v1'lsm2='v2'lsm3='v3'
>>
>> while multiplexing these interfaces is not the only possibility, any
>> option will require changes to apparmor, and I would like to propose
>> we begin making changes that will ensure apparmor is ready no matter
>> what solution is chosen.
> 
> What are the chances that the stacking patches will be merged soon?
> They've been discussed for years. I'd hate for us to make interface
> changes for something that still isn't close to being merged.
> 

unknown, but Casey keeps inching things closer. He has already achieved
the feat of getting minor stacking merged and he is iterating the
extension. I believe if he keeps at it, it will eventually come

The goal is to be ahead of him so that any changes that come, are
not disruptive.

>> AppArmor's problem with these interfaces
>> comes down to it allows arbitrary characters to be read and written,
>> making it impossible for the generic infrastructure to parse out what
>> is/should be apparmor's. The problem can be split into two distinct
>> parts, what apparmor returns when the interface is read and what
>> apparmor allows/requires be written to the interface.
>>
>> The read problem is basically what characters are allowed in the
>> profiles name. Currently apparmor allows any character except \000 as
>> part of a profile name that begins with / (any valid file path), other
>> profile names are more restricted but the exact restrictions are
>> inconsistent and have never been formalized. Arbitrary characters for
>> a profile name are problematic as 3rd partly applications and generic
>> infrastructure must be able to cope and display the label (eg. ps,
>> pstree, ...). These applications do not handle the apparmor label well
>> when there are control characters etc, embedded. Nor does it make it
>> easy for a generic infrastructure to be able to tease apart the
>> apparmor label from other lsm labels.
>>
>> So I propose we standardize on a set of printable characters for
>> profile names. This will mean that profiles should be defined with a
>> name separate from their attachment, ie.
>>
>>   profile foo /usr/bin/bar { ... }
> 
> I agree that this is a good change. I've always been uneasy about how
> the profile names are used throughout a number of userspace projects
> even though they're not guaranteed to be printable.
> 
>> but even in cases where this is not the case we can have the parser
>> convert the name to a valid set using a well defined
>> transform. Somewhat like the standard we already employ for profile
>> files in /etc/apparmor.d/.  However even with such support we should
>> encourage the movement to none attachment based profile names, as they
>> are easier to write policy around and shorter (which is beneficial
>> when stacking is used).
> 
> What's the justification for performing a transform? Why couldn't we
> simply not allow non-printable names and let the profile author handle
> the transform in a way that's known and predictable to them?
> 
because we currently don't disallow non-printable names, so we are making
sure they can't break us in the kernel. I am very in favor of changing
it so they just aren't allowed.

>> The write issue is harder, in that \000 is allowed, even required, and
>> we use the size of the write to determine how much input their really
>> is. This is required to continue to allow interfaces like change_hatv
>> to continue to work, so instead of restricting the write input I
>> propose we move the writes to a custom apparmor interface. For most
>> applications the details can be hidden behind libapparmor. Basically
>> we would provide
>>
>>   apparmor/current
>>   apparmor/exec
>>   apparmor/prev
>>
>> files in apparmorfs. Since writes are already restricted to be done
>> only by the current task, we don't loose anything by not writing to
>> the tasks attr. Once this interface is up, and the library is using
>> it, we can deprecate writing to the proc interface in apparmor,
>> defaulting to reject writes, but providing a sysctl to allow them. In
>> this way we can find and migrate direct users of the current interface
>> to the new one without breaking them.
> 
> IMO, this is the most disruptive change discussed in this email and I
yes, hence do it gradual. Add new interface, and mechanism to warn,
add control to ensure old behavior. I don't envision support for the old
interface going away for years, maybe never unless major stacking is
enabled, which under the current scheme can be discovered dynamically.

Its also possible that perhaps change_hatv when stacked is the only
one we require to use the new interface (especially if we apply the
profile name restriction). But I think there are efficiency reason's
to move our own stuff off of it.

> think we need to think carefully about it. Is aa_change_hatv() the only
> thing affected? If so, I think I'd prefer libapparmor's aa_change_hatv()
Its the only one that relies on inline null characters

> implementation to be updated to iterate over the list of hats and
> effectively do an aa_change_hat() on each one of them instead of handing
> the raw byte sequence, which contains NUL's, over to the kernel.
>