On Thu, Jan 23, 2014 at 03:49:51AM -0800, John Johansen wrote: > On 01/23/2014 02:45 AM, Steve Beattie wrote: > > This patch adds code that checks the resulting hat that apache gets > > placed into, and verifies that if the apache configuration specified > > that an AAHatName or AADefaultHatName should have been the resulting > > hat. If it wasn't, emit a warning message to the apache log, as this > > likely indicates a mismatch between the apache configuration and its > > apparmor policy (i.e. why define AAHatName if you aren't going to > > create the corresponding hat in the apparmor policy?) > > > > (Note for AADefaultHatName, a message is not logged if a defined > > AAHatName would also apply or if there is a hat defined for the uri, > > as each of those come first in the order of attempted hats.) > > > > Signed-off-by: Steve Beattie <[email protected]> > > So a few comments inline, with that said I am not opposed to committing > this as is. So with or with out the changes > > Acked-by: John Johansen <[email protected]> > > > --- > > changehat/mod_apparmor/mod_apparmor.c | 31 > > +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > Index: b/changehat/mod_apparmor/mod_apparmor.c > > =================================================================== > > --- a/changehat/mod_apparmor/mod_apparmor.c > > +++ b/changehat/mod_apparmor/mod_apparmor.c > > @@ -137,6 +137,7 @@ immunix_enter_hat (request_rec *r) > > ap_get_module_config (r->server->module_config, > > &apparmor_module); > > const char *aa_hat_array[5] = { NULL, NULL, NULL, NULL, NULL }; > > int i = 0; > > + char *aa_con, *aa_mode, *aa_hat; > > > > debug_dump_uri(r); > > ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "in immunix_enter_hat > > (%s) n:0x%lx p:0x%lx main:0x%lx", > > @@ -191,6 +192,36 @@ immunix_enter_hat (request_rec *r) > > ap_log_rerror(APLOG_MARK, APLOG_WARNING, errno, r, "aa_change_hatv > > call failed"); > > } > > > > + /* Check to see if a defined AAHatName or AADefaultHatName would > > + * apply, but wasn't the hat we landed up in; report a warning if > > + * that's the case. */ > > + sd_ret = aa_getcon(&aa_con, &aa_mode); > Do we always want to do this or only when a certain LOG or warning level > is set?
Hrm, maybe. It's only useful to do the work when APLOG_WARNING is being
reported, so it could test on that, but that's a log level I'd almost
certainly expect to be enabled by default. We could adjust the log
level lower as well, I guess, but I fear that it's not nearly as useful
to carry the code if it's at a level that isn't reported by default.
It kind of points to a minor deficiency in aa_change_hatv()'s
interface, in that you know you successfully changed to hat or not,
but not which one.
> > + if (sd_ret < 0) {
> > + ap_log_rerror(APLOG_MARK, APLOG_WARNING, errno, r, "aa_getcon call
> > failed");
> > + } else {
> > + ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r,
> > + "AA checks: aa_getcon result is '%s', mode '%s'",
> > aa_con, aa_mode);
> > + aa_hat = strstr(aa_con, "//");
>
> This will break for nested profiles and stacking. That being said I think it
> is fine for all current deployments as we don't allow nesting beyond the
> first subprofile level in policy yet. And stacking will likely only be used
> between namespaces where aa_getcon will not report parent namespace info.
>
> But perhaps you could add a little TODO: comment something like
>
> /* TODO: update to use library find hat_name fn */
Comment added, thanks for raising the issue.
--
Steve Beattie
<[email protected]>
http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
