[apparmor] [patch 09/11] mod_apparmor: add logging for AAHatName/AADefaultHatName policy misconfig

[Steve Beattie]

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 <steve at nxnw.org>
> 
> 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 <john.johansen at canonical.com>
> 
> > ---
> >  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
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- 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/20140123/307e55b5/attachment.pgp>