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

[John Johansen]

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?

> +    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 */



> +	if (aa_hat != NULL && strcmp(aa_mode, "enforce") == 0) {
> +	    aa_hat += 2;  /* skip "//" */
> +    	    ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r,
> +			    "AA checks: apache is in hat '%s', mode '%s'", aa_hat, aa_mode);
> +	    if (dcfg != NULL && dcfg->hat_name != NULL) {
> +		if (strcmp(aa_hat, dcfg->hat_name) != 0)
> +        	    ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
> +			"AAHatName '%s' applies, but does not appear to be a hat in the apache apparmor policy",
> +			dcfg->hat_name);
> +	    } else if (scfg != NULL && scfg->hat_name != NULL) {
> +		if (strcmp(aa_hat, scfg->hat_name) != 0 &&
> +		    strcmp(aa_hat, r->uri) != 0)
> +        	    ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
> +			"AADefaultHatName '%s' applies, but does not appear to be a hat in the apache apparmor policy",
> +			scfg->hat_name);
> +	    }
> +	}
> +	free(aa_con);
> +    }
> +
>      return OK;
>  }
>  
> 
> 
> -- AppArmor mailing list AppArmor at lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
>