From 8e83d0073e4655388e8dd13c785875996faa494d Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 25 Mar 2015 22:29:10 +0100 Subject: [PATCH] stream: fix bad last_ack update leading to gaps A bad last_ack update where it would be set beyond next_seq could lead to rejection of valid segments and thus stream gaps. Update tests to reflect new last_ack/next_seq behaviour. --- src/stream-tcp.c | 112 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 92 insertions(+), 20 deletions(-) diff --git a/src/stream-tcp.c b/src/stream-tcp.c index d414186bf8..5cc5513ab5 100644 --- a/src/stream-tcp.c +++ b/src/stream-tcp.c @@ -753,18 +753,25 @@ uint32_t StreamTcpGetStreamSize(TcpStream *stream) } /** - * \brief macro to update last_ack only if the new value is higher + * \brief macro to update last_ack only if the new value is higher and + * the ack value isn't beyond the next_seq * * \param ssn session * \param stream stream to update * \param ack ACK value to test and set */ #define StreamTcpUpdateLastAck(ssn, stream, ack) { \ - if (SEQ_GT((ack), (stream)->last_ack)) { \ + if (SEQ_GT((ack), (stream)->last_ack) && \ + (SEQ_LEQ((ack),(stream)->next_seq) || \ + ((ssn)->state >= TCP_FIN_WAIT1 && SEQ_EQ((ack),((stream)->next_seq + 1))))) \ + { \ (stream)->last_ack = (ack); \ SCLogDebug("ssn %p: last_ack set to %"PRIu32, (ssn), (stream)->last_ack); \ StreamTcpSackPruneList((stream)); \ - } \ + } else { \ + SCLogDebug("ssn %p: no update: ack %u, last_ack %"PRIu32", next_seq %u (state %u)", \ + (ssn), (ack), (stream)->last_ack, (stream)->next_seq, (ssn)->state); \ + }\ } /** @@ -2036,6 +2043,7 @@ static int HandleEstablishedPacketToServer(ThreadVars *tv, TcpSession *ssn, Pack /* Check if the ACK value is sane and inside the window limit */ StreamTcpUpdateLastAck(ssn, &ssn->server, TCP_GET_ACK(p)); + SCLogDebug("ack %u last_ack %u next_seq %u", TCP_GET_ACK(p), ssn->server.last_ack, ssn->server.next_seq); if (ssn->flags & STREAMTCP_FLAG_TIMESTAMP) { StreamTcpHandleTimestamp(ssn, p); @@ -9910,12 +9918,11 @@ static int StreamTcpTest38 (void) Flow f; ThreadVars tv; StreamTcpThread stt; - uint8_t payload[4]; + uint8_t payload[128]; TCPHdr tcph; - TcpReassemblyThreadCtx ra_ctx; PacketQueue pq; - memset(&ra_ctx, 0, sizeof(TcpReassemblyThreadCtx)); + TcpReassemblyThreadCtx *ra_ctx = StreamTcpReassembleInitThreadCtx(NULL); memset (&f, 0, sizeof(Flow)); memset(&tv, 0, sizeof (ThreadVars)); memset(&stt, 0, sizeof (StreamTcpThread)); @@ -9933,7 +9940,7 @@ static int StreamTcpTest38 (void) tcph.th_flags = TH_SYN; p->tcph = &tcph; p->flowflags = FLOW_PKT_TOSERVER; - stt.ra_ctx = &ra_ctx; + stt.ra_ctx = ra_ctx; StreamTcpInitConfig(TRUE); SCMutexLock(&f.m); @@ -9983,7 +9990,27 @@ static int StreamTcpTest38 (void) goto end; } - p->tcph->th_ack = htonl(2984); + p->tcph->th_ack = htonl(1); + p->tcph->th_seq = htonl(1); + p->tcph->th_flags = TH_PUSH | TH_ACK; + p->flowflags = FLOW_PKT_TOCLIENT; + + StreamTcpCreateTestPacket(payload, 0x41, 127, 128); /*AAA*/ + p->payload = payload; + p->payload_len = 127; + + if (StreamTcpPacket(&tv, p, &stt, &pq) == -1) { + printf("failed in processing packet in StreamTcpPacket\n"); + goto end; + } + + if (((TcpSession *)(p->flow->protoctx))->server.next_seq != 128) { + printf("the server.next_seq should be 128, but it is %"PRIu32"\n", + ((TcpSession *)(p->flow->protoctx))->server.next_seq); + goto end; + } + + p->tcph->th_ack = htonl(256); // in window, but beyond next_seq p->tcph->th_seq = htonl(5); p->tcph->th_flags = TH_PUSH | TH_ACK; p->flowflags = FLOW_PKT_TOSERVER; @@ -9997,10 +10024,32 @@ static int StreamTcpTest38 (void) goto end; } + /* last_ack value should be 1, not 256, as the previous sent ACK value + is inside window, but beyond next_seq */ + if (((TcpSession *)(p->flow->protoctx))->server.last_ack != 1) { + printf("the server.last_ack should be 1, but it is %"PRIu32"\n", + ((TcpSession *)(p->flow->protoctx))->server.last_ack); + goto end; + } + + p->tcph->th_ack = htonl(128); + p->tcph->th_seq = htonl(8); + p->tcph->th_flags = TH_PUSH | TH_ACK; + p->flowflags = FLOW_PKT_TOSERVER; + + StreamTcpCreateTestPacket(payload, 0x41, 3, 4); /*AAA*/ + p->payload = payload; + p->payload_len = 3; + + if (StreamTcpPacket(&tv, p, &stt, &pq) == -1) { + printf("failed in processing packet in StreamTcpPacket\n"); + goto end; + } + /* last_ack value should be 2984 as the previous sent ACK value is inside window */ - if (((TcpSession *)(p->flow->protoctx))->server.last_ack != 2984) { - printf("the server.last_ack should be 2984, but it is %"PRIu32"\n", + if (((TcpSession *)(p->flow->protoctx))->server.last_ack != 128) { + printf("the server.last_ack should be 128, but it is %"PRIu32"\n", ((TcpSession *)(p->flow->protoctx))->server.last_ack); goto end; } @@ -10013,6 +10062,8 @@ end: SCMutexUnlock(&f.m); SCFree(p); FLOW_DESTROY(&f); + if (stt.ra_ctx != NULL) + StreamTcpReassembleFreeThreadCtx(stt.ra_ctx); return ret; } @@ -10030,10 +10081,9 @@ static int StreamTcpTest39 (void) StreamTcpThread stt; uint8_t payload[4]; TCPHdr tcph; - TcpReassemblyThreadCtx ra_ctx; PacketQueue pq; - memset(&ra_ctx, 0, sizeof(TcpReassemblyThreadCtx)); + TcpReassemblyThreadCtx *ra_ctx = StreamTcpReassembleInitThreadCtx(NULL); memset (&f, 0, sizeof(Flow)); memset(&tv, 0, sizeof (ThreadVars)); memset(&stt, 0, sizeof (StreamTcpThread)); @@ -10052,7 +10102,7 @@ static int StreamTcpTest39 (void) p->tcph = &tcph; p->flowflags = FLOW_PKT_TOSERVER; int ret = 0; - stt.ra_ctx = &ra_ctx; + stt.ra_ctx = ra_ctx; StreamTcpInitConfig(TRUE); @@ -10081,7 +10131,27 @@ static int StreamTcpTest39 (void) goto end; } - p->tcph->th_ack = htonl(2984); + p->tcph->th_ack = htonl(1); + p->tcph->th_seq = htonl(1); + p->tcph->th_flags = TH_PUSH | TH_ACK; + p->flowflags = FLOW_PKT_TOCLIENT; + + StreamTcpCreateTestPacket(payload, 0x41, 3, 4); /*AAA*/ + p->payload = payload; + p->payload_len = 3; + + if (StreamTcpPacket(&tv, p, &stt, &pq) == -1) { + printf("failed in processing packet in StreamTcpPacket\n"); + goto end; + } + + if (((TcpSession *)(p->flow->protoctx))->server.next_seq != 4) { + printf("the server.next_seq should be 4, but it is %"PRIu32"\n", + ((TcpSession *)(p->flow->protoctx))->server.next_seq); + goto end; + } + + p->tcph->th_ack = htonl(4); p->tcph->th_seq = htonl(2); p->tcph->th_flags = TH_PUSH | TH_ACK; p->flowflags = FLOW_PKT_TOSERVER; @@ -10095,15 +10165,15 @@ static int StreamTcpTest39 (void) goto end; } - /* last_ack value should be 2984 as the previous sent ACK value is inside + /* last_ack value should be 4 as the previous sent ACK value is inside window */ - if (((TcpSession *)(p->flow->protoctx))->server.last_ack != 2984) { - printf("the server.last_ack should be 2984, but it is %"PRIu32"\n", + if (((TcpSession *)(p->flow->protoctx))->server.last_ack != 4) { + printf("the server.last_ack should be 4, but it is %"PRIu32"\n", ((TcpSession *)(p->flow->protoctx))->server.last_ack); goto end; } - p->tcph->th_seq = htonl(2984); + p->tcph->th_seq = htonl(4); p->tcph->th_ack = htonl(5); p->tcph->th_flags = TH_PUSH | TH_ACK; p->flowflags = FLOW_PKT_TOCLIENT; @@ -10119,8 +10189,8 @@ static int StreamTcpTest39 (void) /* next_seq value should be 2987 as the previous sent ACK value is inside window */ - if (((TcpSession *)(p->flow->protoctx))->server.next_seq != 2987) { - printf("the server.next_seq should be 2987, but it is %"PRIu32"\n", + if (((TcpSession *)(p->flow->protoctx))->server.next_seq != 7) { + printf("the server.next_seq should be 7, but it is %"PRIu32"\n", ((TcpSession *)(p->flow->protoctx))->server.next_seq); goto end; } @@ -10133,6 +10203,8 @@ end: SCMutexUnlock(&f.m); SCFree(p); FLOW_DESTROY(&f); + if (stt.ra_ctx != NULL) + StreamTcpReassembleFreeThreadCtx(stt.ra_ctx); return ret; } -- 2.47.2