[Oisf-devel] [COMMIT] OISF branch, master, updated. suricata-1.3beta1-94-ge3764b9

[Pierre Chifflier]

On Fri, Jun 08, 2012 at 06:20:35PM +0200, Victor Julien wrote:
> On 06/08/2012 05:58 PM, Pierre Chifflier wrote:
> >> commit 270ea253a24f1759d9c36d6b34abd6360c5633b0
> >> Author: Anoop Saldanha <poonaatsoc at gmail.com>
> >> Date:   Fri May 18 21:18:30 2012 +0530
> >>
> >>     ssl parser fix/updates
> > 
> > Hi all,
> > 
> > This commit really looks wrong to me. Not only does it change
> > everything, but it also break some features.
> > For example, it removes the ability to extract the ciphersuite,
> > compression method etc. from the handshake.
> 
> Right, I understand your frustration. However the patches are the result
> of a big effort to stabilize this code. We got a lot of reports of
> random memory corruptions.

I can't tell, I have not seen the reports (maybe I can add a CC
somewhere in the BT?). However, the patch is so big
it makes impossible to differentiate structure changes (->curr_connp)
from eventual fixes.

> 
> The code in question was not being used at this time so it was removed
> in this effort. It's a small function that can easily be reintroduced.
> 
> > It deletes the function DecodeTLSHandshakeServerHello which is really
> > important for all of the TLS functions. (see changes on files
> > rc/detect-ssl-version.c and rc/detect-ssl-version.h)
> 
> Not sure how these files are effected?

Some code was removed. The objective, for these files, is to contain a
decoder function for each (interesting) message type, and they
are used on the patches which are not pushed (rules to detect specific
TLS cipher families, lack of PFS, etc.).
The reason for not pushing them yet is that there are some design
choices to be made (for ex, how to store the kind of database for all
the list ciphers, and how to access/refresh it, but that should be
discussed in another thread).

> 
> > I have not reviewed all of the patch, but most changes are really
> > intrusive so I would ask either to revert it, or to split it into
> > smaller parts. It would also be nice to warn people working on other
> > branches before pushing such big changes, as it completely breaks work
> > from Eric and I.
> 
> Yup, agreed about the warnings. Will try to do that earlier next time.
> In this case pressure of wanting to get a release out combined with
> having to debug the issue on people sending in bt's alone made us solve
> it this way.
> 

Right, I was just pointing out the fact that a few messages could have
spared us from some happy conflict-merging in several files :)
I think I will be able to fix it locally.

Cheers,
Pierre