Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
I'm suggesting changing the static string WHAT_THE_HECK_GOES_HERE? in ap_auth_nonce() to ap_get_server_name()... comments?
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
Jim Jagielski wrote: I'm suggesting changing the static string WHAT_THE_HECK_GOES_HERE? in ap_auth_nonce() to ap_get_server_name()... comments? see my prior comment on that section of code ;) Dirk's later patch got rid of extra %s in the format string, so zap the last %s as well as my lame WHAT_THE_HECK_GOES_HERE?. Anybody want to think about what happens if we're so unlucky that the ap_user_name or ap_pid_fname string with '\0' is smaller than sizeof(unsigned long) and just happens to be allocated at the end of a page? Unlikely, but still... Maybe those are supposed to be ap_user_name, ap_listeners, etc.?
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
Jeff Trawick wrote: see my prior comment on that section of code ;) Dirk's later patch got rid of extra %s in the format string, so zap the last %s as well as my lame WHAT_THE_HECK_GOES_HERE?. There was som discussion on making ServerName a semi-realm-based aspect of the nonce... Anybody want to think about what happens if we're so unlucky that the ap_user_name or ap_pid_fname string with '\0' is smaller than sizeof(unsigned long) and just happens to be allocated at the end of a page? Unlikely, but still... Maybe those are supposed to be ap_user_name, ap_listeners, etc.? In which case we could use our native '%pp' format (which we should be doing anyway). From my read of it, I think that Dirk's intent was to use the *addresses* of those parameters, so yeah, I think that's not quite right. -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
On Apr 16, 2004, at 9:39 AM, Jim Jagielski wrote: Jeff Trawick wrote: Anybody want to think about what happens if we're so unlucky that the ap_user_name or ap_pid_fname string with '\0' is smaller than sizeof(unsigned long) and just happens to be allocated at the end of a page? Unlikely, but still... Maybe those are supposed to be ap_user_name, ap_listeners, etc.? In which case we could use our native '%pp' format (which we should be doing anyway). From my read of it, I think that Dirk's intent was to use the *addresses* of those parameters, so yeah, I think that's not quite right. Maybe something like this: Index: src/main/http_core.c === RCS file: /home/cvs/apache-1.3/src/main/http_core.c,v retrieving revision 1.333 diff -u -r1.333 http_core.c --- src/main/http_core.c15 Apr 2004 15:51:51 - 1.333 +++ src/main/http_core.c16 Apr 2004 13:46:03 - @@ -563,13 +563,12 @@ * But then again - you should use AuthDigestRealmSeed in your config * file if you care. So the adhoc value should do. */ -return ap_psprintf(r-pool,%lu%lu%lu%lu%lu%s, - *(unsigned long *)((r-connection-local_addr).sin_addr ), - *(unsigned long *)ap_user_name, - *(unsigned long *)ap_listeners, - *(unsigned long *)ap_server_argv0, - *(unsigned long *)ap_pid_fname, - WHAT_THE_HECK_GOES_HERE?); +return ap_psprintf(r-pool,%pp%pp%pp%pp%pp, + (void *)((r-connection-local_addr).sin_addr ), + (void *)ap_user_name, + (void *)ap_listeners, + (void *)ap_server_argv0, + (void *)ap_pid_fname); } API_EXPORT(const char *) ap_default_type(request_rec *r)
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
On Apr 14, 2004, at 12:12 AM, Ben Laurie wrote: Surely this advice is not good - this value (according to my reading) is the only secret that prevents forgery of nonces. OTOH, its late, and I may not be thinking clearly about this - in fact, I'm suspecting that forgery of nonces is not an issue - the issue is using the same nonce in different realms - but I'll send this anyway just so it gets discussed. Also, this isn't really a nonce - the constructed value is - this is a nonce seed, or something along those lines. Correct - it is a nonce-seed. AuthDigestNonce -- AuthDigestSeed or AuthDigestNonceSeed ? It should be identical across an XS realm - but different from realm to realm. If one realm is used on multiple servers (e.g. non sticky loadbalancing) it should be identical across those servers. As a -lot- of different site's use common realm names (such as 'DAV' or 'webfolder') so it should not be set to the same as the realm. Hence the IP address advice for single servers. (This is the problem I found in the wild - recycle a captured wire digest from a common realm name such as 'webfolder', 'dav', 'ical' and use it on a totally different server to which the user uses the same convenience username and password). Dw
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
As an aside, I am unable to successfully apply either patch to the current apache-1.3 tree (not fuzz related, just bad patches, eg: patching file src/modules/standard/mod_digest.c Hunk #2 FAILED at 329. 1 out of 2 hunks FAILED -- saving rejects to file src/modules/standard/mod_digest.c.rej or File to patch: src/main/http_core.c patching file src/main/http_core.c Hunk #1 succeeded at 202 (offset -34 lines). patch: malformed patch at line 132: API_EXPORT(const char *) ap_default_type(request_rec *r) This is under OS X 10.3.3... -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
Joshua Slive wrote: I do have one question about this: Is anyone actually using mod_digest? I was under the impression that there doesn't exist any client that can interoperate with this module (as opposed to mod_auth_digest, which supports modern clients). If this is true, why don't we just delete the darn thing? AFAIK, it's the IE variety that fails to work with mod_digest, but most others do. I have no idea on how widely used the module itself is, but our options are: 1. Remove it 2. Demote it to experimental with a note that it suffers from this bug 3. Fix it. 4. Ignore it. #4 is not under consideration at present. #3 seems like the right thing to do, but only if it doesn't result in 1.3.3x being delayed for weeks. #2 is reasonable, if only we didn't have to worry about the CVS aspects of the move (yeah svn!) -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
On Apr 14, 2004, at 1:57 PM, Ben Laurie wrote: Correct - it is a nonce-seed. AuthDigestNonce -- AuthDigestSeed or AuthDigestNonceSeed ? It should be identical across an XS realm - but different from realm to realm. If one realm is used on multiple servers (e.g. non sticky loadbalancing) it should be identical across those servers. As a -lot- of different site's use common realm names (such as 'DAV' or 'webfolder') so it should not be set to the same as the realm. Hence the IP address advice for single servers. (This is the problem I found in the wild - recycle a captured wire digest from a common realm name such as 'webfolder', 'dav', 'ical' and use it on a totally different server to which the user uses the same convenience username and password). Right. We should be more explicit about the threat model. To that end, how about something like AuthDigestRealmSeed as the name? I think that makes it clearer, yes. +1
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
I'd like to propose that I simply commit the revised patch to CVS for us to poke around with/test/review, etc... My guess is that we'll ship with something similar and this will provide, at least, a nice framework.
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
++1 - if we can correct that directive's name on the way in. Bill At 04:09 PM 4/14/2004, you wrote: I'd like to propose that I simply commit the revised patch to CVS for us to poke around with/test/review, etc... My guess is that we'll ship with something similar and this will provide, at least, a nice framework.
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
Jim Jagielski wrote: On Apr 13, 2004, at 11:13 AM, Jim Jagielski wrote: There is a known bug/issue in the current implementation of mod_digest regarding the nonce. I am looking to have this plugged for our next 1.3 release. There are 2 suggested patches, which I will post under separate Emails. I will also adjust STATUS to reflect these 2 potential patches. PLEASE look these over! I would still like to get a 1.3 release out soon. My expectation is that we will toss 1.3.30... Candidate patch #1: This was my patch to an earlier patch to address some build issues and point out a run-time problem with a sprintf call I guess I need to go through patch 2 and verify that everything was addressed, and/or point out the missing pieces (after I get my tax returns finished and in the mail).
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
Jeff Trawick wrote: Candidate patch #1: This was my patch to an earlier patch to address some build issues and point out a run-time problem with a sprintf call I guess I need to go through patch 2 and verify that everything was addressed, and/or point out the missing pieces (after I get my tax returns finished and in the mail). It looks like the other suggested patch incorporates some of your comments, but not all. -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
Jim Jagielski wrote:
On Apr 13, 2004, at 11:13 AM, Jim Jagielski wrote:
static const char *set_bs2000_account(cmd_parms *cmd, void *dummy, char
*name)
{
@@ -3395,6 +3446,9 @@
An HTTP authorization type (e.g., \Basic\) },
{ AuthName, set_authname, NULL, OR_AUTHCFG, TAKE1,
The authentication realm (e.g. \Members Only\) },
+{ AuthNonce, set_authnonce, NULL, OR_AUTHCFG, TAKE1,
+ An authentication token which should be different for each logical
realm. \
+ A random value or the servers IP may be a good choise.\n },
Surely this advice is not good - this value (according to my reading) is
the only secret that prevents forgery of nonces. OTOH, its late, and I
may not be thinking clearly about this - in fact, I'm suspecting that
forgery of nonces is not an issue - the issue is using the same nonce in
different realms - but I'll send this anyway just so it gets discussed.
Also, this isn't really a nonce - the constructed value is - this is a
nonce seed, or something along those lines.
Cheers,
Ben.
--
http://www.apache-ssl.org/ben.html http://www.thebunker.net/
There is no limit to what a man can do or how far he can go if he
doesn't mind who gets the credit. - Robert Woodruff
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
I do have one question about this: Is anyone actually using mod_digest? I was under the impression that there doesn't exist any client that can interoperate with this module (as opposed to mod_auth_digest, which supports modern clients). If this is true, why don't we just delete the darn thing? Joshua.
Re: [PATCH] Candidate 1: Re: 1.3.3x digest/nonce issue
Joshua Slive wrote: I do have one question about this: Is anyone actually using mod_digest? I was under the impression that there doesn't exist any client that can interoperate with this module (as opposed to mod_auth_digest, which supports modern clients). If this is true, why don't we just delete the darn thing? 1.3's mod_digest works with opera, mozilla, konquer, and a few others last time I checked. it does not work with msie, though, which I guess is the big reason it wasn't widely deployed. --Geoff
