httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Brad Nicholes" <BNICHO...@novell.com>
Subject [PATCH] Re: mod_authz_core:check_provider_list bug?
Date Thu, 09 Mar 2006 16:25:42 GMT
>>> On 3/9/2006 at 4:49:24 am, in message <44101644.5000907@ukf.net>,
Max Bowsher
<maxb1@ukf.net> wrote:
> Joe Orton wrote:
>> Found by the Coverity report, this one looks like a real bug:
>> 
>> check_provider_list has a if() branch to handle the passed-in 
>> current_provider being NULL, but never sets it to anything else; 
>> current_provider is later dereferenced unconditionally.
> 
> It looks like the authn-provider implementation was cloned to
produce
> starting point of the authz-provider implementation, and this code
> wasn't removed when it became redundant.
> 
> All callers of check_provider_list() ensure that they pass a
non-NULL
> current_provider.  The AUTHZ_DEFAULT_PROVIDER that is being looked up
in
> this code is never registered.  The no-providers-configured case is
> dealt with in authorize_user(), before check_provider_list() is ever
called.
> 
> I suggest the following patch:
> 
> [[[
> * modules/aaa/mod_authz_core.c (check_provider_list):
>   Remove redundant code.
> * modules/aaa/mod_auth.h (AUTHZ_DEFAULT_PROVIDER):
>   Remove redundant definition.
> ]]]
> 
> [[[
> Index: modules/aaa/mod_authz_core.c
> ===================================================================
> --- modules/aaa/mod_authz_core.c        (revision 384494)
> +++ modules/aaa/mod_authz_core.c        (working copy)
> @@ -482,28 +482,10 @@
> 
>      const authz_provider *provider;
> 
> -    /* For now, if a provider isn't set, we'll be nice and use the
file
> -     * provider.
> -     */
> -    if (!current_provider) {
> -        provider = ap_lookup_provider(AUTHZ_PROVIDER_GROUP,
> -                                      AUTHZ_DEFAULT_PROVIDER, "0");
> +    provider = current_provider->provider;
> +    apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE,
> +                   current_provider->provider_name);
> 
> -        if (!provider || !provider->check_authorization) {
> -            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
> -                          "No default authz provider configured");
> -            auth_result = AUTHZ_GENERAL_ERROR;
> -            return auth_result;
> -        }
> -        apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE,
> -                       AUTHZ_DEFAULT_PROVIDER);
> -    }
> -    else {
> -        provider = current_provider->provider;
> -        apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE,
> -                       current_provider->provider_name);
> -    }
> -
>      /* check to make sure that the request method requires
>       * authorization before calling the provider
>       */
> Index: modules/aaa/mod_auth.h
> ===================================================================
> --- modules/aaa/mod_auth.h      (revision 384494)
> +++ modules/aaa/mod_auth.h      (working copy)
> @@ -38,7 +38,6 @@
>  #define AUTHN_PROVIDER_GROUP "authn"
>  #define AUTHZ_PROVIDER_GROUP "authz"
>  #define AUTHN_DEFAULT_PROVIDER "file"
> -#define AUTHZ_DEFAULT_PROVIDER "default"
> 
>  #define AUTHZ_GROUP_NOTE "authz_group_note"
>  #define AUTHN_PROVIDER_NAME_NOTE "authn_provider_name"
> ]]]
> 
> Max

+1  During the develop, mod_authz_default flip-flopped between being a
provider vs. hook.  It turned out that ultimately authz_default need to
remain a hook so that it could be guaranteed to run last.  So the
provider 'default' doesn't actually exist.  Also given that with the
checks that are in place before check_provider_list() is called prevent
current_provider from being a NULL value, removing the code that
attempts to load a default authz provider seems reasonable.  Also the
concept of a default provider should happen fairly automatically anyway
since the access control directives 'Allow/Deny' have been folded in as
providers.  Setting 'Require All Granted' or 'Require All Denied' for a
directory carries through to sub directories as a default provider. 
Given that our standard httpd.conf specifies 'Require all denied' for
root, the 'all denied' becomes the default provider. 

Brad

Mime
View raw message