Re: Patch for Stream5 TCP direction

[John Eure <john.eure () gmail com>]

Ah, I guess I picked the wrong ones.  :-)  I'll look forward to the next
release!

Thanks,
John


On Mon, Feb 24, 2014 at 8:35 AM, Russ Combs (rucombs) <rucombs () cisco com>wrote:

>  Stream5GetReassemblyFlushPolicyTcp() has it backwards and some others
> too.  I'm opening a bug to address this.
>
> Thanks John!
> Russ
>
>  ------------------------------
> *From:* John Eure [john.eure () gmail com]
> *Sent:* Friday, February 21, 2014 7:51 PM
> *To:* Russ Combs (rucombs)
> *Cc:* Carter Waxman (cwaxman); snort-devel
>
> *Subject:* Re: [Snort-devel] Patch for Stream5 TCP direction
>
>   Hm.  The thing that brought it to my attention wasn't in your code, but
> I think the following might help.
>
>  In the file src/preprocessors/Stream5/snort_stream5_tcp.c, in the
> functions Stream5IsPafActiveTcp() and Stream5GetReassemblyFlushPolicyTcp(),
> for the first we have:
>
>         bool Stream5IsPafActiveTcp (Stream5LWSession* lwssn, bool
> to_server)
>         ...
>             fm = to_server ? &*tcpssn->server.flush_mgr* :
> &tcpssn->client.flush_mgr;
>
>             return ( (fm->flush_policy == STREAM_FLPOLICY_PROTOCOL)
>
>  And for the second, we have:
>
>         char Stream5GetReassemblyFlushPolicyTcp(Stream5LWSession *lwssn,
> char dir)
>         ...
>             if (dir & SSN_DIR_FROM_CLIENT)
>             {
>                 return (char)*tcpssn->client.flush_mgr*.flush_policy;
>             }
>
>             if (dir & SSN_DIR_FROM_SERVER)
>             {
>                 return (char)tcpssn->server.flush_mgr.flush_policy;
>             }
>
>  So assuming we're intrested in the direction from-client-to-server, in
> the first function, if we pass in to_server == true, then we get a result
> from tcpssn->server.flush_mgr, but in the second function, if we pass in
> dir == SSN_DIR_FROM_CLIENT, we get a result from tcpssn->client.flush_mgr.
>
>  That didn't seem right to me, and when I checked, I found that a similar
> reason was why Stream5ActivatePafTcp() wasn't working.  And when I tried
> switching the direction inside that function, it started working, too.  I
> had previously assumed that the weird invalid TCP in my pcaps was causing
> Stream5 to enter an unrecoverable state, when PAF in one direction was
> still active but PAF in the other direction was inactive.  But it turned
> out that I had merely been trying to reactivate the direction that was
> still active.  And this also explained why, when both directions went
> inactive at the same time, my recovery code suddenly started working
> again.  :-)
>
>  So I looked at more functions, and how they were called and used.  Going
> through them again, for the direction from-client-to-server, I think we
> have (and I might be misreading something - there are a few places where
> the direction is deliberately reversed, but I didn't see any in these
> functions):
>
> Stream5GetPAFUserDataTcp()
>     tcpssn->server.paf_state.user
> Stream5IsPafActiveTcp()
>     *tcpssn->server.flush_mgr*.flush_policy
> Stream5ActivatePafTcp()
>     tcpssn->server.tcp_policy->flush_point_list
>     tcpssn->server.paf_state
>
> *tcpssn->server.flush_mgr *GetTcpRebuiltPackets()
>     tcpssn->server
> GetTcpStreamSegments()
>     tcpssn->server
> Stream5AddSessionAlertTcp()
>     tcpssn->server
> Stream5CheckSessionAlertTcp()
>     tcpssn->server
> Stream5SetExtraDataTcp()
>     tcpssn->server
> Stream5ClearExtraDataTcp()
>     tcpssn->server
>  Stream5GetReassemblyDirectionTcp()
>     *tcpssn->client.flush_mgr*.flush_policy
> Stream5GetFlushPointTcp()
>     *tcpssn->client.flush_mgr*.flush_pt
> Stream5SetFlushPointTcp()
>     *tcpssn->client.flush_mgr*
> Stream5SetReassemblyTcp()
>    * tcpssn->client.flush_mgr*
>     tcpssn->client.tcp_policy->flush_point_list
>     tcpssn->client.flags
> Stream5GetReassemblyFlushPolicyTcp()
>     *tcpssn->client.flush_mgr*.flush_policy
> Stream5IsStreamSequencedTcp()
>     tcpssn->server.flags
> Stream5MissingInReassembledTcp()
>     tcpssn->server.flags
> Stream5PacketsMissingTcp()
>     tcpssn->server.flags
>
>  What this looks like, assuming all these directions are intended to be
> the same, is that for the direction from-client-to-server, most things are
> stored in tcpssn->server, but the flush manager is located in
> tcpssn->client.flush_mgr.  (Although if that is the case, then it looks
> like Stream5SetReassemblyTcp() might have something odd in it, too...)
>
>  If I'm misunderstanding something, you have my apologies.  For instance,
> none of this may matter if there's a semantic difference in the term
> "direction", when we're talking about regular packets vs. rebuilt
> pseudo-packets vs. PAF, and data vs. acks.  And it's certainly possible
> that I happened to pick the wrong direction, and it's the other 5 functions
> that should be switched.
>
>  Thanks a lot,
>
> John
>
>
>
>
> On Fri, Feb 21, 2014 at 9:38 AM, Russ Combs (rucombs) <rucombs () cisco com>wrote:
>
> >  John,
> >
> >  The flush policy and the queued segments are in the receiving tracker.
> >  The code looks correct to me, but maybe I'm not seeing your point of view.
> >  Can you point out where they are used opposite to these two functions?
> >
> >  Thanks
> > Russ
> >
> >   ------------------------------
> > *From:* John Eure [john.eure () gmail com]
> > *Sent:* Thursday, February 20, 2014 10:10 PM
> > *To:* Carter Waxman (cwaxman)
> > *Cc:* snort-devel
> > *Subject:* Re: [Snort-devel] Patch for Stream5 TCP direction
> >
> >    The issues are:
> >
> > 1) Stream5IsPafActiveTcp() returns incorrect results
> > 2) Stream5ActivatePafTcp() fails when it should work
> >
> >  What I found is that Stream5IsPafActiveTcp() returned the result for
> > the direction opposite to the direction specified in its arguments, and
> > that Stream5ActivatePafTcp() activated PAF in the direction opposite to the
> > direction specified in its arguments.  Most of the time, this is hidden,
> > because Stream5ActivatePafTcp() is called twice in a row, for both
> > directions, on a stream that has PAF inactive in both directions.  But when
> > called on a stream with PAF active in one direction but not in the other,
> > the results are incorrect.
> >
> >  This is all, of course, assuming that "PKT_FROM_CLIENT",
> > "SSN_DIR_FROM_CLIENT", "to_server == true", and "c2s == true" should all
> > refer to the same direction, that is, from the client to the server.  As
> > far as I can tell, they do:  they all refer to the same direction
> > everywhere, except for in 2 of the 3 Stream5 API functions that have an
> > argument named "to_server".  So this patch changes those 2 functions.
> >
> >  (It would, of course, also be possible to leave the inside of those
> > functions the same, and just change the argument names from "to_server" to
> > "to_client" or "from_server", but that would add inconsistency to the API.)
> >
> >  Does that answer your question?
> >
> >  John
> >
> >
> > On Thu, Feb 20, 2014 at 6:48 AM, Carter Waxman (cwaxman) <
> > cwaxman () cisco com> wrote:
> >
> > >  Hi John,
> > >
> > >  What is the issue you are trying to fix? Is there a particular
> > > behavior this is causing?
> > >
> > >  -Carter
> > >
> > >
> > >   From: John Eure <john.eure () gmail com>
> > > Date: Wednesday, February 19, 2014 6:32 PM
> > > To: snort-devel <snort-devel () lists sourceforge net>
> > > Subject: [Snort-devel] Patch for Stream5 TCP direction
> > >
> > >   Hello, snort-devel,
> > >
> > >  I've got a patch that fixes a tiny issue with some of the Stream5 TCP
> > > API functions.
> > >
> > > Stream5IsPafActiveTcp() takes a boolean called "to_server", but which
> > > actually behaves as if it were "to_client"; I fixed that by swapping the
> > > references to the TcpSession->client/server fields inside the function.
> > > Stream5ActivatePafTcp() has a similar problem, but there are two variables,
> > > one for the StreamTracker and one for the FlushMgr.  I switched the
> > > FlushMgr references, and I know that's correct, but I left the
> > > StreamTracker references alone, and I *think* that's correct, but I'm not
> > > sure.  I don't use this function in my code anymore, so it's hard for me to
> > > verify that everything works correctly, but I know at least the FlushMgr
> > > should switch, so I did that.
> > >
> > >  There's also a related function, Stream5GetPAFUserDataTcp(), which was
> > > already working correctly, so I left it alone.  I've included a few
> > > comments that have helped me keep track of which direction is which.  And
> > > I've updated the http_inspect preprocessor, which was the only one to call
> > > stream_api->is_paf_active(), so that it continues to do the right thing.
> > >
> > > On a related subject, I very much appreciate the recent renaming from
> > > SSN_DIR_CLIENT to SSN_DIR_FROM_CLIENT - it helped me find and fix a few
> > > issues with my own code.  At that point I went through and changed all of
> > > my relevant names to have either "s2c" or "c2s" in them (borrowing that
> > > terminology from the PAF module), and the result is that my code is now
> > > simpler, less buggy, and less prone to cause insanity from trying to figure
> > > out what "client" and "server" actually mean when applied to streams.  I
> > > heartily recommend the practice.   :-)
> > >
> > >  Thanks for everything,
> > >  John Eure
------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk

_______________________________________________
Snort-devel mailing list
Snort-devel () lists sourceforge net
https://lists.sourceforge.net/lists/listinfo/snort-devel
Archive:
http://sourceforge.net/mailarchive/forum.php?forum_name=snort-devel

Please visit http://blog.snort.org for the latest news about Snort!