Commit Graph

12092 Commits (6e5b7199cfd04ffbbfce4fa1244b80ed1ef61e2a)
 

Author SHA1 Message Date
Jeff Lucovsky e28d093ae7 general: Fix typo
(cherry picked from commit b53fced452)
5 years ago
Jeff Lucovsky 41f38bd5c0 config/ref: Raise errors for ref.config parsing
This commit raises an error in configuration test mode if there was an
error parsing reference.config.

Issue: 4659
(cherry picked from commit be2155b4ed)
5 years ago
Jason Ish e3638cfd04 logging: change ownership of application log if needed
When running with privilege dropping, the application log file
is opened before privileges are dropped resulting in Suricata
failing to re-open the file for file rotation.

If needed, chown the application to the run-as user/group after
opening.

Ticker #4523

(cherry picked from commit 59ac1fe277)
5 years ago
Jason Ish 38db09bbb5 startup: initialize run as user info sooner
Initialize the run-as user info after loading the config, but
before setting up logging (previously it was done while initializing
signal handlers). This will allow the log file to be given the
correct permissions if Suricata is configured to run as a non-root
user.

(cherry picked from commit 08518df373)
5 years ago
Victor Julien 7b47bffda8 swf: fix coverity warnings
*** CID 1499365:    (UNINIT)
/src/util-file-swf-decompression.c: 98 in FileSwfZlibDecompression()
92         infstream.avail_in = (uInt)compressed_data_len;
93         infstream.next_in = (Bytef *)compressed_data;
94         infstream.avail_out = (uInt)decompressed_data_len;
95         infstream.next_out = (Bytef *)decompressed_data;
96
97         inflateInit(&infstream);

