vlan: add vlan.id keyword - v3 #12273

AkakiAlice · 2024-12-12T23:35:28Z

Ticket: #1065

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

Contribution style:

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

I have updated the User Guide (in doc/userguide/) to reflect the changes made
I have updated the JSON schema (in etc/schema.json) to reflect all logging changes
(including schema descriptions)
I have created a ticket at
https://redmine.openinfosecfoundation.org/projects/suricata/issues

Link to ticket: https://redmine.openinfosecfoundation.org/issues/1065

Describe changes:

Introduce vlan.id keyword

Provide values to any of the below to override the defaults.

To use an LibHTP, Suricata-Verify or Suricata-Update pull request,
link to the pull request in the respective _BRANCH variable.
Leave unused overrides blank or remove.

SV_BRANCH= OISF/suricata-verify#2183
Previous PR= #12103

Ticket: OISF#1065

victorjulien · 2024-12-13T11:11:56Z

doc/userguide/rules/vlan-id.rst

+and filter network packets based on Virtual Local Area Network IDs. By default,
+it matches all VLAN IDs. However, if a specific layer is defined, it will only match that layer.
+
+vlan.id uses :ref:`unsigned 32-bit integer <rules-integer-keywords>`.


why 32 bit? Iirc we can have a 12bit number max, so should we use u16?

ah I see the code uses u16

indeed, doc should state u16 as used in the code

Should we also mention here the max value, and the specification that defines that?

catenacyber

Thanks for the work Alice

CI : red, could you investigate ?
Code : looking into it now
Commits segmentation : ok
Commit messages : title could be detect: add vlan.id keyword
Git ID set : looks fine for me
CLA : you already contributed
Doc update : good, some remark inline
Redmine ticket : ok
Rustfmt : looks ok for vlan_id.rs
Tests : nice, thanks, added a remark there
Dependencies added: none

catenacyber · 2024-12-13T16:16:44Z

doc/userguide/rules/vlan-id.rst

+
+Suricata has a ``vlan.id`` keyword that can be used in signatures to identify
+and filter network packets based on Virtual Local Area Network IDs. By default,
+it matches all VLAN IDs. However, if a specific layer is defined, it will only match that layer.


you mean all layers if a packet has multiple vlan layers

catenacyber · 2024-12-13T16:17:19Z

rust/src/detect/error.rs

