gnn > Leo, it seems you are the only one with understanding my answer so far. should have been 'the only one with having troubles understanding my answer so far.'
LieGrue, strub ----- Original Message ----- > From: Mark Struberg <[email protected]> > To: MyFaces Development <[email protected]> > Cc: > Sent: Thursday, November 15, 2012 6:15 PM > Subject: Re: [VOTE] Design of MyFaces server side state saving algorithm and > related considerations > > 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 >> >
