*** CID 1220099: Dereference before null check (REVERSE_INULL)
/src/log-droplog.c: 199 in LogDropLogNetFilter()
193 if (SCConfLogReopen(dlt->file_ctx) != 0) {
194 /* Rotation failed, error already logged. */
195 return TM_ECODE_FAILED;
196 }
197 }
198
>>> CID 1220099: Dereference before null check (REVERSE_INULL)
>>> Null-checking "dlt->file_ctx" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
199 if (dlt->file_ctx == NULL) {
200 return TM_ECODE_FAILED;
201 }
202
203 char srcip[46] = "";
204 char dstip[46] = "";
Victor Julien [Fri, 2 May 2014 15:11:10 +0000 (17:11 +0200)]
http-json: fix coverity warning
*** CID 1211009: Bad bit shift operation (BAD_SHIFT)
/src/output-json-http.c: 265 in JsonHttpLogJSON()
259 /* log custom fields if configured */
260 if (http_ctx->fields != 0)
261 {
262 HttpField f;
263 for (f = HTTP_FIELD_ACCEPT; f < HTTP_FIELD_SIZE; f++)
264 {
>>> CID 1211009: Bad bit shift operation (BAD_SHIFT)
>>> In expression "1 << f", left shifting by more than 31 bits has undefined behavior. The shift amount, "f", is as much as 46.
265 if ((http_ctx->fields & (1<<f)) != 0)
266 {
267 /* prevent logging a field twice if extended logging is
268 enabled */
269 if (((http_ctx->flags & LOG_HTTP_EXTENDED) == 0) ||
270 ((http_ctx->flags & LOG_HTTP_EXTENDED) !=
________________________________________________________________________________________________________
*** CID 1211010: Bad bit shift operation (BAD_SHIFT)
/src/output-json-http.c: 492 in OutputHttpLogInitSub()
486 {
487 if ((strcmp(http_fields[f].config_field,
488 field->val) == 0) ||
489 (strcasecmp(http_fields[f].htp_field,
490 field->val) == 0))
491 {
>>> CID 1211010: Bad bit shift operation (BAD_SHIFT)
>>> In expression "1 << f", left shifting by more than 31 bits has undefined behavior. The shift amount, "f", is as much as 46.
492 http_ctx->fields |= (1<<f);
493 break;
494 }
495 }
496 }
497 }
Victor Julien [Thu, 24 Apr 2014 15:31:08 +0000 (17:31 +0200)]
stream: cleanup
StreamTcpSetDisableRawReassemblyFlag() has the same effect as
AppLayerParserTriggerRawStreamReassembly in that it will force the
raw reassembly to flush out asap. So it is redundant to call both.
Victor Julien [Thu, 24 Apr 2014 08:48:37 +0000 (10:48 +0200)]
stream: implement raw reassembly stop api
Implement StreamTcpSetDisableRawReassemblyFlag() which stops raw
reassembly for _NEW_ segments in a stream direction.
It is used only by TLS/SSL now, to flag the streams as encrypted.
Existing segments will still be reassembled and inspected, while
new segments won't be. This allows for pattern based inspection
of the TLS handshake.
Like is the case with completely disabled 'raw' reassembly, the
logic is that the segments are flagged as completed for 'raw' right
away. So they are not considered in raw reassembly anymore.
As no new segments will be considered, the chunk limit check will
return true on the next call.
Victor Julien [Fri, 2 May 2014 09:01:18 +0000 (11:01 +0200)]
http-json: init 'fields' to 0 before setting it
httplog_ctx->fields would not be initialized before setting flags in
it:
Scanbuild:
output-json-http.c:491:46: warning: The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage
http_ctx->fields |= (1<<f);
~~~~~~~~~~~~~~~~ ^
1 warning generated.
Victor Julien [Wed, 23 Apr 2014 09:00:02 +0000 (11:00 +0200)]
rohash: fix potential bad shift
Fix issue detected byCoverity:
*** CID 1197756: Bad bit shift operation (BAD_SHIFT)
/src/util-rohash.c: 74 in ROHashInit()
68 }
69 if (hash_bits < 4 || hash_bits > 32) {
70 SCLogError(SC_ERR_HASH_TABLE_INIT, "invalid hash_bits setting, valid range is 4-32");
71 return NULL;
72 }
73
>>> CID 1197756: Bad bit shift operation (BAD_SHIFT)
>>> In expression "1U << hash_bits", left shifting by more than 31 bits has undefined behavior. The shift amount, "hash_bits", is as much as 32.
74 uint32_t size = hashsize(hash_bits) * sizeof(ROHashTableOffsets);
75
76 ROHashTable *table = SCMalloc(sizeof(ROHashTable) + size);
77 if (unlikely(table == NULL)) {
78 SCLogError(SC_ERR_HASH_TABLE_INIT, "failed to alloc memory");
79 return NULL;
This was only a potential issue as ROHashInit was only called with
hash_bits 16 in the code.
Eric Leblond [Wed, 23 Apr 2014 09:21:11 +0000 (11:21 +0200)]
af-packet: exit in case of a fatal error
During socket creation all error cases were leading to suricata to
retry the opening of capture. This patch updates this behavior to
have fatal and recoverable error case. In case of a fatal error,
suricata is leaving cleanly.
Victor Julien [Wed, 16 Apr 2014 16:10:22 +0000 (18:10 +0200)]
stream: update GAP detection
Change GAP detection logic. If we encounter missing data before
last_ack, we know we have missed data. The receiving host has ack'd
it already, so a retransmission of the missing data is highly
unlikely.
Victor Julien [Fri, 11 Apr 2014 15:31:46 +0000 (17:31 +0200)]
stream: fix raw reassembly flag issue
AppLayer reassembly correctly only flags a segment as done when it's
completely used in reassembly. Raw reassembly could flag a partially
used segment as complete as well. In this case the segment could be
discarded early. Further reassembly would miss data, leading to a
gap. Due to this, up to 'window size' bytes worth of segments could
remain in the session for a long time, leading to memory resource
pressure.
Victor Julien [Fri, 11 Apr 2014 10:20:04 +0000 (12:20 +0200)]
protocol detection: handle very unbalanced case
Some traffic is very unbalanced. For example a 4 bytes request
followed by 12mb of response data. If the 4 bytes don't lead to
the protocol being detected, then app layer processing won't
start, but it will not give up either. It will just wait for more
data. This leads to piling up data on the opposite side.
Observed:
TS: 4 bytes. PP failed (bit set), PM has not given up (bit not set).
This makes sense as max_depth is not yet reached.
TC: 12mb. PP and PM failed (bits set).
As ts-PM never gives up, we never consider proto detect complete,
so all segments in either direction are still kept in the session.
This patch adds a cutoff for this case:
- IF for TS we have PP bit set, PM not set, AND
- we have TC both bits set, AND
- proto is unknown, AND
- TC data is 100k already, THEN
- give up on protocol detection.
Victor Julien [Fri, 11 Apr 2014 08:11:04 +0000 (10:11 +0200)]
stream: improve midstream reassembly gap detection
The reassembly gap detection makes use of the window. However, in
midstream mode the window size we use is unreliable, as we have to
assume window scaling is in place. This patch improves midstream
handling of those cases.
Victor Julien [Fri, 11 Apr 2014 08:08:55 +0000 (10:08 +0200)]
stream: detect data gap at stream start
In midstream mode we may encounter a case where the data we is beyond
the isn, but below last_ack. This means we're missing some data, that
is already acked so it won't be retransmitted. Therefore, we can
conclude it's a data gap.
Victor Julien [Wed, 22 Jan 2014 18:06:31 +0000 (19:06 +0100)]
proto detect: add cutoff for unbalanced traffic
If we're getting a lot of data in one direction and the proto for this
direction is unknown, proto detect will hold up segments in the segment
list in the stream. They are held so that if we detect the protocol on
the opposing stream, we can still parse this side of the stream as well.
However, some sessions are very unbalanced. FTP data channels, large
PUT/POST request and many others, can lead to cases where we would have
to store many megabytes worth of segments before we see the opposing
stream. This leads to risks of resource starvation.
In this patch, a cutoff point is enforced. If we've stored 100k in one
direction and we've seen no data in the other direction, we give up.
If we've given up, the applayer_proto_detection_skipped event is set.
app-layer-event: applayer_proto_detection_skipped;
Victor Julien [Thu, 10 Apr 2014 15:59:18 +0000 (17:59 +0200)]
protocol detection: midstream handling update
If a TCP session is midstream, we may end up with a case where the
start of an HTTP request is missing. We won't detect HTTP based on
the request. However, the reply is fine, so we detect HTTP anyway.
This leads to passing the incomplete request to the htp parser.
This has been observed, where the http parser then saw many bogus
requests in the incomplete data. This is not limited to HTTP.
To counter this case, a midstream session MUST find it's protocol
in the toserver direction. If not, we assume the start of the
request/toserver is incomplete and no reliable detection and parsing
is possible. So we give up.
Eric Leblond [Thu, 10 Apr 2014 11:11:34 +0000 (13:11 +0200)]
packet handling: fix release function
Extended data were freed before the release function was called.
The result was that, in AF_PACKET IPS mode, the release function
was only sending void data because it the content of the extended
data is the content of the packet.
This patch updates the code to have the freeing of extended data
done in the cleaning function for a packet which is called by the
release function. This improves consistency of the code and fixes
the bug.
Victor Julien [Thu, 3 Apr 2014 11:55:22 +0000 (13:55 +0200)]
proto-detect: update port logic
If a flow matches both an 'sp' based PP registration and a 'dp' based,
until now we would only check the 'dp' one. This patch changes that. It
will inspect both.
Victor Julien [Thu, 27 Mar 2014 15:13:08 +0000 (16:13 +0100)]
app-layer: proto detection update
Instead of the notion of toserver and toclient protocol detection, use
destination port and source port.
Independent of the data direction, the flow's port settings will be used
to find the correct probing parser, where we first try the dest port,
and if that fails the source port.
Update the configuration file format, where toserver is replaced by 'dp'
and toclient by 'sp'. Toserver is intrepreted as 'dp' and toclient as
'sp' for backwards compatibility.
Example for dns:
dns:
# memcaps. Globally and per flow/state.
#global-memcap: 16mb
#state-memcap: 512kb
# How many unreplied DNS requests are considered a flood.
# If the limit is reached, app-layer-event:dns.flooded; will match.
#request-flood: 500
Ken Steele [Tue, 15 Apr 2014 14:18:30 +0000 (10:18 -0400)]
Fix unaligned load in AC-TILE MPM.
The SLOAD define using __insn_ld2s_L2 is used to provide a compiler
hint that the load will come from the L2 cache instead of the L1. It
also specifies that it is a 2 byte signed load. For the Tiny MPM, that
needs to be a 1-byte load, which is what is specified in util-ac-mpm-tile.c,
but the #undef was removing that definition.
Victor Julien [Tue, 15 Apr 2014 10:39:22 +0000 (12:39 +0200)]
detect: fix alstate handling
Previously, the alstate use in the main detect loop was unsafe. The
alstate pointer would be set duing a lock, but it would again be used
after one or more lock/unlock cycles. If the data pointed to would
disappear, a dangling pointer would be the result.
Due to they way flows are cleaned up using reference counting and
such, changes of this happening were very small. However, at least
one path can lead to this situation. So it had to be fixed.
Victor Julien [Tue, 15 Apr 2014 10:23:21 +0000 (12:23 +0200)]
detect: locking update continued
Make DeStateDetectContinueDetection get it's own alstate pointer instead
of using the one that was passed to it. We now get and use it only
inside a flow lock.
Victor Julien [Tue, 15 Apr 2014 09:18:47 +0000 (11:18 +0200)]
detect: locking update
Make DeStateDetectStartDetection get it's own alstate pointer instead
of using the one that was passed to it. We now get and use it only
inside a flow lock.
Victor Julien [Tue, 15 Apr 2014 08:31:48 +0000 (10:31 +0200)]
detect: modify AMATCH locking
This is an intrusive change. This patch modifies the way AMATCH
inspection uses locking.
So far, each keyword did it's own locking. This lead to a situation
where a 'alstate' pointer was passed around that was not always
protected by a lock.
This patch moves the locking to the Stateful detection functions.
Eric Leblond [Thu, 10 Apr 2014 09:05:46 +0000 (11:05 +0200)]
util-ioctl: only get MTU when iface name is set
This patch fixes a warning message when suricata is started without
giving an interface name on the command line. The code was trying
to get the MTU even if pcap_dev was not set.
Victor Julien [Sun, 13 Apr 2014 08:35:36 +0000 (10:35 +0200)]
Fix 2 compiler warnings
FreeBSD 10 32-bit with clang 3.3:
log-tlslog.c:172:14: error: format specifies type 'long' but the argument has type 'time_t' (aka 'int') [-Werror,-Wformat]
p->ts.tv_sec,
^~~~~~~~~~~~
1 error generated.
detect-engine-payload.c:508:27: warning: format specifies type 'long' but the argument has type 'time_t' (aka 'int') [-Wformat]
printf("%ld.%06ld\n", tv_diff.tv_sec, (long int)tv_diff.tv_usec);
~~~ ^~~~~~~~~~~~~~
%d
1 warning generated.
Victor Julien [Thu, 10 Apr 2014 07:45:21 +0000 (09:45 +0200)]
output-api: cleanup handling
Add output 'free list' that contains all the output ctx' that need
cleanup at shutdown. It differs from the runmode output list in that
it will also contain a 'parent' for the submodules that share the
context of it's parent.