On Jun 14, 2013, at 2:16 PM, Stefan Fritsch wrote: > On Thursday 13 June 2013, Roy T. Fielding wrote: >> On Jun 12, 2013, at 12:34 PM, s...@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. > > I agree. That's what I wanted to express in the commit message, sorry > if that did not come out correctly. > > >> 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. > > It's the process pool, and that won't get cleaned up before server > shutdown. And the secret will be initialized in post_config hook at > the latest, so there is no chance that it will be called from > .htaccess. But moving the whole thing to pre_config would be clearer > and better. I will do that when I have some cycles.
The original code did not need access to a pool -- it was a static global array. As near as I can tell, the reason you changed that was just to check to see if it has already been initialized, which can be done easily by making the first byte a non-random, non-zero constant. In spite of the name, this value has almost no need to be secret, so reducing the number of random bytes by one won't matter. >> 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. > > Ack. Though in absence of a configured value, the random secret is > fine. I think it will cause horrible performance issues for any large installation of Digest, such as WebDAV services load-balanced across multiple machines, so I think the fix is worse than the bug. However, I am not confidant that I know enough about this code to object further. The original code seems much more convoluted than it should be, which means either the design is inherently wrong or I simply don't know why it needs to be so complicated. ....Roy
