Add ndpi integration #11671

cardigliano · 2024-08-28T16:26:43Z

Make sure these boxes are signed before submitting your Pull Request -- thank you.

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Describe changes:

Add configure --enable-ndpi and --with-ndpi params to enable nDPI support and link the nDPI library
Add nDPI metadata export in alerts/netflow (Eve JSON)
Add ndpi-protocol keyword to match Layer-7 protocols detected by nDPI
Add ndpi-risk keyword to match flow risks detected by nDPI

Add configure --enable-ndpi and --with-ndpi params Add nDPI metadata export in alerts/netflow (Eve JSON) Add ndpi-protocol signature match Add ndpi-risk signature match

github-actions · 2024-08-28T16:27:22Z

NOTE: This PR may contain new authors.

codecov · 2024-08-28T16:46:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.37%. Comparing base (b1e7917) to head (ca32564).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11671      +/-   ##
==========================================
- Coverage   83.37%   83.37%   -0.01%     
==========================================
  Files         910      910              
  Lines      257556   257562       +6     
==========================================
- Hits       214748   214745       -3     
- Misses      42808    42817       +9

Flag	Coverage Δ
fuzzcorpus	`61.55% <83.33%> (+<0.01%)`	⬆️
livemode	`19.44% <16.66%> (+0.03%)`	⬆️
pcap	`44.46% <50.00%> (-0.04%)`	⬇️
suricata-verify	`62.78% <100.00%> (+0.02%)`	⬆️
unittests	`59.35% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

jasonish · 2024-08-28T17:33:50Z

Thanks for the submission. Could you please create a ticket that discusses this feature, the benefits, etc.

cardigliano · 2024-08-29T07:57:39Z

Thanks for the submission. Could you please create a ticket that discusses this feature, the benefits, etc.

I created https://redmine.openinfosecfoundation.org/issues/7231

catenacyber · 2024-09-01T19:38:48Z

My big question is how could this work as a plugin ?
Leaving a few remarks in the code about this...

catenacyber · 2024-09-01T19:39:23Z

src/decode.h

@@ -580,6 +580,10 @@ typedef struct Packet_
    uint8_t *payload;
    uint16_t payload_len;

+#ifdef HAVE_NDPI
+    uint16_t ip_len;


I guess we can avoid to store this, and recompute it based on the total packet length and offset of ip header...

Yeah, we'll want to get rid of additional #ifdef here as well for the move to a plugin.

catenacyber · 2024-09-01T19:41:05Z

src/flow.h

@@ -503,6 +503,12 @@ typedef struct Flow_
    uint64_t todstbytecnt;
    uint64_t tosrcbytecnt;

+#ifdef HAVE_NDPI


Could this somehow go into the storage section below ?

Perhaps by using FlowStorageRegister and friends? Example usage in app-layer-expectation.c.

catenacyber · 2024-09-01T19:41:52Z

src/flow.c

@@ -387,6 +387,41 @@ void FlowHandlePacketUpdate(Flow *f, Packet *p, ThreadVars *tv, DecodeThreadVars
 {
    SCLogDebug("packet %"PRIu64" -- flow %p", p->pcap_cnt, f);

+#ifdef HAVE_NDPI
+    if (tv->ndpi_struct && f->ndpi_flow && (!f->detection_completed) && (p->ip_len > 0)) {


So, we would need a hook here in FlowHandlePacketUpdate for every packet...

catenacyber · 2024-09-01T19:42:30Z

src/output-json-file.h

+JsonBuilder *JsonBuildFileInfoRecord(
+        const Packet *p, const File *ff, void *tx, const uint64_t tx_id, const bool stored,
+        uint8_t dir, HttpXFFCfg *xff_cfg, OutputJsonCtx *eve_ctx
+#ifdef HAVE_NDPI


I guess we could accept this for every case (even if it unused in most cases)

catenacyber · 2024-09-01T19:43:07Z

src/output-json-flow.h

@@ -27,5 +27,8 @@
 void JsonFlowLogRegister(void);
 void EveAddFlow(Flow *f, JsonBuilder *js);
 void EveAddAppProto(Flow *f, JsonBuilder *js);
+#ifdef HAVE_NDPI
+void EveAddnDPIProto(Flow *f, JsonBuilder *js, ThreadVars *tv);


catenacyber · 2024-09-01T19:46:12Z

src/threadvars.h

@@ -135,6 +135,10 @@ typedef struct ThreadVars_ {
    struct FlowQueue_ *flow_queue;
    bool break_loop;

+#ifdef HAVE_NDPI
+    struct ndpi_detection_module_struct *ndpi_struct;


Hmm... I do not know about this one... (how we could make it work as a plugin)

could just use thread_local?

Would we delay initialization until first use, which would be in the packet path then?

I don't think a callback on thread creation would work, as in the library work a thread may be provided by the user, in which case some sort of callback with some user storage in the ThreadVar might make more sense?

I'm fine with user data on ThreadVars. When/how to construct/destruct needs some thought. Do plugins have "thread init" callbacks? Where it would be called once per thread at init?

I'm fine with user data on ThreadVars. When/how to construct/destruct needs some thought. Do plugins have "thread init" callbacks? Where it would be called once per thread at init?

No they do not, but this is what I was thinking we might need.

catenacyber · 2024-09-01T19:49:22Z

src/output-json.h

@@ -120,4 +120,7 @@ OutputJsonThreadCtx *CreateEveThreadCtx(ThreadVars *t, OutputJsonCtx *ctx);
 void FreeEveThreadCtx(OutputJsonThreadCtx *ctx);
 void JSONFormatAndAddMACAddr(JsonBuilder *js, const char *key, const uint8_t *val, bool is_array);

+#ifdef HAVE_NDPI
+void ndpiJsonBuilder(Flow *f, JsonBuilder *js, ThreadVars *tv);


And we would need to hook into logging...

lucaderi · 2024-09-04T15:45:03Z

My big question is how could this work as a plugin ? Leaving a few remarks in the code about this...

We are open to both a plugin and native support. The latter is the best IMHO as it enables all users to exploit it, but if a plugin is just a packaging detail fine for me.

Just let us know what we can do to expedite your decision as we would like to push additional features after this PR is likely merged.

jasonish

After some discussion on our side, we'd like to help where we can to make this a plugin a.s.a.p. This will require some work on our side as we don't have all the support required yet, but this PR does help identify where we need to provide the hooks.

On your end could you:

see about using thread local storage
remove the additional fields from the header and see about using the storage apis
register and ndpi feature so rules can conditinally use this

On our side we'll closely review this for the hooks that are needed and prepare a PR from our side.

I think this would be a plugin we'll bundle with Suricata though, so the user experience won't be much different than it is with this PR.

lucaderi · 2024-09-28T17:41:40Z

Hi @jasonish do you have some updates to share? We're ready to move forward with the plugin development as soon as you're ready. Thanks

jufajardini

Had some minor comments that for some reason were pending since forever 😓

jufajardini · 2024-08-30T19:41:29Z

src/output-json-file.c

+#ifdef HAVE_NDPI
+
+static void ndpiJsonBuilderTLSQUIC(Flow *f, JsonBuilder *js, ThreadVars *tv)
+{


Should we move these ndpi-related changes to their own file?

nit: naming convention: s/n/N ?

jufajardini · 2024-08-30T19:45:49Z

src/tm-threads.c

+        ndpi_set_protocol_detection_bitmask2(tv->ndpi_struct, &protos);
+        ndpi_finalize_initialization(tv->ndpi_struct);
+
+        /* printf("%s - ndpi_init_detection_module()\n", __FUNCTION__); */


Is keeping these necessary? If yes, couldn't they be changed to SCLogDebug statements?

jufajardini · 2024-08-30T20:02:54Z

doc/userguide/rules/ndpi-risk.rst

+- Self-signed Certificate
+- Susp DGA Domain name
+- Malware host contacted
+- and many other...


Q: Should we add any a reference here for what was consulted to define those and where people could check them, or isn't this necessary?

See https://github.com/ntop/nDPI/blob/dev/doc/flow_risks.rst

jasonish · 2024-10-10T14:45:09Z

src/detect-ndpi-protocol.c

+        SCLogDebug("ndpi protocol match on protocol = %u.%u (match %u)",
+                f->detected_l7_protocol.app_protocol, f->detected_l7_protocol.master_protocol,
+                data->l7_protocol);


Compile error here:

detect-ndpi-protocol.c: In function ‘DetectnDPIProtocolPacketMatch’: detect-ndpi-protocol.c:80:40: error: ‘ndpi_protocol’ {aka ‘const struct ndpi_proto’} has no member named ‘app_protocol’ 80 | f->detected_l7_protocol.app_protocol, f->detected_l7_protocol.master_protocol, | ^

jasonish · 2024-10-10T14:46:34Z

src/detect-ndpi-risk.c

+/**********************************Unittests***********************************/
+
+#ifdef UNITTESTS
+


These unit tests below also fail to compile.

jasonish

@lucaderi @cardigliano Can we get a rebase that builds against current nDPI, or a specific version of nDPI?

jasonish · 2024-10-10T14:55:38Z

Linker error.. When I build Suricata with ./configure --enable-ndpi --with-ndpi=/home/jason/src/nDPI, it fails to link:

/usr/bin/ld: /home/jason/src/nDPI/src/lib/libndpi.a(ndpi_main.o): in function `ndpi_handle_rule.isra.0':
ndpi_main.c:(.text+0xaef3): undefined reference to `nbpf_parse'
/usr/bin/ld: /home/jason/src/nDPI/src/lib/libndpi.a(ndpi_main.o): in function `ndpi_exit_detection_module':
ndpi_main.c:(.text+0x11d99): undefined reference to `nbpf_free'
/usr/bin/ld: /home/jason/src/nDPI/src/lib/libndpi.a(ndpi_main.o): in function `ndpi_detection_process_packet':
ndpi_main.c:(.text+0x14d58): undefined reference to `nbpf_match'

this appears to be linking against the .a static library. Probably because I use --disable-shared. It works against the shared library. Is there a known reason why?

lucaderi · 2024-10-10T19:20:56Z

I believe you have built nDPI with nBPF support and the nBPF library was not used during linking. I think you have /home/jason/PF_RING/userland/nbpf/libnbpf.a but that was not used during linking.
Just delete libnbpf.a and rebuild nDPi and it will work

jasonish · 2024-10-11T20:32:00Z

@lucaderi @cardigliano Its still needs much review, but here's what I'm thinking for the support required for nDPI to be a plugin, including a stub plugin that shows off the call backs and user storage for threads and flows.

#11944

jasonish · 2024-10-11T20:32:30Z

I believe you have built nDPI with nBPF support and the nBPF library was not used during linking. I think you have /home/jason/PF_RING/userland/nbpf/libnbpf.a but that was not used during linking. Just delete libnbpf.a and rebuild nDPi and it will work

Yeah, it picked up some older pfring install from /usr/local. Thanks.

github-actions · 2024-11-04T07:51:12Z

NOTE: This PR may contain new authors.

cardigliano · 2024-11-04T09:10:24Z

Closing as this has been reworked as a plugin

lucaderi and others added 3 commits August 28, 2024 17:01

Add initial nDPI integration

c8d659f

Add configure --enable-ndpi and --with-ndpi params Add nDPI metadata export in alerts/netflow (Eve JSON) Add ndpi-protocol signature match Add ndpi-risk signature match

Format ndpi patch with clang-format

c9262c4

Add ndpi-protocol and ndpi-risk keywords documentation

d8e237d

cardigliano requested review from jufajardini, victorjulien and a team as code owners August 28, 2024 16:26

jasonish added the needs ticket Needs (link to) redmine ticket label Aug 28, 2024

jasonish removed the needs ticket Needs (link to) redmine ticket label Aug 29, 2024

jufajardini added the decision-required Waiting on deliberation from the team label Aug 30, 2024

catenacyber reviewed Sep 1, 2024

View reviewed changes

jasonish requested changes Sep 6, 2024

View reviewed changes

jufajardini reviewed Sep 30, 2024

View reviewed changes

jasonish reviewed Oct 10, 2024

View reviewed changes

jasonish requested changes Oct 10, 2024

View reviewed changes

jasonish mentioned this pull request Oct 10, 2024

[draft] flow callbacks for ndpi - v1 #11935

Closed

Fix unit tests in detect-ndpi-risk

de33b11

Merge branch 'OISF:master' into ndpi-integration-v0

ca32564

cardigliano closed this Nov 4, 2024

		/********************************Unittests*********************************/

		#ifdef UNITTESTS

Add ndpi integration #11671

Add ndpi integration #11671

Conversation

cardigliano commented Aug 28, 2024

github-actions bot commented Aug 28, 2024

codecov bot commented Aug 28, 2024 • edited Loading

Codecov Report

jasonish commented Aug 28, 2024

cardigliano commented Aug 29, 2024

catenacyber commented Sep 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaderi commented Sep 4, 2024

jasonish left a comment

Choose a reason for hiding this comment

lucaderi commented Sep 28, 2024

jufajardini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonish left a comment

Choose a reason for hiding this comment

jasonish commented Oct 10, 2024

lucaderi commented Oct 10, 2024

jasonish commented Oct 11, 2024

jasonish commented Oct 11, 2024

github-actions bot commented Nov 4, 2024

cardigliano commented Nov 4, 2024

codecov bot commented Aug 28, 2024 •

edited

Loading