Hi MS>> And now for your 'turning around my words'
MS>> > 1. Go back to the old way, just one class. MS>> NO definitely not, see my answer 3 MS>> > 2. The viewId is not important --> get rid of it. MS>> NO, FIRST check if it is important, then either get rid or fix it. MS>> > 3. Split ServerSideStateCacheImpl into many classes MS>> YES, and clean it up, document it, etc. Mark, Your response is too ambiguous. Definitively, I don't know what do you want to do. I can't see an action course from your observations. Don't answer with another question. In 1 and 2 we need a clear position. Remember that if there is no agreement, lazy consensus applies and the older code needs to be preserved, so I will be forced to revert your patches. I don't want that, I want to get to a conclusion. MK>>Well, the hashcode doesn't make it perfectly safe, but it certainly MK>>makes it more safe. The real question is what are we safe-guarding MK>>against? A bug in our code? Or is there some other way that the MK>>counter could be corrupted? Is this a check to insure that the MK>>response didn't get corrupted? I've certainly experienced response MK>>corruption due to exceptions during page generation as well as proxy MK>>servers that truncate total form submission length, and checks against MK>>this are valuable to me. In theory, the hashCode of the viewId guards against apply the state stored into session to a different view. What happens here is the key is encrypted and has a mac code that protects against tampering, so use a counter value + encryption + tampering is safe, but the problem is for the same counter number the same key will be generated, for all views. So, just executing a view multiple times, since the counter produce ordered values (1,2,...) I can calculate the next key given a current one. That's potentially unsafe. The hashCode or the viewId just add some bytes to the key, so according to the viewId, different keys will be generated, because the combination of the hash and the counter will be different. Since the key is encrypted and has a mac code, even if you know the message to write, you need an infeasible amounts of computation to calculate another mac code, encrypt and send it to the server. MK>>If we are checking against possible corruption, then this check MK>>doesn't have to be perfect, just like an md5 checksum on a downloaded MK>>file doesn't perfectly guarantee that you are getting back the same MK>>file However, hashcode for classes not under our control don't MK>>guarantee useful checksums across various JVMs. The hashcode could be MK>>zero for all values. But in reality, is this likely to be true? I MK>>would imagine that any String hashcode would be valid as a checksum. MK>> Mark, you said there were issues with IBM's JVM hashcodes. Can you MK>>summarize what those would be in this specific situation? MK>>For my limited understanding of this topic, I'd say that we either MK>>drop all but the counter, or, if this is some kind of corrupted data MK>>check, that we only use a checksum value rather than the full viewID. As I already said before, does the hashCode of the viewId helps? Yes. Is it strictly necessary? No. Is safe to use the viewId hashCode? Yes. Why? because the probability of calculate that two different REAL viewIds has the same hashCode is astronomically low. A checksum over the viewId can be a replacement of the hashCode. What we need here is something so we can "salt" the generated key, so different views will generate different keys in different order. The other alternative is generate a random sequence. The algorithm already allows that, but it is not enabled by default. Both strategies (counter+viewId hashCode or random sequence) are effective ways to generate a key. LU>> The idea is serialize an Object is more expensive than serialize a LU>> primitive type. LU>> So, using IntIntSerializedViewKey and IntByteArraySerializedViewKey in LU>> production stage will lead to a smaller session size compared to use the old LU>> implementation of SerializedViewKey, with requires to store the full viewId. MK>>I'm confused about this one. Can you provide more information? Why MK>>do we need more than one way to do it? Why wouldn't we pick the most MK>>efficient method and use it in both production and development? In development time, it is quite useful to store the full viewId, because you can inspect the key and know quickly which state belongs to an specific view, but in production, the hashCode will lead to a very small key, and at the end to a better performance (less markup rendered, faster encryption, less session size). LU>> the current design using private static classes LU>> ensures that it is not possible to create keys outside the algorithm LU>> and override LU>> the values inside the internal map that holds the view state. For example, with LU>> Java 2 security it is possible to ensure that code cannot be executed LU>> using reflection LU>> and so on. Putting the classes outside the class makes harder to control that. MK>> I'm also confused about this one. Can you provide more information? MK>> What security problem are we trying to prevent? The developer from MK>> creatiing their own key algorithm? In this moment the generated keys in ServerSideStateCacheImpl are closed to modifications. So, a developer cannot create any key, but it can extend SerializedViewKey and provide alternate implementations. In other words, the developer can create its own key algorithm but it can't mess with the existing one. Since all related code is in just one file, it is easy to set Java 2 security rules to forbid any malicious code to generate its own keys and override the view state. In this moment by design it is not possible, but if we split ServerSideStateCacheImpl, we are exposing the details of this algorithm, and people will be tempted to override them, even if that's not the intention. Since there are no intentions to expose the details of that class, there is no need to split ServerSideStateCacheImpl. In this moment, the file has 1451 lines, which is far below the 3000 lines limit imposed by checkstyle rules. Mark mentioned that if an atacker has come to that point, there are alternatives to get the same effect. Ok, I accept that. This point is more about "shape" than "content". I feel confortable with that file, and instead it is a big advance, because we have more flexibility for the key generation. But the work is far from over. In 2.1.x-client-window I have some changes to include client window concept in the state saving algorithm. If we split ServerSideStateCacheImpl now, we need to reflect those changes in 2.1.x-client-window and 2.2.x, and that is a complete waste of time. My vote is still against split that file, because there is still work in progress over that detail (I really hate checkstyle rules with passion). regards, Leonardo Uribe
