[apparmor] [Patch] mod_apparmor: try uri hat after AADefaultHatName, not before

[Seth Arnold]

On Thu, Jun 12, 2014 at 02:23:46PM -0700, Steve Beattie wrote:
> Bug: https://bugs.launchpad.net/bugs/1322778
> 
> In trunk revno 2335, a bug was fixed in mod_apparmor that corrected
> the storage location for AADefaultHatName.  The incorrect storage
> caused the hat specified by the AADefaultHatName keyword to be the
> default value for AAHatName, and meant that if both an AAHatName and
> an AADefaultHatName entry were given in a vhost, mod_apparmor would
> not fall back to trying AADefaultHatName if the hat specified in
> AAHatName did not exist in the apache apparmor profile.
> 
> However, because the value specified in AADefaultHatName was the
> default, if no AAHatName was specified, it would be attempted first,
> before a hat based on the passed URI, rather than after as the
> documentation stated and the code intended. By fixing the storage bug,
> the attempted hat ordering now matched the documentation. But a number
> of users came to rely on AADefaultHatName being attempted before
> the URI. For trunk, this issue is less severe because mod_apparmor
> passes a vector of hats to aa_change_hatv(), and thus missing URI
> hats are not logged by the kernel apparmor bits. It still represents
> a behavioral change to users, though.
> 
> This patch re-adjusts the ordering so that the URI-based hat is
> attempted after the hat specified by AADefaultHatName is attempted,
> thus maintaining the actual behavior before the bug addressed in
> revno 2335 was fixed.
> 
> (Apologies that the manpage changes are represented as larger than the
> actual changes made; I reflowed the paragraphs, which caused gratuitous
> changes.)
> 

I don't like this much; if I've understood it correctly, this patch makes
it impossible to have most URIs in an application use one hat and
special-case a few URIs with another hat. AADefaultHatName and AAHatName
are more or less identical, right?

That said, the implementation looks good.

Should we use this in a distro patch somewhere to fix a regression?

Thanks

> Signed-off-by: Steve Beattie <steve at nxnw.org>
> ---
>  changehat/mod_apparmor/mod_apparmor.c   |    8 ++++----
>  changehat/mod_apparmor/mod_apparmor.pod |   21 ++++++++++-----------
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> Index: b/changehat/mod_apparmor/mod_apparmor.c
> ===================================================================
> --- a/changehat/mod_apparmor/mod_apparmor.c
> +++ b/changehat/mod_apparmor/mod_apparmor.c
> @@ -159,10 +159,6 @@ immunix_enter_hat (request_rec *r)
>          aa_hat_array[i++] = dcfg->hat_name;
>      }
>  
> -    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
> -		    "[uri] adding uri '%s' to aa_change_hat vector", r->uri);
> -    aa_hat_array[i++] = r->uri;
> -
>      if (scfg) {
>      	ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "Dumping scfg info: "
>      	          "scfg='0x%lx' scfg->hat_name='%s'",
> @@ -184,6 +180,10 @@ immunix_enter_hat (request_rec *r)
>      }
>  
>      ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
> +		    "[uri] adding uri '%s' to aa_change_hat vector", r->uri);
> +    aa_hat_array[i++] = r->uri;
> +
> +    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
>  		    "[default] adding '%s' to aa_change_hat vector", DEFAULT_URI_HAT);
>      aa_hat_array[i++] = DEFAULT_URI_HAT;
>  
> Index: b/changehat/mod_apparmor/mod_apparmor.pod
> ===================================================================
> --- a/changehat/mod_apparmor/mod_apparmor.pod
> +++ b/changehat/mod_apparmor/mod_apparmor.pod
> @@ -72,14 +72,13 @@ behavior described above.
>  
>  =item B<AADefaultHatName>
>  
> -AADefaultHatName allows you to specify a default hat to be used
> -for virtual hosts and other Apache server directives, so that you
> -can have different defaults for different virtual hosts. This can
> -be overridden by the AAHatName directive and is checked for only if
> -there isn't a matching AAHatName or hat named by the URI. The default
> -value of AADefaultHatName is the ServerName for the server/vhost
> -configuration. If the AADefaultHatName hat does not exist, it falls
> -back to the DEFAULT_URI hat if it exists (as described above).
> +AADefaultHatName allows you to specify a default hat to be used for
> +virtual hosts and other Apache server directives, so that you can have
> +different defaults for different virtual hosts. This can be overridden
> +by the AAHatName directive and is checked for only if there isn't
> +a matching AAHatName. The default value of AADefaultHatName is the
> +ServerName for the server/vhost configuration.  If the AADefaultHatName
> +hat does not exist, then it falls back to the behavior described above.
>  
>  =back
>  
> @@ -99,12 +98,12 @@ will:
>  1. try to aa_change_hat(2) into a matching AAHatName hat if it exists and
>  applies, otherwise it will
>  
> -2. try to aa_change_hat(2) into the URI itself, otherwise it will
> -
> -3. try to aa_change_hat(2) into an AADefaultHatName hat, either the
> +2. try to aa_change_hat(2) into an AADefaultHatName hat, either the
>  ServerName (the default) or the configuration value specified by the
>  AADefaultHatName directive, for the server/vhost, otherwise it will
>  
> +3. try to aa_change_hat(2) into the URI itself, otherwise it will
> +
>  4. try to aa_change_hat(2) into the DEFAULT_URI hat, if it exists, otherwise it
>  will
>  
> -- 
> Steve Beattie
> <sbeattie at ubuntu.com>
> http://NxNW.org/~steve/



> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140612/cef4b035/attachment.pgp>