[apparmor] [PATCH RFC] Add profile-based libapparmor query interface

[John Johansen]

On 03/05/2013 10:44 PM, Tyler Hicks wrote:
> I've got an initial libapparmor patch to complement the kernel query
> interface patch that I recently sent out to the list. It is functional
> but it is quite ugly so I'm looking for suggestions on how we want this
> to look since there's not really a libapparmor precedence for an
> interface like this.
> 
> * I made this dead really as far as what information can be extracted
>   from the interface. The application using the interface will simply
>   know whether it should allow the action and whether it should audit
>   the action. My thoughts are that we can add a more complex interface
>   later when we need it. For D-Bus, I think this simple of an interface
>   is sufficient.
> 
agreed

> * I reused JJ's design from the aa_has_perm() function where the
>   application needs to allocate a query buffer of AA_SOME_HEADER_SIZE +
>   query size and then the query needs to be offset by
>   AA_SOME_HEADER_SIZE bytes in the buffer. Then, the libapparmor
>   function fills in the header and doesn't have to do any extra
>   allocations or copies. Certainly good from an efficiency point of
>   view, but not extremely user friendly. Something to worry about or
>   not?
>
this is tough, I don't like doing extra allocations if not needed but
at the same time I hate shoving this ugliness into the callers

One potential solution is to have an allocate and free routine associated
with the query fn. The alloc fn can add any headers needed and maybe
even set them up. Basically we could hide the header portion from the
caller, so it only has to worry about filling in its part.

> * Setting two int return parameters to indicate allow and audit isn't a
>   final solution. I was thinking that I'd return a special return code
>   that will indicate error or allow and audit statuses. Then define some
>   simple macros (aa_query_profile_err(rc), aa_query_profile_allow(rc),
>   aa_query_profile_audit(rc)) in apparmor.h that can be used to
>   translate the return code. I'm open to other suggestions...
> 
I'm torn on this one. I really don't like the way C handles returning anything
more than a single value, but I am not fond of simple_macros to decode things
either. What if the single return value can not encapsulate all possible
combinations in the future, etc.

It is possible to return a small well defined struct, but I am not sure that
is any better. Basically I need to think about it more.


> * Feel free to comment on any other parts of the patch, as well.
> 
> ---
> 
> Description: Add profile-based libapparmor query interface
>  Wrap the apparmorfs profile query interface with a very simple libapparmor
>  interface. This function takes a permission mask and query string consisting
>  of a profile name and a DFA match string separated by a NUL char. It sets two
>  output parameters indicating whether the action should be allowed and if the
>  action should be audited.
>  .
>  The allowed and audited output parameters take into account deny and quiet
>  permission masks returned in the kernel query. Additionally, the audited
>  ouput parameter takes into account whether the action is to be allowed or
>  not. If not, audited is set to true as long as there was no specific quiet
>  rules for the queried permission.
>  .
>  The function requires a static char array to be allocated and initialized to
>  the path of the apparmorfs .access file the first time it is called.
>  Otherwise, aa_find_mountpoint() would need to be called for every query which
>  would be inefficient. pthread_once() is used to ensure that aa_query_profile()
>  is thread-safe while the char array is being allocated and initialized.
We could just require a call to setup query once. And that would open a file handle
and we could leave the handle open, and reset queries by seeking to the beginning
instead of opening the .access file each time.

I realize this would require some kernel modifications and I am not sure whether
it is worth doing, but its an alternative.

