Re: cvs commit: jakarta-tomcat/src/share/org/apache/tomcat/utilSessionIdGenerator.java
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]:
marcsaeg01/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
RE: cvs commit: jakarta-tomcat/src/share/org/apache/tomcat/utilSessionIdGenerator.java
Actually, no. My product embeds TC3.2.x. When TC4.0 is ready I'll probably
switch to that, but it will take time and I'll still have to support an
existing customer base with the TC3.2.x version installed. That's just a
fact of life and business.
For this reason it is important to me that bugs in 3.2.x get fixed and that
this version remains stable and robust. That's why I became a committer. I
would really like to join in the development of 4.0 but I simply don't have
enough time right now. I wish I did because I think it would be a blast.
Maybe in a few months...
I have *NO* intention, desire, or time to enhance 3.2.x beyond fixing bugs.
Stop. Read the previous sentence again. Repeat until it sinks in.
I could have used the anon-CVS server and made all my bug fixes privately.
I chose not to.
I could have posted bug fixes to tomcat-dev hoping that one of the
committers would take time away from 4.0 development to commit them for me.
I chose not to.
I decided to take the more active approach to help keep the 3.2.x version in
good shape both for myself and for all the others who, for very valid
business reasons, still need to use it. I would think that committers
currently working on 4.0 would appreciate this as it reduces the time they
need to spending supporting the currently released version.
I have no intention of joining the 3.x vs. 4.x flame war.
Finally, the problem this commit addressed, the long PRNG initialization, is
a problem that I've been meaning to fix for a while. I just happened to
like Craig's fix better than the one I proposed several weeks ago so I went
ahead and committed it now.
-Original Message-
From: Jon Stevens [mailto:[EMAIL PROTECTED]]
Sent: Friday, December 22, 2000 12:10 PM
To: tomcat-dev
Subject: FW: cvs commit:
jakarta-tomcat/src/share/org/apache/tomcat/utilSessionIdGenerator.java
Just think. Instead of having to watch the commit messages and backport
everything that keeps getting fixed in 4.0, you could have been working on
fixing bugs in 4.0 or developing features for 4.0.
Geee..what a concept!
-jon
--
From: [EMAIL PROTECTED]
Reply-To: [EMAIL PROTECTED]
Date: 22 Dec 2000 17:35:06 -
To: [EMAIL PROTECTED]
Subject: cvs commit: jakarta-tomcat/src/share/org/apache/tomcat/util
SessionIdGenerator.java
marcsaeg00/12/22 09:35:06
Modified:src/share/org/apache/tomcat/util Tag: tomcat_32
SessionIdGenerator.java
Log:
The PRNG is now initialized with a seed value to prevent the multi-second
delay seen on many platforms.
This code is based on Craig's changes to Catalina that address the same
issue there.
Revision ChangesPath
No revision
No revision
1.3.2.3 +51 -7
jakarta-tomcat/src/share/org/apache/tomcat/util/SessionIdGenerator.java
Index: SessionIdGenerator.java
===
RCS file:
/home/cvs/jakarta-tomcat/src/share/org/apache/tomcat/util/SessionIdGenerator
.java,v
retrieving revision 1.3.2.2
retrieving revision 1.3.2.3
diff -u -r1.3.2.2 -r1.3.2.3
--- SessionIdGenerator.java 2000/11/18 01:33:59 1.3.2.2
+++ SessionIdGenerator.java 2000/12/22 17:35:05 1.3.2.3
@@ -1,7 +1,7 @@
/*
- * $Header:
/home/cvs/jakarta-tomcat/src/share/org/apache/tomcat/util/SessionIdGenerator
.java,v 1.3.2.2 2000/11/18 01:33:59 craigmcc Exp $
- * $Revision: 1.3.2.2 $
- * $Date: 2000/11/18 01:33:59 $
+ * $Header:
/home/cvs/jakarta-tomcat/src/share/org/apache/tomcat/util/SessionIdGenerator
.java,v 1.3.2.3 2000/12/22 17:35:05 marcsaeg Exp $
+ * $Revision: 1.3.2.3 $
+ * $Date: 2000/12/22 17:35:05 $
*
*
*
@@ -114,7 +114,41 @@
*/
public final static long ticDifference = 2000;
-// ** NOTE that this must work together with
get_jserv_session_balance()
+/**
+ * A String initialization parameter used to increase the entropy of
+ * the initialization of our random number generator.
+ */
+private static String entropy = null;
+
+
+/**
+ * Return the entropy increaser value, or compute a semi-useful value
+ * if this String has not yet been set.
+ */
+public static String getEntropy() {
+
+ // Calculate a semi-useful value if this has not been set
+ if (entropy == null)
+ setEntropy((new Object()).toString());
+
+ return (entropy);
+
+}
+
+
+/**
+ * Set the entropy increaser value.
+ *
+ * @param entropy The new entropy increaser value
+ */
+public static void setEntropy(String newEntropy) {
+
+ entropy = newEntropy;
+
+}
+
+
+ // ** NOTE that this must work together with
get_jserv_session_balance()
// ** in jserv_balance.c
static synchronized public String getIdentifier (String jsIdent)
{
@@ -133,10 +167,20 @@
