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

[John Johansen]

On 01/23/2014 02:33 PM, Steve Beattie wrote:
> 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.
> 
yep, its actually a problem I tried to come up with a solution for
but didn't like the option presented with the proc interface. Basically
the only thing we could do is return an error code, and map a specific
range of error codes to being success, and subtracting the base of the
range to find the hat number.

But to be change_hat compatible we need to return the size consumed, at
least if its the first hat that matched. Its doable but ugly and I'd
rather a different solution.