NOTE: Tomcat 4 uses essentially the same approach (see
org.apache.catalina.session.ManagerBase), so this discussion is relevant
there too. See below for more.
On Tue, 4 Sep 2001, Christopher Cain wrote:
> Date: Tue, 04 Sep 2001 23:31:01 -0600 (MDT)
> From: Christopher Cain <[EMAIL PROTECTED]>
> Reply-To: [EMAIL PROTECTED]
> To: [EMAIL PROTECTED]
> Subject: Re: cvs commit: jakarta-tomcat/src/share/org/apache/tomcat/util
> SessionIdGenerator.java
>
> 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.
If the entropy value is configurable, and is not visible to potential
attackers (yah, I know, it's in server.xml right now, but ...), would that
still be true? The idea of salting the initial seed with entropy was in
fact suggested by people who claimed some cryptographic experiene (should
be visible in the TOMCAT-DEV archives over the last year or so) as a means
of improving the unpredicatability of session id values.
> 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.
>
We're all assuming, of course, that the default PRNG seeding algorithm is
cryptographically secure ....
> - Christopher
>
> /**
> * Pleurez, pleurez, mes yeux, et fondez vous en eau!
> * La moitié de ma vie a mis l'autre au tombeau.
> * ---Cornelle
> */
>
Craig
