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 <[email protected]> > > 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 > > >
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
