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

Next/60x/20220901/v4 #7806

Merged
merged 13 commits into from Sep 1, 2022
Merged

Next/60x/20220901/v4 #7806

merged 13 commits into from Sep 1, 2022

Conversation

victorjulien
Copy link
Member

jufajardini and others added 13 commits August 31, 2022 13:19
@jufajardini
decode: make PacketDrop use action as parameter
6742ecb 
A Packet may be dropped due to several different reasons. This change
adds action as a parameter, so we can update the packet action when we
drop it, instead of setting it to drop.

Related to
Bug OISF#5458

(cherry picked from commit 1774ff1)
@jufajardini
detect/alert: ensure reject action is applied
32f9037 
Bug 5458 states that the reject action is no longer working. While SV
tests that use the reject action still pass, it indeed seems that a
regression has happened with commit aa93984, because while the
function that applies rule actions to the flow (RuleActionToFlow) does
check for the reject action, the newly added function PacketApply
SignatureActions only checks for ACTION_DROP or ACTION_PASS when
deciding to call RuleActionToFlow.

Bug OISF#5458

(cherry picked from commit 1f54e86)
@jufajardini
decode: validate if dropped packet has drop reason
b3ae9f8 
Related to
Bug OISF#5458

(cherry picked from commit abd595d)
@jufajardini
detect/alert: add unittests to check packet action
2b59f65 
Add unittests to check that packet flags are correctly updated after
detection finds drop or reject rules that match.

Related to
Bug OISF#5458

(cherry picked from commit f897761)
@jufajardini
stream/tcp: remove repeated header declaration
dcf990c 
StreamTcpRegisterTests was being declared twice.

(cherry picked from commit d07a6c6)
@jufajardini
decode: remove unused macros
1710c5f 
With the recent changes, these macros weren't being used anymore.

Related to
Bug OISF#5458

(cherry picked from commit e7727c3)
@catenacyber @victorjulien
fuzz: disable enip detection based on source port
7dc493c 
So as to avoid fuzzing detecting protocol polyglots with enip

(cherry picked from commit d1ebf32)
@jufajardini @victorjulien
commandline: add alert-queue expand failure option
8f3ec86 
For testing purposes. Meant to simulate a reallocation failure when
dynamically growing the alert queue in DetectEngineThreadCtx, so we can
check that Suri's behavior doesn't break under such circumstances.

Task OISF#5319

(cherry picked from commit 58928b2)
@jufajardini @victorjulien
detect/engine: fix typos in assorted files
15ec088 
(cherry picked from commit 5f4bcfe)
@jufajardini @victorjulien
detect/parse: test sig parsing for more actions
d21d4c7 
Our unittests were only covering sig parsing for alert actions. As in
environments without LibNet the reject action will not work, we must
ensure that our parser properly fails in such cases, instead of silently
accepting an unsupported action.

Added tests for the reject and drop action.

Task OISF#5496

(cherry picked from commit c81b78f)
@victorjulien
github: bump ubuntu-18.04 runners to 22.04
449944e 
(cherry picked from commit 3dea69e)
@jufajardini @victorjulien
exceptions: error out when invalid policy is used
0a124f2 
Before, if an invalid value was passed as exception policy, Suricata
would log a warning and set the exception policy to "ignore". This is a
very different result, than, say, dropping or bypassing a midstream flow.

Task OISF#5504

(cherry picked from commit 58ef3cd)
@inashivb @victorjulien
doc/conf: fix sphinx language setting
8762699 
sphinx-build 5.1.1 and above throws a warning which is treated as an
error while building.

Invalid configuration value found: 'language = None'. Update your configuration to a valid language code. Falling back to 'en' (English).

(cherry picked from commit 2c4d6b3)
@victorjulien victorjulien requested review from norg and a team as code owners September 1, 2022 09:12
@victorjulien
Copy link
Member Author

@catenacyber now we see

Failed with input length 26 versus 531, found enip instead of smb
Assertion failure: enip-smb

@catenacyber
Copy link
Contributor

I have been digging more and this can be fixed by cherry-picking the commits 05f9b3f and f4449d3

Cherry-pick is not clean, but it is easy to make the commit cf

diff --git a/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c b/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c
index ec6da106f..868b17957 100644
--- a/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c
+++ b/src/tests/fuzz/fuzz_applayerprotodetectgetproto.c
@@ -59,7 +59,11 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
     f->protoctx = &ssn;
     f->protomap = FlowGetProtoMapping(f->proto);
 
-    alproto = AppLayerProtoDetectGetProto(alpd_tctx, f, data+HEADER_LEN, size-HEADER_LEN, f->proto, data[0], &reverse);
+    uint8_t flags = STREAM_TOCLIENT;
+    if (data[0] & STREAM_TOSERVER) {
+        flags = STREAM_TOSERVER;
+    }
+    alproto = AppLayerProtoDetectGetProto(alpd_tctx, f, data+HEADER_LEN, size-HEADER_LEN, f->proto, flags, &reverse);
     if (alproto != ALPROTO_UNKNOWN && alproto != ALPROTO_FAILED && f->proto == IPPROTO_TCP) {
         /* If we find a valid protocol :
          * check that with smaller input
@@ -70,7 +74,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
             // reset detection at each try cf probing_parser_toserver_alproto_masks
             AppLayerProtoDetectReset(f);
             alproto2 = AppLayerProtoDetectGetProto(
-                    alpd_tctx, f, data + HEADER_LEN, i, f->proto, data[0], &reverse);
+                    alpd_tctx, f, data + HEADER_LEN, i, f->proto, flags, &reverse);
             if (alproto2 != ALPROTO_UNKNOWN && alproto2 != alproto) {
                 printf("Failed with input length %" PRIuMAX " versus %" PRIuMAX
                        ", found %s instead of %s\n",

@victorjulien
Copy link
Member Author

Can you do a PR once this one is merged?

@catenacyber
Copy link
Contributor

Ok waiting for the merge then

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 8936

@victorjulien victorjulien merged commit 8762699 into OISF:master-6.0.x Sep 1, 2022
This was referenced Sep 1, 2022
Closed
Closed
Closed
Closed
Closed
@catenacyber
Copy link
Contributor

Can you do a PR once this one is merged?

#7819

@victorjulien victorjulien deleted the next/60x/20220901/v4 branch September 19, 2022 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@norg norg Awaiting requested review from norg norg is a code owner

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
5 participants
@victorjulien @catenacyber @suricata-qa @jufajardini @inashivb