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
signature.asc
Description: OpenPGP digital signature
