Quoting [EMAIL PROTECTED]:
> marcsaeg 01/09/04 19:42:14
>
> Modified: src/share/org/apache/tomcat/startup Tag: tomcat_32
> Tomcat.java
> src/share/org/apache/tomcat/util Tag: tomcat_32
> SessionIdGenerator.java
[snip]
> - // Set the seed PRNG's seed value
> - long seed = System.currentTimeMillis();
> - char entropy[] = getEntropy().toCharArray();
> - for (int i = 0; i < entropy.length; i++) {
> - long update = ((byte) entropy[i]) <<
((i % 8) * 8);
> - seed ^= update;
> - }
> - randomSource.setSeed(seed);
> - }
> + String entropyValue = getEntropy();
> + if(entropyValue != null){
> + /*
> + * We only do the manual seed generation if there is
> a user
> + * supplied entropy value. This is only for
> backwards
> + * compatibility. It is expected that very few
> people
> + * ever took advantage of this feature and
> defaulting
> + * to the normal PRNG seed generator is more secure
> than this
> + * calculation.
> + */
> + long seed = System.currentTimeMillis();
> + char entropy[] = entropyValue.toCharArray();
> + for (int i = 0; i < entropy.length; i++) {
> + long update = ((byte) entropy[i]) << ((i % 8) *
> 8);
> + seed ^= update;
> + }
> + randomSource.setSeed(seed);
> + }else{
> + randomSource.nextInt();
> + }
> + }
> + }
First off, let me say that the above patch is a _really_ good idea, as far as
letting the PRNG default to its own entropy-collection routine, so that
definitely need to be done IMO.
Now I know that what I am about to suggest falls under the category of a trying-
to-save-users-from-themselves enhancement, and not an actual bug fix as would
be appropriate for the 3.2 branch, but it never hurts to offer, right? =)
The problem (which has nothing to do with the above patch other than having
brought the existing algorithm to my attention :-) is that the for{} loop here
serves no real purpose. If the user passes an insecure seed value, this for{}
loop provides absolutely no additional security value. Now it doesn't really
degrade security either, which is why it's not really a "bug" per se. My only
concern is this: It's probably not patently obvious to someone without prior
cryptography experience that this additional pass is merely smoke-and-mirrors
with no practical benefit. I also know that users are _always_ trying to find
shortcuts around the delay in Java's PRNG initalization (in fact, that alone
accounts for at least 50% of the security flaws in Java crypto based on my
experience). My concern is that people will start investigating startup delays,
track it down to this, see that the internal Tomcat code is doing some kind of
mumbo-jumbo with a user-provided seed value, try it (to find that they shaved
~10 seconds off their startup), and assume that whatever Tocmat is doing is
probably "sufficient for my application". IMHO, it's much safer to just remove
this for{} loop voodoo in order to avoid the implication that it does anything
meaningful.
Anyway, that's my take. Usually, bad crypto is worse than no crypto at all. In
this case, it will probably result in a few less people trying to outsmart the
PRNG.
- Christopher
/**
* Pleurez, pleurez, mes yeux, et fondez vous en eau!
* La moitié de ma vie a mis l'autre au tombeau.
* ---Cornelle
*/
