Leo, it seems you are the only one with understanding my answer so far.
You are just searching for lame excuses that you can revert the code. As you did very often already with other committers. And you already pissed off quite a few people that way. So just stop it and start working as community member. You have some really great ideas sometimes, but you are not always right - noone of us is. Back to discussing facts: > What happens here is the key is encrypted and > has a mac code that protects against tampering, Please don't mix the viewkey creation and the encryption which gets added at a later state. What you describes happens in the later stage and has _nothing_ to do with the current discussion. The same viewKey will _always_ give the same encrypted bytes. So if we have a collision in the viewKey we will blow up. Adding the viewId does btw help _nothing_ if the private secret used in the encryption gets leaked. Also your example with executing the same view multiple times is just wrong as you will use the same viewId obviously. > As I already said before, does the hashCode of the viewId helps? Yes. Please show the situation where this could happen. > Is it strictly necessary? No. drop the strictly. This is not question about 'how do you feel today' but a _purely_ binary "yes or no" question. If there is a _single_ situation where we need it, well THEN WE NEED THE VIEWID. If not -> let's drop it. > 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. You already did proof this argument wrong with the link you posted _yourself_! > In development time, it is quite useful to store the full viewId, > because you can inspect the key It's really easy to get this info other ways. And the downside is that we use different code in production which typically doesn't get used by programmers. That will f***k up every security review and testing/debugging effort. While developing you typically know _exactly_ which page you access with your web browser! > So, a developer cannot create any key, but it can extend > SerializedViewKey Bollocks again as this was a private inner class and all the other needed stuff was private as well. LieGrue, strub ----- Original Message ----- > From: Leonardo Uribe <[email protected]> > To: MyFaces Development <[email protected]> > Cc: > Sent: Thursday, November 15, 2012 5:49 PM > Subject: Re: [VOTE] Design of MyFaces server side state saving algorithm and > related considerations > > 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 >