>>>     CID 1499365:    (UNINIT)
>>>     Using uninitialized value "infstream.total_out" when calling "inflate".
98         int result = inflate(&infstream, Z_NO_FLUSH);
99         switch(result) {
100             case Z_STREAM_END:
101                 break;
102             case Z_OK:
103                 break;

/src/util-file-swf-decompression.c: 98 in FileSwfZlibDecompression()
92         infstream.avail_in = (uInt)compressed_data_len;
93         infstream.next_in = (Bytef *)compressed_data;
94         infstream.avail_out = (uInt)decompressed_data_len;
95         infstream.next_out = (Bytef *)decompressed_data;
96
97         inflateInit(&infstream);

>>>     CID 1499365:    (UNINIT)
>>>     Using uninitialized value "infstream.total_out" when calling "inflate".
98         int result = inflate(&infstream, Z_NO_FLUSH);
99         switch(result) {
100             case Z_STREAM_END:
101                 break;
102             case Z_OK:
103                 break;

*** CID 1499363:  Error handling issues  (CHECKED_RETURN)
/src/util-file-swf-decompression.c: 97 in FileSwfZlibDecompression()
91
92         infstream.avail_in = (uInt)compressed_data_len;
93         infstream.next_in = (Bytef *)compressed_data;
94         infstream.avail_out = (uInt)decompressed_data_len;
95         infstream.next_out = (Bytef *)decompressed_data;
96
>>>     CID 1499363:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "inflateInit_(&infstream, "1.2.11", 112)" without checking return value. This library function may fail and return an error code.
97         inflateInit(&infstream);
98         int result = inflate(&infstream, Z_NO_FLUSH);
99         switch(result) {
100             case Z_STREAM_END:
101                 break;
102             case Z_OK:

Bug: #5079.
(cherry picked from commit e1f7c63fa8)
5 years ago
Shivani Bhardwaj 45745ed2ea doc: add usage of flowbits OR op
Ticket 5130

(cherry picked from commit 015c9fe1e3)
5 years ago
Victor Julien 8438dab412 unittests: clean up packet clear logic
(cherry picked from commit 0437ca61ff)
5 years ago
Shivani Bhardwaj 20f4fb7e12 applayer: fix clang formatting 5 years ago
Victor Julien b04a8bdb96 decode: release refs from PacketFree
Mostly helps unittests.

(cherry picked from commit f07d5b2d89)
5 years ago
Victor Julien 8d12189a08 detect/iprep: fix host locking issues
Separate the code paths between reusing a Packet stored host reference
and fetching a new reference from the host hash.

This addresses the issue where in some conditions use_cnt could get
desync'd.

Bug: #2802.
(cherry picked from commit 49a36bb323)
5 years ago
Victor Julien 1ec3a574c7 iprep: unify free handling
Introduce a new util function to free a Hosts iprep code. It also
handles the Host use_cnt decrement.

This change makes sure we also decrement the use_cnt when cleaning
up when shutting down the host table.

Move the BUG_ON check for use_cnt into the HostClearMemory() func
to check it in more cases.

(cherry picked from commit 172d2b28a5)
5 years ago
Victor Julien 5753833e2e detect/iponly: add tests for 5170
(cherry picked from commit 935ea745f5)
5 years ago
Victor Julien 9605cca6fa detect/address: minor unittest cleanup
(cherry picked from commit 053b2b3b5b)
5 years ago
Victor Julien 1932cc1d6b detect/iponly: validate netmask
Only accept netmask in dotted quad notation if they can be turned
into a CIDR.

According to rfc 4632, CIDR (compat) netmasks are all that should be
used.

Bug: #5168.
(cherry picked from commit 79b7b7a0dd)
5 years ago
Victor Julien 8f1644594f detect/address: validate netmasks
Only accept netmask in dotted quad notation if they can be turned
into a CIDR.

According to rfc 4632, CIDR (compat) netmasks are all that should be
used.

Bug: #5168.
(cherry picked from commit 259bd8aa92)
5 years ago
Victor Julien 20171f9be1 detect/iponly: break out range insert code
So we can reuse it.

(cherry picked from commit 4020e2faa7)
5 years ago
Victor Julien cdafefcad7 util/cidr: add util to convert netmask to cidr
(cherry picked from commit a67b97e14c)
5 years ago
Victor Julien 2075038390 nfs: clean up partial record handling
There should be no remaining data after parsing the partial
RPC record, so don't handle it but instead add a debug validation
bug on.

Successful processing for NFSv3 read/write records returns
AppLayerResult::ok() directly as all data is consumed.

(cherry picked from commit 07b1100713)
5 years ago
Victor Julien fb8f4ef849 nfs3: improve read validation; fix partial handling
(cherry picked from commit d85b77cad0)
5 years ago
Victor Julien 29468e60f5 nfs3: fix partial write record handling
Comment from Jason Ish: This was a bit more than a cherry pick as some
of the modifications weren't directly portable to the nom macros. So
instead bring in some of the nom function based parsers as they work
fine with nom 5.

(cherry picked from commit 4418fc1b02)
5 years ago
Victor Julien 8c09ba0508 nfs3: enforce more values
Enforce values of a number of u32's that are used as bools or for
really low values.

(cherry picked from commit 5baf94e40d)
5 years ago
Victor Julien 5bb752e65a rpc: enforce various field values
Minimal frag_len. Correct msgtype and others.

(cherry picked from commit 1c57e3c18d)
5 years ago
Victor Julien 98773d0985 nfs/rpc: update full record parsers to be more exact
Instead of 'take'ing all data for the RPC prog_data and then
letting the higher level parsers figure out which part to use
take the exact amount.

Comments from Shivani Bhardwaj: This is was more than a cherry pick and
some of the modifications were not directly portable to the nom macros.
So, parsers were changed to make sure the functionality remained same while
making the transition to nom5 while keeping the diff minimal.

(cherry picked from commit 64d8a1e16e)
5 years ago
Victor Julien 12beed09ae nfs: break out partial record handling
(cherry picked from commit bfb5ae867e)
5 years ago
Victor Julien 353bfd6d0a nfs/rpc: enforce length field limits
Limits based on the Linux kernel limits. Then multiplied a few times
to allow for other implementations to have higher limits.

(cherry picked from commit fe76ab1803)
5 years ago
Victor Julien 1fba08a27d nfs4: verify bool fields
(cherry picked from commit 5ecb626e50)
5 years ago
Victor Julien 4031328a43 output: fix timestamp missing usecs
On ARM 32bit with Musl `tv_usecs` is defined as `int64_t` which lead to
CreateIsoTimeString() printing all zeros on the usecs. Work around this
by first assigning to a `int64_t` and then updating the expected format
string to accept `int64_t`.

Bug: #5094.
(cherry picked from commit a0c0471f1f)
5 years ago
Victor Julien eea65ca0b1 detect/address: use common cidr code
(cherry picked from commit 8a73b242e3)
5 years ago
Victor Julien dec98984f8 radix: fix unittests after stict checks
(cherry picked from commit 38aec1439c)
5 years ago
Victor Julien 594ac6f9ab radix: improve address range handling
Handle non-exact address ranges from string. This can come directly
from user input, so here it is accepted but the address is converted
to the address range start. A warning will be issued.

Debug validation checks are added to catch this.

This issue could lead to bad input from iprep (with cidr), defrag config
and htp server personalities to produce a bad radix tree.

Bug: #5084.
Bug: #5085.
Bug: #5086.
(cherry picked from commit 7fd6fe732b)
5 years ago
Victor Julien 5d64c811c2 detect/iponly: fix netmask handling
If the ipaddress was not the address range start, it was not masked to turn
it into that. So 1.2.3.4/24 was not stored as address 1.2.3.0 with netmask 24,
but as 1.2.3.4 with netmask 24. This was then propagated into the radix tree,
where it was used as an exact key in exact lookups, giving unexpected results.

This patch implements the netmask handling for IPv4 and IPv6, and adds a set
of tests for it.

Bug: #5081.
Bug: #5066.
(cherry picked from commit 51d4e0dced)
5 years ago
Victor Julien 95aac74828 radix: fix unittest not cleaning up
(cherry picked from commit 311085dd34)
5 years ago
Victor Julien e45f05c33a detect/iponly: update SigNumArray comment
(cherry picked from commit 860daceb04)
5 years ago
Victor Julien 053c512e32 radix: cleanup test
(cherry picked from commit d04dface20)
5 years ago
Victor Julien 69db383c27 radix: add tests for Bug #5066
Bug: #5066.
(cherry picked from commit 89b7ac0a60)
5 years ago
Victor Julien 3d9b420bef radix: fix FP/FN issue in IP-only
A bug was reported about the IP-only rules not correctly matching. This was
traced to the rules in question not getting recorded into the IP-only radix
tree correctly.

Sequence:

- 100.117.241.0/25 inserted into the tree

- 100.117.241.0/26 inserted into the tree

Both are part of the same radix node, but recorded by their different netmasks
in the user data portion.

Then faulty insert happens:

- 100.117.241.64/26

For reference, these net blocks compute to:

- 100.117.241.0/25:  100.117.241.0  - 100.117.241.127
- 100.117.241.0/26:  100.117.241.0  - 100.117.241.63
- 100.117.241.64/26: 100.117.241.64 - 100.117.241.127

The IP-only engine first does a search to get to the user data it may need to
include. It does so for with `SCRadixFindKeyIPV4ExactMatch` for single IPs, or
using `SCRadixFindKeyIPV4Netblock` in case of a netblock. Any "match" from
either of these is considered an "exact match" by the IP-only setup code.

This exact match expectation turned out to be wrong and
`SCRadixFindKeyIPV4Netblock` behaved more like "best match" instead, which is
a non-exact match, but its the next best match if no exact match is found.

The way the look up for 100.117.241.64/26 went wrong, is that it returned
the user data for 100.117.241.0/26. This happens as follows:

- first it would do an exact find, which didn't give a result

- then it removed bits from the keystream until it found a matching node
  and explore if any of the netmasks it contained matched. Here the first
  step of the bug started:

  it considered the netmask (with user data) a match that matched the
  number of bits of the matching key, but not of the actual range netmask cidr
  value.

  So in this case the number of shared bits between `100.117.241.0/25` and
  `100.117.241.64/26` was 25, so it assumed that the user data for the
  netmask 25 was the match.

  To summarize this step, there are 2 problems with this:
  1. it returns a match on something that isn't an exact match
  2. it considered the wrong netmask value

- the radix code then took the returned node, and did the netmask check
  again. This time it did use its own netmask value, so this time
  it did find the netmask 26 (+ user data). However because of the node that
  was returned, this netmask (+user data) belongs to `100.117.241.0`, not to
  `100.117.241.64`.

- the IP-only detection code was satisfied with what it assumed to be
  "exact match" and just updated the user data to include the user data that
  should have been associated with `100.117.241.64/26` to `100.117.241.0/26`.

This patch addresses the issue as follows:

It makes `SCRadixFindKeyIPV4Netblock` also return an exact match by propagating
the netmask in the search and in the evaluation of the stored netmasks.

It does away with the secondary netmask (+user data) evaluation.
`SCRadixFindKeyIPV4Netblock` is expected to handle this correctly.

The IP-only engine will fall back to the "not found" path, which does an explicit
"best match" lookup and then insert a new entry into the radix tree based on
the user data of the "best match".

Issue was present for IPv6 as well.

Bug: #5066.
(cherry picked from commit 6aa6e3f953)
5 years ago
Victor Julien 90b0b9dd01 detect/iponly: cleanups
(cherry picked from commit 6381b1a643)
5 years ago
Victor Julien 1bdfdcf139 detect/iponly: minor debug 'Print' improvements
(cherry picked from commit de4354abcb)
5 years ago
Victor Julien 6f90a07907 radix: minor debug 'Print' improvements
(cherry picked from commit 3ca3c9dfbe)
5 years ago
Victor Julien 20bce78ab1 util/cidr: simplify IPv4 CIDR handling; add IPv6
Instead of building a table at init just calculate it on demand.

Callsites are all during init, so its not performance critical.

Add similar function for IPv6.

(cherry picked from commit e04d378e58)
5 years ago
Philippe Antoine 18bf8c89d9 mqtt: parse properties with the right buffer's length
(cherry picked from commit f0e869b26b)
5 years ago
Eloy Pérez González 1556ea0ceb dcerpc: remove prev_tx_call_id and clear_bind_cache from DCERPCState
remove those fields since are not used because of the removal of
handle_bind_cache.

(cherry picked from commit 15f493f516)
5 years ago
Eloy Pérez González 0c400b2ddb dcerpc: avoid delete the rpc state interface context
The bug:
The dcerpc dce_iface keyword just match the packet following the bind. Only the
next request after the rpc is sent will match. However the expected behaviour it
that all the rpc requests/responses sent under the context of the given
interface would match.

In the Open Group c706 the following is indicated:

In 2.2.1 Binding-related Operations, indicates that one category of binding
operations are those that "operations that establish internal call routing
information for the server." (The other are to establish the protocol which is
not relevant here). And the following statement can be found:

Operations in the second category establish a set of mappings that the server
can use to route calls internally to the appropriate manager routine. This
routing is based on the interface and version, operation and any object
requested by the call.

It indicates that server routes (to call methods) are based on the operation,
interface and object.

- Operation: To indicate the method to call, and operation number is
             specified as indicated in the second step of 2.3.3.2 (Client
             Binding Steps).
- Interface: An interface is a set of remotely callable operations offered by a
             server and invokable by clients. (2.1.1.1)
- Object: Is the manager that implements the interface, as stated in section
          Interface and Manager Selection of 2.3.3.3. It is not mandatory, can
          be nil.

To call a method, a client must send a request message as defined in 2.6.4.9,
that contains these identifiers:

- opnum: The opnum field identifies the operation being invoked within the
         interface.
- p_cont_id (Context ID in Wireshark): The p_cont_id field holds a presentation
                                       context identifier that identifies the
                                       data representation and interface, as
                                       defined in 12.6.3.4 (Context Identifiers).
- object: The object field is contained if the PFC_OBJECT_UUID is set. (Could be
          interesting to create a keyword dce_object for matching this UUID)

Therefore, to get the correct method to invoke, the server must map the context
to the correct interface. This is negotiated by the bind request

Interfaces are first negotiated using the bind message (12.6.4.3), contained in
the p_context_elem array. Then they are accepted or rejected using the bind_ack
message (12.6.4.4).

Once these contexts are established, both client and server can use the context
id, which is the index of the p_context_elem array, to refer the interface they
are using.

Moreover, in the middle of the connection, the context can be changed with the
alter_context message.

This is way suricata shouldn't delete the bindack attribute, that contains
the contexts, used by match_backuuid. This is the only way to know the interface
a request message is referring to.

ticket: 4769
https://redmine.openinfosecfoundation.org/issues/4769

(cherry picked from commit 1aca2676a6)
5 years ago
Eloy Pérez González 3028bdadc5 smb/dce_iface: avoid deleting current ifaces from state
The smb dce_iface keyword must match for all those dcerpc requests
and responses sent in the context of the given interface. They are
not matching as the current bind interfaces are deleted by any
non bind message.

Ticket: 4767
(cherry picked from commit bff0774767)
5 years ago
Eloy Pérez González 9bc31affa4 smb/dce_iface: use DCERPC_TYPE_REQUEST
The smb dce_iface keyword must match for all those dcerpc requests and
responses sent in the context of the given interface. They are not
matching because in rs_smb_tx_get_dce_iface, x.req_cmd is erroneously
compared with 1. Fix this by comparing with DCERPC_TYPE_REQUEST instead.

Ticket: 4767
(cherry picked from commit 1ae22fd5de)
5 years ago
Eloy Pérez González 09c8b17bf7 smb/dce_opnum: move range if to outer context
The smb dce_opnum matches all the opnums that are higher that the
indicated opnum. This is due the range comparison if was put in the
exact comparison context, and in case the opnum doesn't match exactly,
then the range comparison is triggered (the upper limit is always true).

Move the erroneus if to the outer context, as else option of the block
checks if comparison should be exact or range.

Ticket: 4767
(cherry picked from commit 333db3b385)
5 years ago
Eloy Pérez González 97d90ea62c smb/dce_opnum: use DCERPC_TYPE_REQUEST
The smb dce_opnum keyword doesn't match the dcerpc requests/responses.
This occurs because in the rs_smb_tx_match_dce_opnum function, the
x.req_cmd is matched against the erroneous code 1. Fix this by using
DCERPC_TYPE_REQUEST for the comparison instead.

Ticket: 4767
(cherry picked from commit 8dca3d0416)
5 years ago
Victor Julien 0e299c30eb github-ci: replace failing CentOS 8 by AlmaLinux 8.4
(cherry picked from commit 6c240938b5)
5 years ago
Jason Ish 85e42c9d47 github-ci: use curl -L for npcap: follow redirects
(cherry picked from commit 90996958aa)
5 years ago
Angelo Mirabella b4caf35d20 stream-tcp-reassemble: fix reassembly direction for FIN packets
Suricata invokes the stream reassembly logic only for the current packet
direction if the packet contains a FIN flag. However, this does not
handle the case in which the packet ACKs data from the opposing direction.
This patch forces the invocation of the stream reassembly logic
on both direction when Suricata sees a FIN packet.

(cherry picked from commit 41a139b590)
5 years ago