[apparmor] [PATCH 26/32] apparmor: fix fully qualified name parsing

[John Johansen]

On 01/31/2013 05:11 PM, Seth Arnold wrote:
> On Wed, Jan 16, 2013 at 01:28:55PM -0800, John Johansen wrote:
>> currently apparmor name parsing is only correctly handling
>> :<NS>:<profile>
>>
>> but
>> :<NS>://<profile>
>>
>> is also a valid form and what is exported to userspace.
>>
>> Signed-off-by: John Johansen <john.johansen at canonical.com>
>> ---
>>  security/apparmor/lib.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Hrm, I think the current code and this change aren't very resilient to
> an illegal input; what happens if the input is:
> 
> :<ns>:
> 
name is first set to the trailing \0 and then, detected and set to NULL
below. A null name is valid, for most of the cases where this is used
and means use the specified namespace but use the name of the current profile.

so if you are doing change_profile and are in the unconfined profile
you can switch to a new namespace by just specifying

  :new_ns:



> It feels like the : would be over-written with \0, but name would be set
> to the next byte -- not in the input string.
> 
> I looked at the callers only briefly, but it didn't look like anything
> would shield this code from bad inputs.
> 
The code does make the assumption that their is a trailing \0 on the string
which is should be done by the various callers.

The name will be set to the input following which could indeed be invalid
for a name, but that should just cause lookup failures because of where
it is used.

A string of null length is guarded again below with
	if (name && *name == 0)
		name = NULL;

It certainly wouldn't hurt to revisit this function and a few others to
have them better enforce the rules about what a profile name can be.
But that can be a separate patch



>> diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
>> index d6e1f21..d40bc59 100644
>> --- a/security/apparmor/lib.c
>> +++ b/security/apparmor/lib.c
>> @@ -45,8 +45,10 @@ char *aa_split_fqname(char *fqname, char **ns_name)
>>  		*ns_name = skip_spaces(&name[1]);
>>  		if (split) {
>>  			/* overwrite ':' with \0 */
>> -			*split = 0;
>> -			name = skip_spaces(split + 1);
>> +			*split++ = 0;
>> +			if (strncmp(split, "//", 2) == 0)
>> +				split += 2;
>> +			name = skip_spaces(split);
>>  		} else
>>  			/* a ns name without a following profile is allowed */
>>  			name = NULL;
> 
> Thanks
> 
> 
>