@@ -29,6 +29,8 @@ pub enum RuleParseError<I> {
    InvalidTransformBase64(String),
    InvalidByteExtract(String),

+    InvalidGeneric(String),


can be removed

catenacyber · 2024-12-13T16:40:09Z

src/detect-vlan-id.c

+    const PrefilterPacketHeaderCtx *ctx = pectx;
+
+    for (int i = 0; i < p->vlan_idx; i++) {
+        if (p->vlan_id[i] == ctx->v1.u16[0] && (ctx->v1.u8[0] == 0 || ctx->v1.u8[0] - 1 == i))


This function should use some more similar logic as DetectVlanIdMatch with DetectU16Match...

catenacyber · 2024-12-13T16:40:41Z

src/detect-vlan-id.c

+static void DetectVlanIdFree(DetectEngineCtx *de_ctx, void *ptr)
+{
+    DetectVlanIdData *data = ptr;
+    SCFree(data);


I am afraid that we have to do the free in the rust side, so we need to call a rust function here

catenacyber · 2024-12-13T16:41:18Z

src/detect-vlan-id.c

+    }
+
+    const DetectVlanIdData *vdata = (const DetectVlanIdData *)ctx;
+    if (vdata->layer == INT8_MIN) {


We should name this constant INT8_MIN with some name containing vlan

catenacyber · 2024-12-13T16:41:30Z

src/detect-vlan-id.c

+        }
+    } else {
+        if (vdata->layer < 0) {
+            if (((int16_t)p->vlan_idx) + vdata->layer < 0) {


maybe add some comment here

catenacyber · 2024-12-13T16:42:20Z

rust/src/detect/vlan_id.rs

+    if du16.is_err() {
+        return None;
+    }
+    let du16 = du16.unwrap().1;


@jasonish do you have proposals for better rust style here ?

catenacyber · 2024-12-13T16:43:32Z

doc/userguide/rules/vlan-id.rst

+
+ alert ip any any -> any any (msg:"Vlan ID is equal to 400"; vlan.id:400,-1; sid:1;)
+
+In this example, we use the negative value -1 to represent the last layer of the VLAN IDs.


@jufajardini what do you think about the doc ?

A bi nitty here, I'd suggest:

Suggested change

In this example, we use the negative value -1 to represent the last layer of the VLAN IDs.

In the last example, we use the negative value -1 to represent the last layer of the VLAN IDs.

It's probably also a good idea to mention this in the description, not just here when explaining the example.

@jufajardini what do you think about the doc ?

It's looking good :) I've made some suggestions, but we're going on the right direction /o/

Should we add if vlan.id supports prefiltering or not?

jufajardini

Shared some comments to help with the docs, mostly. ^^

We're missing adding the new file to doc/userguide/rules/index.rst, too, otherwise it won't be added, and the docs won't be generated, throwing an error.

jufajardini · 2024-12-13T18:51:02Z

doc/userguide/rules/vlan-id.rst

@@ -0,0 +1,32 @@
+Vlan.id Keyword


Considering what was brought up in the call we had:

Thinking about the structure we have for other keywords, I do think we should prepare the doc for the possibility of other vlan keywords being added. I'll point out in the files the changes needed for that.

That said, My suggestion is that we name the file vlan-keywords - and then the first title in this file would be VLAN Keywords -, and then have a section vlan.id for this description of the keyword itself.

jufajardini · 2024-12-13T18:57:28Z

doc/userguide/rules/vlan-id.rst

+
+ vlan.id: [op]id[,layer];
+
+The id can be matched exactly, or compared using the _op_ setting::


nit: for bold, you can use **, for italic, *, and `` for inline code

Suggested change

The id can be matched exactly, or compared using the _op_ setting::

The id can be matched exactly, or compared using the ``op`` setting::

jufajardini · 2024-12-13T19:04:22Z

doc/userguide/rules/vlan-id.rst

+
+ alert ip any any -> any any (msg:"Vlan ID is equal to 300"; vlan.id:300; sid:1;)
+
+::
+
+ alert ip any any -> any any (msg:"Vlan ID is equal to 300"; vlan.id:300,1; sid:1;)
+
+::
+
+ alert ip any any -> any any (msg:"Vlan ID is equal to 400"; vlan.id:400,-1; sid:1;)
+


We have a few directives that help with formatting rule examples. You can add them in the first lines of your document, and then benefit from some neat options.

More on this: https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html#rule-examples (and then you can take advantage of that to highlight your keyword on the examples, for instance ) usage example in our docs: https://raw.githubusercontent.com/OISF/suricata/refs/heads/master/doc/userguide/rules/http-keywords.rst

jufajardini · 2024-12-13T19:05:33Z

doc/userguide/rules/vlan-id.rst

+and filter network packets based on Virtual Local Area Network IDs. By default,
+it matches all VLAN IDs. However, if a specific layer is defined, it will only match that layer.
+
+vlan.id uses :ref:`unsigned 32-bit integer <rules-integer-keywords>`.


Should we also mention here the max value, and the specification that defines that?

jufajardini · 2024-12-13T19:06:35Z

doc/userguide/rules/vlan-id.rst

+
+ alert ip any any -> any any (msg:"Vlan ID is equal to 400"; vlan.id:400,-1; sid:1;)
+
+In this example, we use the negative value -1 to represent the last layer of the VLAN IDs.


A bi nitty here, I'd suggest:

Suggested change

In this example, we use the negative value -1 to represent the last layer of the VLAN IDs.

In the last example, we use the negative value -1 to represent the last layer of the VLAN IDs.

It's probably also a good idea to mention this in the description, not just here when explaining the example.

jufajardini · 2024-12-13T19:08:29Z

doc/userguide/rules/vlan-id.rst

+
+ alert ip any any -> any any (msg:"Vlan ID is equal to 400"; vlan.id:400,-1; sid:1;)
+
+In this example, we use the negative value -1 to represent the last layer of the VLAN IDs.


@jufajardini what do you think about the doc ?

It's looking good :) I've made some suggestions, but we're going on the right direction /o/

Should we add if vlan.id supports prefiltering or not?

jufajardini · 2024-12-13T19:12:45Z

src/detect-vlan-id.c

+{
+    sigmatch_table[DETECT_VLAN_ID].name = "vlan.id";
+    sigmatch_table[DETECT_VLAN_ID].desc = "match vlan id";
+    sigmatch_table[DETECT_VLAN_ID].url = "/rules/vlan-id.html";


If we agree to having a file more generic for vlan keywords, then this one becomes:

Suggested change

sigmatch_table[DETECT_VLAN_ID].url = "/rules/vlan-id.html";

sigmatch_table[DETECT_VLAN_ID].url = "/rules/vlan-keywords.html#vlan-id";

catenacyber · 2024-12-13T19:31:22Z

rust/src/detect/vlan_id.rs

+        return None;
+    }
+    let layer = if parts.len() == 2 {
+        i8::from_str(parts[1])


Suggestion for even more expressivity : add support vlan.id: >100,all which matches only if all vlan layers match

vlan: add vlan.id keyword

97d3890

Ticket: OISF#1065

AkakiAlice requested review from jasonish, jufajardini and victorjulien as code owners December 12, 2024 23:35

AkakiAlice mentioned this pull request Dec 12, 2024

test: add test for vlan.id - v5 OISF/suricata-verify#2183

Closed

victorjulien reviewed Dec 13, 2024

View reviewed changes

catenacyber requested changes Dec 13, 2024

View reviewed changes

catenacyber reviewed Dec 13, 2024

View reviewed changes

jufajardini requested changes Dec 13, 2024

View reviewed changes

catenacyber reviewed Dec 13, 2024

View reviewed changes

AkakiAlice mentioned this pull request Dec 13, 2024

detect: add vlan.id keyword - v4 #12285

Open

5 tasks

AkakiAlice closed this Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vlan: add vlan.id keyword - v3 #12273

vlan: add vlan.id keyword - v3 #12273

AkakiAlice commented Dec 12, 2024

victorjulien Dec 13, 2024

victorjulien Dec 13, 2024

catenacyber Dec 13, 2024

jufajardini Dec 13, 2024

catenacyber left a comment

catenacyber Dec 13, 2024

catenacyber Dec 13, 2024

catenacyber Dec 13, 2024

catenacyber Dec 13, 2024

catenacyber Dec 13, 2024

catenacyber Dec 13, 2024

catenacyber Dec 13, 2024

catenacyber Dec 13, 2024

jufajardini Dec 13, 2024

jufajardini Dec 13, 2024

jufajardini left a comment

jufajardini Dec 13, 2024

jufajardini Dec 13, 2024

jufajardini Dec 13, 2024

jufajardini Dec 13, 2024

jufajardini Dec 13, 2024

jufajardini Dec 13, 2024

jufajardini Dec 13, 2024

catenacyber Dec 13, 2024


		alert ip any any -> any any (msg:"Vlan ID is equal to 400"; vlan.id:400,-1; sid:1;)

		In this example, we use the negative value -1 to represent the last layer of the VLAN IDs.

	In this example, we use the negative value -1 to represent the last layer of the VLAN IDs.
	In the last example, we use the negative value -1 to represent the last layer of the VLAN IDs.


		vlan.id: [op]id[,layer];

		The id can be matched exactly, or compared using the _op_ setting::

	sigmatch_table[DETECT_VLAN_ID].url = "/rules/vlan-id.html";
	sigmatch_table[DETECT_VLAN_ID].url = "/rules/vlan-keywords.html#vlan-id";

vlan: add vlan.id keyword - v3 #12273

vlan: add vlan.id keyword - v3 #12273

Conversation

AkakiAlice commented Dec 12, 2024

Contribution style:

Our Contribution agreements:

Changes (if applicable):

Provide values to any of the below to override the defaults.

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

catenacyber 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment