httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Roy T. Fielding <field...@gbiv.com>
Subject Re: svn commit: r1492395 - in /httpd/httpd/trunk: CHANGES modules/aaa/mod_auth_digest.c
Date Thu, 13 Jun 2013 07:35:26 GMT
On Jun 12, 2013, at 12:34 PM, sf@apache.org wrote:

> Author: sf
> Date: Wed Jun 12 19:34:19 2013
> New Revision: 1492395
> 
> URL: http://svn.apache.org/r1492395
> Log:
> Actually use the secret when generating nonces.
> 
> This change may cause problems if used with round robin load balancers.
> Before it is backported, we should add a directive to use a user specified
> secret.
> 
> PR: 54637

FWIW, I don't think this code can be released as is.

Yes, the prior code is broken, in that it creates the
nonce using a supposedly random value before that value is
actually initialized, and thus ends up creating a nonce that
is effectively just a timestamp.

However, fixing that is an error in itself, since the nonce
should not be random, which is why the existing code actually
works the way the reporter wanted.

The change from static global array to a static global pointer
into an allocated module config pool (?) obscures the "bug fix"
change of

> @@ -453,6 +473,21 @@ static void *create_digest_dir_config(ap
> static const char *set_realm(cmd_parms *cmd, void *config, const char *realm)
> {
>     digest_config_rec *conf = (digest_config_rec *) config;
> +    int i;
> +
> +    /* pass NULL because cmd->server may not have a valid log config yet */
> +    if (secret == NULL
> +        && initialize_secret(cmd->server->process->pool, NULL) != APR_SUCCESS)
> +        return "Could not get random numbers for secret";
> +
> +#ifdef AP_DEBUG
> +    /* check that we got random numbers */
> +    for (i = 0; i < SECRET_LEN; i++) {
> +        if (secret[i] != 0)
> +            break;
> +    }
> +    ap_assert(i < SECRET_LEN);
> +#endif

Using a global pointer to an allocated pool variable is
not even remotely safe when that pool gets deallocated.
And a routine that gets called within .htaccess files is not an
appropriate place to set a server-wide value.

The nonce is supposed to be window-valid and verifiable, not
random.  Hence, this part of the nonce should be a configurable
salt shared by all servers for a given hostname.  Likewise,
the request host:port needs to be in the nonce hash, but now
I see that the prior code for that was commented-out by Ronald.
The window seems to be handled elsewhere using a timestamp.

If we assume that something like the above is planned
as an actual fix, then this patch is only complicating matters.
So, I suggest that the new code is not an improvement.

I think a better solution would be to simply remove the secret
hash entirely and figure out why this code in gen_nonce_hash()

1074	ronald	85341	 /*
1075	dougm	88019	 apr_sha1_update_binary(&ctx, (const unsigned char *) server->server_hostname,
1076	aaron	92626	 strlen(server->server_hostname));
1077	dougm	88019	 apr_sha1_update_binary(&ctx, (const unsigned char *) &server->port,
1078	aaron	92626	 sizeof(server->port));
1079	ronald	85341	 */

was commented out by Ronald in

http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/aaa/mod_auth_digest.c?r1=85340&r2=85341&pathrev=1492395&view=patch

because that is where the real fix should be.  The current secret
should be replaced here by a configurable string that is set in
the virtual host config.

However, I am no expert with this code, and have no convenient
means for testing whatever I'd screw up by making changes here.
I am hoping that others on the list can look at this discussion
and either confirm my worries or explain why the random secret
is needed.

Cheers,

....Roy


Mime
View raw message