>>> On 3/9/2006 at 4:49:24 am, in message <[EMAIL PROTECTED]>,
Max Bowsher
<[EMAIL PROTECTED]> 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
