You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Andras Rozsa <an...@yahoo.com> on 2012/04/09 04:04:32 UTC
Tomcat 6 org.apache.catalina.session.ManagerBase issue
Tomcat Developers,
I am a UCCS student and the project I have been working on is related to session ID generation.
I have checked the source code of Tomcat 6 (6.0.24) and I think I have found a mistake.
org.apache.catalina.session.ManagerBase
Line 567: long update = ((byte) entropy[i]) << ((i % 8) * 8);
This solution is not perfect.
The update will be a 32-bit integer this way, so only the 32 LSB of the seed will be modified by entropy through the XOR.
The byte casting should be replaced by a long casting.
like this: long update = ((long) entropy[i]) << ((i % 8) * 8);
I hope you understand my point.
Sincerely,Andras Rozsa
Re: Tomcat 6 org.apache.catalina.session.ManagerBase issue
Posted by Andras Rozsa <an...@yahoo.com>.
Dear Chris,
You missed my point.
I've tested that code.
So, the seed itself is a long (first it is System.currentTimeMillis() - that means the most significant bits don't change because of its nature.)
The left-shifts with 8 bits and the XOR would achieve to "update" all the bits of the seed variable.
With this code its impossible to update the 32 most significant bits since the right side becomes an 32-bit int as you mentioned as well. So the long update will be a 32-bit int!
That means even if there would be a 56-bit left shift (when i is 7 for example), because of the 32-bit int that's a 24 bit left shift (the shift operation is circular) and the XOR will "update" the seed bits from 24 to 31.
The bits from 32 to 63 of the seed are never changed by update! I think that's not the goal there!
There should be a long cast before the shift operation.
Andras
________________________________
From: Christopher Schultz <ch...@christopherschultz.net>
To: Tomcat Developers List <de...@tomcat.apache.org>
Cc: Andras Rozsa <an...@yahoo.com>
Sent: Monday, April 9, 2012 11:18 AM
Subject: Re: Tomcat 6 org.apache.catalina.session.ManagerBase issue
Andras,
On 4/8/12 10:04 PM, Andras Rozsa wrote:
> Tomcat Developers,
>
> I am a UCCS student and the project I have been working on is related
> to session ID generation.
>
> I have checked the source code of Tomcat 6 (6.0.24) and I think I
> have found a mistake.
>
> Line 567: long update = ((byte) entropy[i]) << ((i % 8) * 8);
In trunk (pre-6.0.36), the line of code is o.a.c.session.ManagerBase:583.
> This solution is not perfect.
>
> The update will be a 32-bit integer this way, so only the 32 LSB of
> the seed will be modified by entropy through the XOR. The byte
> casting should be replaced by a long casting.
>
> like this: long update = ((long) entropy[i]) << ((i % 8) * 8);
For the record:
entropy is char[32]
i is int
Here is my analysis of the expression. All references are to JLS 7.
First, we have "(byte) entropy[i]" which is a narrowing conversion (S
5.5) from char to byte, so we have only 8 bits of real data. On the
other side of the <<, we have all integer operations (32-bit), so we
ultimately have
byte << int
This represents a binary numeric promotion (S 5.6.2) which will result
in an integer (32-bit) operation. The result is subject to an assignment
conversion from 32-bit int to 64-bit long (S 5.2).
The local variable "update" is then used as an operand in an XOR
operation with a seed value.
Also,
1. The 'entropy' value is an array of 32 characters, so i varies
from 0 .. 31 (which is actually irrelevant, given #2)
2. 'i' is reduced by the modulus operator to 0..7
3. Thus, the value of entropy[i] is never left-shifted more than 7 bits
4. The 'entropy' array, while a char[] array, only contains
byte-promoted values (see ManagerBase.getEntropy).
Since ((byte)entropy[i]) is a 32-bit value (due to widening prior to the
<< operation) but only has 8-bits of useful information, shifting it up
to 7 bits to the left does not lose any information.
One could argue that the cast from char to byte on line 583 itself
removes some amount of information, since the value is being converted
from 16-bits to 8-bits. However, casting to long would only serve to
turn a 32-bit << operation into a 64-bit one, which doesn't actually help.
If one were to simply remove the (byte) cast, then the full 16-bits of
the entropy character would be shifted and no bit-loss would occur.
Casting to long does not add anything to this expression that removing
the cast to byte would.
Finally, since the entropy data is only significant to 8-bits anyway,
absolutely no information is lost here for any reason.
It may be a good idea to document this method to explain this, as it
does look like a bug on the face of it, until one looks at the actual
data being used in the operation.
-chris
Re: Tomcat 6 org.apache.catalina.session.ManagerBase issue
Posted by Christopher Schultz <ch...@christopherschultz.net>.
All,
On 4/9/12 1:18 PM, Christopher Schultz wrote:
> In trunk (pre-6.0.36), the line of code is o.a.c.session.ManagerBase:583.
Excuse me, I meant to say "6.0.x/trunk", not "trunk". This code doesn't
exist at all in current "trunk".
-chris
Re: Tomcat 6 org.apache.catalina.session.ManagerBase issue
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Chuck,
On 4/9/12 1:23 PM, Caldarale, Charles R wrote:
>> From: Christopher Schultz [mailto:chris@christopherschultz.net]
>> Subject: Re: Tomcat 6 org.apache.catalina.session.ManagerBase issue
>
>> Line 567: long update = ((byte) entropy[i]) << ((i % 8) * 8);
>
>> 2. 'i' is reduced by the modulus operator to 0..7
>
> And then multiplied by 8.
>
>> 3. Thus, the value of entropy[i] is never left-shifted more than 7 bits
>
> No, it's left shifted between 0 and 56 bits (maintaining byte
> alignment). Information is lost.
Rrr. Duh. In fact, the upper 3 bytes of the entropy are lost, which is
quite a bit. This definitely should be cast to long at some point before
the << occurs.
Andros, please log a bug report in Bugzilla:
https://issues.apache.org/bugzilla/
-chris
RE: Tomcat 6 org.apache.catalina.session.ManagerBase issue
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: Christopher Schultz [mailto:chris@christopherschultz.net]
> Subject: Re: Tomcat 6 org.apache.catalina.session.ManagerBase issue
> Line 567: long update = ((byte) entropy[i]) << ((i % 8) * 8);
> 2. 'i' is reduced by the modulus operator to 0..7
And then multiplied by 8.
> 3. Thus, the value of entropy[i] is never left-shifted more than 7 bits
No, it's left shifted between 0 and 56 bits (maintaining byte alignment). Information is lost.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Tomcat 6 org.apache.catalina.session.ManagerBase issue
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Andras,
On 4/8/12 10:04 PM, Andras Rozsa wrote:
> Tomcat Developers,
>
> I am a UCCS student and the project I have been working on is related
> to session ID generation.
>
> I have checked the source code of Tomcat 6 (6.0.24) and I think I
> have found a mistake.
>
> Line 567: long update = ((byte) entropy[i]) << ((i % 8) * 8);
In trunk (pre-6.0.36), the line of code is o.a.c.session.ManagerBase:583.
> This solution is not perfect.
>
> The update will be a 32-bit integer this way, so only the 32 LSB of
> the seed will be modified by entropy through the XOR. The byte
> casting should be replaced by a long casting.
>
> like this: long update = ((long) entropy[i]) << ((i % 8) * 8);
For the record:
entropy is char[32]
i is int
Here is my analysis of the expression. All references are to JLS 7.
First, we have "(byte) entropy[i]" which is a narrowing conversion (S
5.5) from char to byte, so we have only 8 bits of real data. On the
other side of the <<, we have all integer operations (32-bit), so we
ultimately have
byte << int
This represents a binary numeric promotion (S 5.6.2) which will result
in an integer (32-bit) operation. The result is subject to an assignment
conversion from 32-bit int to 64-bit long (S 5.2).
The local variable "update" is then used as an operand in an XOR
operation with a seed value.
Also,
1. The 'entropy' value is an array of 32 characters, so i varies
from 0 .. 31 (which is actually irrelevant, given #2)
2. 'i' is reduced by the modulus operator to 0..7
3. Thus, the value of entropy[i] is never left-shifted more than 7 bits
4. The 'entropy' array, while a char[] array, only contains
byte-promoted values (see ManagerBase.getEntropy).
Since ((byte)entropy[i]) is a 32-bit value (due to widening prior to the
<< operation) but only has 8-bits of useful information, shifting it up
to 7 bits to the left does not lose any information.
One could argue that the cast from char to byte on line 583 itself
removes some amount of information, since the value is being converted
from 16-bits to 8-bits. However, casting to long would only serve to
turn a 32-bit << operation into a 64-bit one, which doesn't actually help.
If one were to simply remove the (byte) cast, then the full 16-bits of
the entropy character would be shifted and no bit-loss would occur.
Casting to long does not add anything to this expression that removing
the cast to byte would.
Finally, since the entropy data is only significant to 8-bits anyway,
absolutely no information is lost here for any reason.
It may be a good idea to document this method to explain this, as it
does look like a bug on the face of it, until one looks at the actual
data being used in the operation.
-chris