[apparmor] [PATCH RFC] Add profile-based libapparmor query interface
Tyler Hicks
tyhicks at canonical.com
Fri Mar 8 00:07:41 UTC 2013
On 2013-03-07 00:07:29, John Johansen wrote:
> 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.
That cleans things up a bit.
I'll modify the dbus code to use the new interface and, while I'm doing
that, see if that's the best option.
>
> > * 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.
I'll leave it as-is for now.
>
>
> > * 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.
Yeah, it is a bummer that simple_transaction_get() enforces one write
per open.
Another option would be to do this in a library constructor function but
support for that is compiler specific.
I do like that we don't currently require callers to keep any kind of
state around (such as a file descriptor), so I'm currently ok with
pthread_once() if everyone else is.
>
> > 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
> >
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130307/2d20392f/attachment.pgp>
More information about the AppArmor
mailing list