[apparmor] [PATCH 03/14] add query helper for link permissions

[John Johansen]

On 06/04/2015 07:08 AM, Tyler Hicks wrote:
> On 2015-06-04 03:56:32, John Johansen wrote:

<<snip>>

>> +	autofree char *query = NULL;
>> +	int rc;
>> +
>> +	/* + 1 for null separators */
>> +	size_t size = AA_QUERY_CMD_LABEL_SIZE + label_len + 1 + target_len +
>> +		1 + link_len;
>> +	size_t pos = AA_QUERY_CMD_LABEL_SIZE;
>> +
>> +	query = malloc(size);
>> +	if (!query)
>> +		return -1;
>> +	memcpy(query + pos, label, label_len);
>> +	/* null separator */
>> +	pos += label_len;
>> +	query[pos] = 0;
>> +	query[++pos] = AA_CLASS_FILE;
>> +	memcpy(query + pos + 1, link, link_len);
>> +	rc = aa_query_label(AA_MAY_LINK, query, size, allowed, audited);
>> +	if (rc || !*allowed)
>> +		return rc;
>> +	pos += 1 + link_len;
>> +	query[pos] = 0;
>> +	memcpy(query + pos + 1, target, target_len);
>> +	return aa_query_label(AA_MAY_LINK, query, size, allowed, audited);
> 
> We would be throwing away the value of 'audited' from the first query.
> Is it possible to have the first query result in allowed being true and
> audited being true but then the second query result in audited being
> false (regardless of allowed)?
> 
yes, we throw the first 'audited' away. The link rule shows up a limitation
of the current query interface. In the kernel there isn't 2 separate queries,
what happens is the kernel walks the first path, check its link bit and if it
isn't set it bails out early, otherwise the target path is then walked as
a tail conditional and the perms there used.

For userspace purposes we could get away with only making the second query,
and as long as everything is being constructed correctly it wouldn't make
a difference. However it is possible to construct a dfa without the link perm
at the correct place on the link path and set on the target (it would be a
bug in the parser).

With this requiring 2 round trips to the kernel, I think maybe instead of
trying to mimic what the kernel does we should just do the final query



>> +}
>> +
>> +/**
>> + * aa_query_link_path - query access permissions for a hard link @link
>> + * @label: apparmor label
>> + * @target: file path that hard link will point to
>> + * @link: file path of hard link
>> + * @allowed: upon successful return, will be 1 if query is allowed and 0 if not
>> + * @audited: upon successful return, will be 1 if query should be audited and 0
>> + *           if not
>> + *
>> + * Returns: 0 on success else -1 and sets errno. If -1 is returned and errno is
>> + *          ENOENT, the subject label in the query string is unknown to the
>> + *          kernel.
>> + */
>> +int aa_query_link_path(const char *label, const char *target, const char *link,
>> +		       int *allowed, int *audited)
>> +{
>> +	return aa_query_link_path_len(label, strlen(label), target,
>> +				      strlen(target), link, strlen(link),
>> +				      allowed, audited);
>> +}
>> diff --git a/libraries/libapparmor/src/libapparmor.map b/libraries/libapparmor/src/libapparmor.map
>> index 8a3c60b..5f123a6 100644
>> --- a/libraries/libapparmor/src/libapparmor.map
>> +++ b/libraries/libapparmor/src/libapparmor.map
>> @@ -56,6 +56,8 @@ APPARMOR_2.10 {
>>    global:
>>          aa_query_file_path;
>>          aa_query_file_path_len;
>> +        aa_query_link_path;
>> +	aa_query_link_path_len;
> 
> Looks like the aa_query_link_path_len line uses a tab when all other
> lines use spaces. Just update this in your local patch queue.
> 
> Tyler
> 
>>          aa_features_new;
>>          aa_features_new_from_string;
>>          aa_features_new_from_kernel;
>> diff --git a/libraries/libapparmor/swig/SWIG/libapparmor.i b/libraries/libapparmor/swig/SWIG/libapparmor.i
>> index c98cca8..98f984f 100644
>> --- a/libraries/libapparmor/swig/SWIG/libapparmor.i
>> +++ b/libraries/libapparmor/swig/SWIG/libapparmor.i
>> @@ -44,5 +44,11 @@ extern int aa_query_file_path_len(uint32_t mask, const char *label,
>>  				  size_t path_len, int *allowed, int *audited);
>>  extern int aa_query_file_path(uint32_t mask, const char *label,
>>  			      const char *path, int *allowed, int *audited);
>> +extern int aa_query_link_path_len(const char *label, size_t label_len,
>> +				  const char *target, size_t target_len,
>> +				  const char *link, size_t link_len,
>> +				  int *allowed, int *audited);
>> +extern int aa_query_link_path(const char *label, const char *target,
>> +			      const char *link, int *allowed, int *audited);
>>  
>>  %exception;
>> -- 
>> 2.1.4
>>
>>
>> -- 
>> AppArmor mailing list
>> AppArmor at lists.ubuntu.com
>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor