snort-devel February 2014 archive
Main Archive Page > Month Archives  > snort-devel archives
snort-devel: Re: [Snort-devel] Patch for Stream5 TCP direction

Re: [Snort-devel] Patch for Stream5 TCP direction

From: Russ Combs (rucombs) <rucombs_at_nospam>
Date: Mon Feb 24 2014 - 16:35:12 GMT
To: John Eure <john.eure@gmail.com>

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<mailto: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<mailto: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<mailto: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<mailto:john.eure@gmail.com>>
Date: Wednesday, February 19, 2014 6:32 PM
To: snort-devel <snort-devel@lists.sourceforge.net<mailto: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!