Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ips handling/v31 #7393

Closed
wants to merge 12 commits into from
Closed

Ips handling/v31 #7393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
92a6cb6
app-layer: make registration structure more compact
victorjulien May 3, 2022
f406f89
WIP streaming/buffer: add debug validation for 'impossible' condition
victorjulien Mar 22, 2022
7d01153
stream/segtree: improve docs, error handling
victorjulien Mar 22, 2022
e90a7b1
stream: detect spurious retransmissions early
victorjulien Mar 22, 2022
11f25be
WIP stream/segtree: simplify error handling
victorjulien Mar 22, 2022
bef5cad
stream: remove now unused tcp.insert_list_fail counter
victorjulien Mar 22, 2022
a21a682
stream/reassemble: clarify error handling
victorjulien Mar 22, 2022
6f7c4a1
stream/tests: remove bad test; update failing test to new behavior
victorjulien Mar 22, 2022
7d2f635
eve/drop: log drop reason
victorjulien Mar 21, 2022
878ea78
decode: turn no payload/packet inspect macros into funcs
victorjulien May 2, 2022
88dcfea
decode: add pass action wrapper
victorjulien May 2, 2022
def8b8f
exceptions: initial memcap-policy implementation
victorjulien Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,7 @@ noinst_HEADERS = \
util-ebpf.h \
util-enum.h \
util-error.h \
util-exception-policy.h \
util-file-decompression.h \
util-file.h \
util-file-swf-decompression.h \
Expand Down Expand Up @@ -1104,6 +1105,7 @@ libsuricata_c_a_SOURCES = \
util-ebpf.c \
util-enum.c \
util-error.c \
util-exception-policy.c \
util-file.c \
util-file-decompression.c \
util-file-swf-decompression.c \
Expand Down
37 changes: 32 additions & 5 deletions src/app-layer-parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,14 @@ typedef struct AppLayerParserProtoCtx_
{
/* 0 - to_server, 1 - to_client. */
AppLayerParserFPtr Parser[2];

bool logger;

/* Indicates the direction the parser is ready to see the data
* the first time for a flow. Values accepted -
* STREAM_TOSERVER, STREAM_TOCLIENT */
uint8_t first_data_dir;

uint32_t logger_bits; /**< registered loggers for this proto */

void *(*StateAlloc)(void *, AppProto);
Expand Down Expand Up @@ -129,11 +136,6 @@ typedef struct AppLayerParserProtoCtx_
/* each app-layer has its own value */
uint32_t stream_depth;

/* Indicates the direction the parser is ready to see the data
* the first time for a flow. Values accepted -
* STREAM_TOSERVER, STREAM_TOCLIENT */
uint8_t first_data_dir;

/* Option flags such as supporting gaps or not. */
uint32_t option_flags;
/* coccinelle: AppLayerParserProtoCtx:option_flags:APP_LAYER_PARSER_OPT_ */
Expand Down Expand Up @@ -170,6 +172,13 @@ struct AppLayerParserState_ {
FramesContainer *frames;
};

enum ExceptionPolicy g_applayerparser_error_policy = EXCEPTION_POLICY_IGNORE;

static void AppLayerConfg(void)
{
g_applayerparser_error_policy = ExceptionPolicyParse("app-layer.error-policy", true);
}

static void AppLayerParserFramesFreeContainer(FramesContainer *frames)
{
if (frames != NULL) {
Expand Down Expand Up @@ -1309,6 +1318,22 @@ int AppLayerParserParse(ThreadVars *tv, AppLayerParserThreadCtx *alp_tctx, Flow
if (input_len > 0 || (flags & STREAM_EOF)) {
Setup(f, flags & (STREAM_TOSERVER | STREAM_TOCLIENT), input, input_len, flags,
&stream_slice);
#ifdef DEBUG
if (((stream_slice.flags & STREAM_TOSERVER) &&
stream_slice.offset >= g_eps_applayer_error_offset_ts)) {
SCLogNotice("putting parser %s into an error state from toserver offset %" PRIu64,
AppProtoToString(alproto), g_eps_applayer_error_offset_ts);
AppLayerIncParserErrorCounter(tv, f);
goto error;
}
if (((stream_slice.flags & STREAM_TOCLIENT) &&
stream_slice.offset >= g_eps_applayer_error_offset_tc)) {
SCLogNotice("putting parser %s into an error state from toclient offset %" PRIu64,
AppProtoToString(alproto), g_eps_applayer_error_offset_tc);
AppLayerIncParserErrorCounter(tv, f);
goto error;
}
#endif
/* invoke the parser */
AppLayerResult res = p->Parser[direction](f, alstate, pstate, stream_slice,
alp_tctx->alproto_local_storage[f->protomap][alproto]);
Expand Down Expand Up @@ -1646,6 +1671,8 @@ void AppLayerParserRegisterProtocolParsers(void)
{
SCEnter();

AppLayerConfg();

RegisterHTPParsers();
RegisterSSLParsers();
rs_dcerpc_register_parser();
Expand Down
37 changes: 27 additions & 10 deletions src/app-layer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,13 @@
#include "flow-util.h"
#include "flow-private.h"
#include "ippair.h"

#include "util-debug.h"
#include "util-print.h"
#include "util-profiling.h"
#include "util-validate.h"
#include "decode-events.h"

#include "app-layer-htp-mem.h"
#include "util-exception-policy.h"

/**
* \brief This is for the app layer in general and it contains per thread
Expand Down Expand Up @@ -334,6 +333,8 @@ static int TCPProtoDetectTriggerOpposingSide(ThreadVars *tv,
return ret;
}

extern enum ExceptionPolicy g_applayerparser_error_policy;

/** \todo data const
* \retval int -1 error
* \retval int 0 ok
Expand Down Expand Up @@ -442,8 +443,7 @@ static int TCPProtoDetect(ThreadVars *tv,
if (TCPProtoDetectTriggerOpposingSide(tv, ra_ctx,
p, ssn, *stream) != 0)
{
DisableAppLayer(tv, f, p);
SCReturnInt(-1);
goto detect_error;
}
if (FlowChangeProto(f)) {
/* We have the first data which requested a protocol change from P1 to P2
Expand Down Expand Up @@ -479,8 +479,7 @@ static int TCPProtoDetect(ThreadVars *tv,
if (first_data_dir && !(first_data_dir & ssn->data_first_seen_dir)) {
AppLayerDecoderEventsSetEventRaw(&p->app_layer_events,
APPLAYER_WRONG_DIRECTION_FIRST_DATA);
DisableAppLayer(tv, f, p);
SCReturnInt(-1);
goto detect_error;
}
/* This can happen if the current direction is not the
* right direction, and the data from the other(also
Expand Down Expand Up @@ -511,7 +510,7 @@ static int TCPProtoDetect(ThreadVars *tv,
StreamTcpUpdateAppLayerProgress(ssn, direction, data_len);
}
if (r < 0) {
SCReturnInt(-1);
goto parser_error;
}
} else {
/* if the ssn is midstream, we may end up with a case where the
Expand Down Expand Up @@ -561,8 +560,7 @@ static int TCPProtoDetect(ThreadVars *tv,
if ((ssn->data_first_seen_dir != APP_LAYER_DATA_ALREADY_SENT_TO_APP_LAYER) &&
(first_data_dir) && !(first_data_dir & flags))
{
DisableAppLayer(tv, f, p);
SCReturnInt(-1);
goto detect_error;
}

/* if protocol detection is marked done for our direction we
Expand Down Expand Up @@ -594,7 +592,7 @@ static int TCPProtoDetect(ThreadVars *tv,
SCLogDebug("packet %"PRIu64": pd done(us %u them %u), parser called (r==%d), APPLAYER_DETECT_PROTOCOL_ONLY_ONE_DIRECTION set",
p->pcap_cnt, *alproto, *alproto_otherdir, r);
if (r < 0) {
SCReturnInt(-1);
goto parser_error;
}
}
*alproto = ALPROTO_FAILED;
Expand All @@ -620,6 +618,12 @@ static int TCPProtoDetect(ThreadVars *tv,
}
}
SCReturnInt(0);
parser_error:
ExceptionPolicyApply(p, g_applayerparser_error_policy, PKT_DROP_REASON_APPLAYER_ERROR);
SCReturnInt(-1);
detect_error:
DisableAppLayer(tv, f, p);
SCReturnInt(-2);
Comment on lines +621 to +626
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if a parser enters an error state, a policy is applied, perhaps event to the flow. On detect error, we disable the parser, will any policy be applied to those packets that will now no longer be parsed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

policy for protocol detection exceptions isn't implemented. This change is just more clearly separate the errors by parser and pd

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the above should change the existing pd exception behavior in any way

}

/** \brief handle TCP data for the app-layer.
Expand Down Expand Up @@ -681,6 +685,10 @@ int AppLayerHandleTCPData(ThreadVars *tv, TcpReassemblyThreadCtx *ra_ctx,
PACKET_PROFILING_APP_END(app_tctx, f->alproto);
/* ignore parser result for gap */
StreamTcpUpdateAppLayerProgress(ssn, direction, data_len);
if (r < 0) {
ExceptionPolicyApply(p, g_applayerparser_error_policy, PKT_DROP_REASON_APPLAYER_ERROR);
SCReturnInt(-1);
}
goto end;
}

Expand Down Expand Up @@ -760,6 +768,11 @@ int AppLayerHandleTCPData(ThreadVars *tv, TcpReassemblyThreadCtx *ra_ctx,
PACKET_PROFILING_APP_END(app_tctx, f->alproto);
if (r != 1) {
StreamTcpUpdateAppLayerProgress(ssn, direction, data_len);
if (r < 0) {
ExceptionPolicyApply(
p, g_applayerparser_error_policy, PKT_DROP_REASON_APPLAYER_ERROR);
SCReturnInt(-1);
}
}
}
}
Expand Down Expand Up @@ -854,6 +867,10 @@ int AppLayerHandleUdp(ThreadVars *tv, AppLayerThreadCtx *tctx, Packet *p, Flow *
PACKET_PROFILING_APP_END(tctx, f->alproto);
PACKET_PROFILING_APP_STORE(tctx, p);
}
if (r < 0) {
ExceptionPolicyApply(p, g_applayerparser_error_policy, PKT_DROP_REASON_APPLAYER_ERROR);
SCReturnInt(-1);
}

SCReturnInt(r);
}
Expand Down
1 change: 0 additions & 1 deletion src/app-layer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,5 +175,4 @@ static inline uint32_t StreamSliceGetGapSize(const StreamSlice *stream_slice)
{
return StreamSliceGetDataLen(stream_slice);
}

#endif
4 changes: 4 additions & 0 deletions src/decode-events.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,10 @@ const struct DecodeEvents_ DEvents[] = {
"stream.pkt_retransmission",
STREAM_PKT_RETRANSMISSION,
},
{
"stream.pkt_spurious_retransmission",
STREAM_PKT_SPURIOUS_RETRANSMISSION,
Comment on lines +806 to +807
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs rule?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, disabled by default though. This is quite common. Might not even warrant an event, but I feel if suri classifies it as something we should be able to match on it

},
{
"stream.pkt_bad_window_update",
STREAM_PKT_BAD_WINDOW_UPDATE,
Expand Down
1 change: 1 addition & 0 deletions src/decode-events.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ enum {
STREAM_PKT_BROKEN_ACK,
STREAM_RST_INVALID_ACK,
STREAM_PKT_RETRANSMISSION,
STREAM_PKT_SPURIOUS_RETRANSMISSION,
STREAM_PKT_BAD_WINDOW_UPDATE,

STREAM_SUSPECTED_RST_INJECT,
Expand Down
32 changes: 32 additions & 0 deletions src/decode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,38 @@ const char *PktSrcToString(enum PktSrcEnum pkt_src)
return pkt_src_str;
}

const char *PacketDropReasonToString(enum PacketDropReason r)
{
switch (r) {
case PKT_DROP_REASON_DECODE_ERROR:
return "decode error";
case PKT_DROP_REASON_DEFRAG_ERROR:
return "defrag error";
case PKT_DROP_REASON_DEFRAG_MEMCAP:
return "defrag memcap";
case PKT_DROP_REASON_FLOW_MEMCAP:
return "flow memcap";
case PKT_DROP_REASON_FLOW_DROP:
return "flow drop";
case PKT_DROP_REASON_STREAM_ERROR:
return "stream error";
case PKT_DROP_REASON_STREAM_MEMCAP:
return "stream memcap";
case PKT_DROP_REASON_APPLAYER_ERROR:
return "applayer error";
case PKT_DROP_REASON_APPLAYER_MEMCAP:
return "applayer memcap";
case PKT_DROP_REASON_RULES:
return "rules";
case PKT_DROP_REASON_RULES_THRESHOLD:
return "threshold detection_filter";
case PKT_DROP_REASON_NOT_SET:
default:
return NULL;
}
}

/* TODO drop reason stats! */
void CaptureStatsUpdate(ThreadVars *tv, CaptureStats *s, const Packet *p)
{
if (unlikely(PacketTestAction(p, (ACTION_REJECT | ACTION_REJECT_DST | ACTION_REJECT_BOTH)))) {
Expand Down
Loading