On 2013-03-05 22:44:35, 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. > > * 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? > > * 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... > > * 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. > Author: Tyler Hicks <[email protected]> > Index: apparmor-2.8.0/libraries/libapparmor/src/kernel_interface.c > ===================================================================
It turns out that this patch had some nasty bugs that I've managed to
chase down. Details below.
> --- 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)
It would be good to make sure that we read all of the data that we
expect to. I'll make a #define with the length that we expect to read
from the kernel and make sure that we get at least that many bytes from
the read().
I'll also use that #define to set the size of buf[].
> + 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",
These should all be SCNx32 since we're reading in hex representations.
SCNu32 results in the wrong values. :/
> + &allow, &deny, &audit, &quiet);
> + if (ret != 4) {
> + errno = EPROTONOSUPPORT;
> + return -1;
> + }
> +
> + *allowed = mask & (allow & ~deny) ? 1 : 0;
This is wrong. mask could contain the same thing as, or a subset of,
(allow & ~deny) and *allowed will properly be set to 1. However, mask
could contain a single matching bit along with any other invalid bits
and *allowed will still be set to 1.
After making those changes, we need to make sure that aa_query_profile()
is not called with a mask value of 0 or *allowed will be set to 1.
> + if (!(*allowed))
> + audit = 0xFFFFFFFF;
> + *audited = mask & (audit & ~quiet) ? 1 : 0;
Same problem here as above.
Patch to follow...
Tyler
> +
> + 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
>
> --
> AppArmor mailing list
> [email protected]
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
