myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Struberg <strub...@yahoo.de>
Subject Re: [VOTE] Design of MyFaces server side state saving algorithm and related considerations
Date Thu, 15 Nov 2012 17:18:27 GMT
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 <struberg@yahoo.de>
> To: MyFaces Development <dev@myfaces.apache.org>
> 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 <lu4242@gmail.com>
>>  To: MyFaces Development <dev@myfaces.apache.org>
>>  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
>> 
> 

Mime
View raw message