> Author: Tyler Hicks <tyhicks at canonical.com>
> Index: apparmor-2.8.0/libraries/libapparmor/src/kernel_interface.c
> ===================================================================
> --- apparmor-2.8.0.orig/libraries/libapparmor/src/kernel_interface.c	2013-03-05 16:45:24.471541316 -0800
> +++ apparmor-2.8.0/libraries/libapparmor/src/kernel_interface.c	2013-03-05 19:43:18.524352019 -0800
> @@ -28,6 +28,8 @@
>  #include <limits.h>
>  #include <stdarg.h>
>  #include <mntent.h>
> +#include <inttypes.h>
> +#include <pthread.h>
>  
>  #include "apparmor.h"
>  
> @@ -677,3 +679,92 @@
>  {
>  	return aa_task_has_perm(aa_gettid(), mask, query, size);
>  }
> +
> +static pthread_once_t aafs_access_control = PTHREAD_ONCE_INIT;
> +static char *aafs_access = NULL;
> +
> +static void aafs_access_init_once(void)
> +{
> +	char *aafs;
> +	int ret;
> +
> +	ret = aa_find_mountpoint(&aafs);
> +	if (ret < 0)
> +		return;
> +
> +	ret = asprintf(&aafs_access, "%s/.access", aafs);
> +	if (ret < 0)
> +		aafs_access = NULL;
> +
> +	free(aafs);
> +}
> +
> +/**
> + * aa_query_profile - test if the profile being enforced allows access to query
> + * @mask: permission bits to query
> + * @query: binary query string, must be offset by AA_QUERY_CMD_PROFILE_SIZE
> + * @size: size of the query string must include AA_QUERY_CMD_PROFILE_SIZE
> + * @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
> + */
> +int aa_query_profile(uint32_t mask, char *query, size_t size,
> +		     int *allowed, int *audited)
> +{
> +	char buf[128];
> +	uint32_t allow, deny, audit, quiet;
> +	int fd, ret, saved;
> +
> +	if (size <= AA_QUERY_CMD_PROFILE_SIZE) {
> +		errno = EINVAL;
> +		return -1;
> +	}
> +
> +	ret = pthread_once(&aafs_access_control, aafs_access_init_once);
> +	if (ret) {
> +		errno = EINVAL;
> +		return -1;
> +	} else if (!aafs_access) {
> +		errno = ENOMEM;
> +		return -1;
> +	}
> +
> +	fd = open(aafs_access, O_RDWR);
> +	if (fd == -1)
> +		return -1;
> +
> +	memcpy(query, AA_QUERY_CMD_PROFILE, AA_QUERY_CMD_PROFILE_SIZE);
> +	errno = 0;
> +	ret = write(fd, query, size);
> +	if (ret != size) {
> +		if (ret >= 0)
> +			errno = EPROTO;
> +		return -1;
> +	}
> +
> +	ret = read(fd, buf, sizeof(buf));
> +	saved = errno;
> +	(void)close(fd);
> +	errno = saved;
> +	if (ret < 0)
> +		return -1;
> +
> +	ret = sscanf(buf, "allow 0x%8" SCNu32 "\n"
> +			  "deny 0x%8"  SCNu32 "\n"
> +			  "audit 0x%8" SCNu32 "\n"
> +			  "quiet 0x%8" SCNu32 "\n",
> +		     &allow, &deny, &audit, &quiet);
> +	if (ret != 4) {
> +		errno = EPROTONOSUPPORT;
> +		return -1;
> +	}
> +
> +	*allowed = mask & (allow & ~deny) ? 1 : 0;
> +	if (!(*allowed))
> +		audit = 0xFFFFFFFF;
> +	*audited = mask & (audit & ~quiet) ? 1 : 0;
> +
> +	return 0;
> +}
> Index: apparmor-2.8.0/libraries/libapparmor/src/apparmor.h
> ===================================================================
> --- apparmor-2.8.0.orig/libraries/libapparmor/src/apparmor.h	2013-03-05 16:45:24.471541316 -0800
> +++ apparmor-2.8.0/libraries/libapparmor/src/apparmor.h	2013-03-05 19:26:18.528327777 -0800
> @@ -87,6 +87,10 @@
>  			    size_t size);
>  extern int aa_has_perm(uint32_t mask, char *query, size_t size);
>  
> +#define AA_QUERY_CMD_PROFILE		"profile\0"
> +#define AA_QUERY_CMD_PROFILE_SIZE	8
> +extern int aa_query_profile(uint32_t mask, char *query, size_t size,
> +			    int *allow, int *audit);
>  
>  #define __macroarg_counter(Y...) __macroarg_count1 ( , ##Y)
>  #define __macroarg_count1(Y...) __macroarg_count2 (Y, 16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,1,0)
> Index: apparmor-2.8.0/libraries/libapparmor/src/libapparmor.map
> ===================================================================
> --- apparmor-2.8.0.orig/libraries/libapparmor/src/libapparmor.map	2013-03-05 16:45:24.471541316 -0800
> +++ apparmor-2.8.0/libraries/libapparmor/src/libapparmor.map	2013-03-05 16:45:24.459535316 -0800
> @@ -39,6 +39,7 @@
>    global:
>          aa_task_has_perm;
>  	aa_has_perm;
> +	aa_query_profile;
>    local:
>  	*;
>  } APPARMOR_1.1;
> Index: apparmor-2.8.0/libraries/libapparmor/swig/SWIG/libapparmor.i
> ===================================================================
> --- apparmor-2.8.0.orig/libraries/libapparmor/swig/SWIG/libapparmor.i	2013-03-05 16:45:24.471541316 -0800
> +++ apparmor-2.8.0/libraries/libapparmor/swig/SWIG/libapparmor.i	2013-03-05 16:45:24.463537315 -0800
> @@ -30,3 +30,5 @@
>  extern int aa_task_has_perm(pid_t task, uint32_t mask, const char *query,
>  			    size_t size);
>  extern int aa_has_perm(uint32_t mask, const char *query, size_t size);
> +extern int aa_query_profile(uint32_t mask, char *query, size_t size,
> +			    int *allow, int *audit);
> Index: apparmor-2.8.0/libraries/libapparmor/src/Makefile.am
> ===================================================================
> --- apparmor-2.8.0.orig/libraries/libapparmor/src/Makefile.am	2013-03-05 16:45:24.411511315 -0800
> +++ apparmor-2.8.0/libraries/libapparmor/src/Makefile.am	2013-03-05 19:55:23.144369240 -0800
> @@ -24,7 +24,7 @@
>  noinst_HEADERS = grammar.h parser.h scanner.h af_protos.h
>  
>  libapparmor_la_SOURCES = grammar.y libaalogparse.c kernel_interface.c scanner.c
> -libapparmor_la_LDFLAGS = -version-info 1:2:0 -XCClinker -dynamic \
> +libapparmor_la_LDFLAGS = -version-info 1:2:0 -XCClinker -dynamic -pthread \
>  	-Wl,--version-script=$(top_srcdir)/src/libapparmor.map -Wl,-soname=libapparmor.so.1
>  
>  libimmunix_la_SOURCES = kernel_interface.c libimmunix_warning.c
>