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.

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

vlan: add vlan.id keyword - v3 #12273

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions doc/userguide/rules/vlan-id.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
Vlan.id Keyword
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

===============

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean all layers if a packet has multiple vlan layers


vlan.id uses :ref:`unsigned 32-bit integer <rules-integer-keywords>`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see the code uses u16

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, doc should state u16 as used in the code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Syntax::

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::


vlan.id:300 # exactly 300
vlan.id:<300,0 # smaller than 300 at layer 0
vlan.id:>=200,1 # greater or equal than 200 at layer 1

Signature examples::

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;)

Comment on lines +21 to +31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jufajardini what do you think about the doc ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

2 changes: 2 additions & 0 deletions rust/src/detect/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ pub enum RuleParseError<I> {
InvalidTransformBase64(String),
InvalidByteExtract(String),

InvalidGeneric(String),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be removed


Nom(I, ErrorKind),
}
impl<I> ParseError<I> for RuleParseError<I> {
Expand Down
1 change: 1 addition & 0 deletions rust/src/detect/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ pub mod iprep;
pub mod parser;
pub mod requires;
pub mod stream_size;
pub mod vlan_id;
pub mod transform_base64;
pub mod transforms;
pub mod uint;
Expand Down
161 changes: 161 additions & 0 deletions rust/src/detect/vlan_id.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
/* Copyright (C) 2024 Open Information Security Foundation
*
* You can copy, redistribute or modify this Program under the terms of
* the GNU General Public License version 2 as published by the Free
* Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* version 2 along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/

use super::uint::{detect_parse_uint, DetectUintData};
use std::ffi::CStr;
use std::str::FromStr;

#[repr(C)]
#[derive(Debug, PartialEq)]
pub struct DetectVlanIdData {
pub du16: DetectUintData<u16>,
pub layer: i8,
}

pub fn detect_parse_vlan_id(s: &str) -> Option<DetectVlanIdData> {
let parts: Vec<&str> = s.split(',').collect();
let du16 = detect_parse_uint(parts[0]);
if du16.is_err() {
return None;
}
let du16 = du16.unwrap().1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if parts.len() > 2 {
return None;
}
if du16.arg1 >= 0xFFF {
// vlan id is encoded on 12 bits
return None;
}
let layer = if parts.len() == 2 {
i8::from_str(parts[1])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
Ok(i8::MIN)
};
if layer.is_err() {
return None;
}
let layer = layer.unwrap();
if parts.len() == 2 && (layer < -3 || layer > 2) {
return None;
}
return Some(DetectVlanIdData { du16, layer });
}

#[no_mangle]
pub unsafe extern "C" fn rs_detect_vlan_id_parse(
ustr: *const std::os::raw::c_char,
) -> *mut DetectVlanIdData {
let ft_name: &CStr = CStr::from_ptr(ustr); //unsafe
if let Ok(s) = ft_name.to_str() {
if let Some(ctx) = detect_parse_vlan_id(s) {
let boxed = Box::new(ctx);
return Box::into_raw(boxed) as *mut _;
}
}
return std::ptr::null_mut();
}

#[cfg(test)]
mod test {
use super::*;
use crate::detect::uint::DetectUintMode;

#[test]
fn test_detect_parse_vlan_id() {
assert_eq!(
detect_parse_vlan_id("300").unwrap(),
DetectVlanIdData {
du16: DetectUintData {
arg1: 300,
arg2: 0,
mode: DetectUintMode::DetectUintModeEqual,
},
layer: i8::MIN
}
);
assert_eq!(
detect_parse_vlan_id("200,1").unwrap(),
DetectVlanIdData {
du16: DetectUintData {
arg1: 200,
arg2: 0,
mode: DetectUintMode::DetectUintModeEqual,
},
layer: 1
}
);
assert_eq!(
detect_parse_vlan_id("200,-1").unwrap(),
DetectVlanIdData {
du16: DetectUintData {
arg1: 200,
arg2: 0,
mode: DetectUintMode::DetectUintModeEqual,
},
layer: -1
}
);
assert_eq!(
detect_parse_vlan_id("!200,2").unwrap(),
DetectVlanIdData {
du16: DetectUintData {
arg1: 200,
arg2: 0,
mode: DetectUintMode::DetectUintModeNe,
},
layer: 2
}
);
assert_eq!(
detect_parse_vlan_id(">200,2").unwrap(),
DetectVlanIdData {
du16: DetectUintData {
arg1: 200,
arg2: 0,
mode: DetectUintMode::DetectUintModeGt,
},
layer: 2
}
);
assert_eq!(
detect_parse_vlan_id("200-300,0").unwrap(),
DetectVlanIdData {
du16: DetectUintData {
arg1: 200,
arg2: 300,
mode: DetectUintMode::DetectUintModeRange,
},
layer: 0
}
);
assert_eq!(
detect_parse_vlan_id("0xC8,2").unwrap(),
DetectVlanIdData {
du16: DetectUintData {
arg1: 200,
arg2: 0,
mode: DetectUintMode::DetectUintModeEqual,
},
layer: 2
}
);
assert!(detect_parse_vlan_id("200abc").is_none());
assert!(detect_parse_vlan_id("4096").is_none());
assert!(detect_parse_vlan_id("600,abc").is_none());
assert!(detect_parse_vlan_id("600,100").is_none());
}
}
2 changes: 2 additions & 0 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ noinst_HEADERS = \
detect-urilen.h \
detect-within.h \
detect-xbits.h \
detect-vlan-id.h \
device-storage.h \
feature.h \
flow-bit.h \
Expand Down Expand Up @@ -876,6 +877,7 @@ libsuricata_c_a_SOURCES = \
detect-urilen.c \
detect-within.c \
detect-xbits.c \
detect-vlan-id.c \
device-storage.c \
feature.c \
flow-bit.c \
Expand Down
3 changes: 3 additions & 0 deletions src/detect-engine-register.c
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@
#include "detect-ike-nonce-payload-length.h"
#include "detect-ike-nonce-payload.h"
#include "detect-ike-key-exchange-payload.h"
#include "detect-vlan-id.h"

#include "action-globals.h"
#include "tm-threads.h"
Expand Down Expand Up @@ -699,6 +700,8 @@ void SigTableSetup(void)

DetectFileHandlerRegister();

DetectVlanIdRegister();

ScDetectSNMPRegister();
ScDetectDHCPRegister();
ScDetectWebsocketRegister();
Expand Down
2 changes: 2 additions & 0 deletions src/detect-engine-register.h
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,8 @@ enum DetectKeywordId {

DETECT_AL_JA4_HASH,

DETECT_VLAN_ID,

/* make sure this stays last */
DETECT_TBLSIZE_STATIC,
};
Expand Down
123 changes: 123 additions & 0 deletions src/detect-vlan-id.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/* Copyright (C) 2024 Open Information Security Foundation
*
* You can copy, redistribute or modify this Program under the terms of
* the GNU General Public License version 2 as published by the Free
* Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* version 2 along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/

#include "detect-vlan-id.h"
#include "detect-engine-uint.h"
#include "detect-parse.h"

static int DetectVlanIdMatch(
DetectEngineThreadCtx *det_ctx, Packet *p, const Signature *s, const SigMatchCtx *ctx)
{
if (p->vlan_idx == 0) {
return 0;
}

const DetectVlanIdData *vdata = (const DetectVlanIdData *)ctx;
if (vdata->layer == INT8_MIN) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should name this constant INT8_MIN with some name containing vlan

for (int i = 0; i < p->vlan_idx; i++) {
if (DetectU16Match(p->vlan_id[i], &vdata->du16)) {
return 1;
}
}
} else {
if (vdata->layer < 0) {
if (((int16_t)p->vlan_idx) + vdata->layer < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add some comment here

return 0;
}
return DetectU16Match(p->vlan_id[p->vlan_idx + vdata->layer], &vdata->du16);
} else {
if (p->vlan_idx < vdata->layer) {
return 0;
}
return DetectU16Match(p->vlan_id[vdata->layer], &vdata->du16);
}
}
return 0;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

static int DetectVlanIdSetup(DetectEngineCtx *de_ctx, Signature *s, const char *rawstr)
{
DetectVlanIdData *vdata = rs_detect_vlan_id_parse(rawstr);
if (vdata == NULL) {
SCLogError("vlan id invalid %s", rawstr);
return -1;
}

if (SigMatchAppendSMToList(
de_ctx, s, DETECT_VLAN_ID, (SigMatchCtx *)vdata, DETECT_SM_LIST_MATCH) == NULL) {
DetectVlanIdFree(de_ctx, vdata);
return -1;
}
s->flags |= SIG_FLAG_REQUIRE_PACKET;

return 0;
}

static void PrefilterPacketVlanIdMatch(DetectEngineThreadCtx *det_ctx, Packet *p, const void *pectx)
{
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

PrefilterAddSids(&det_ctx->pmq, ctx->sigs_array, ctx->sigs_cnt);
}
}

static void PrefilterPacketVlanIdSet(PrefilterPacketHeaderValue *v, void *smctx)
{
const DetectVlanIdData *a = smctx;
v->u8[0] = a->du16.mode;
v->u8[1] = a->layer;
v->u16[2] = a->du16.arg1;
}

static bool PrefilterPacketVlanIdCompare(PrefilterPacketHeaderValue v, void *smctx)
{
const DetectVlanIdData *a = smctx;
if (v.u8[0] == a->du16.mode && v.u8[1] == a->layer && v.u16[2] == a->du16.arg1)
return true;
return false;
}

static int PrefilterSetupVlanId(DetectEngineCtx *de_ctx, SigGroupHead *sgh)
{
return PrefilterSetupPacketHeader(de_ctx, sgh, DETECT_VLAN_ID, SIG_MASK_REQUIRE_FLOW,
PrefilterPacketVlanIdSet, PrefilterPacketVlanIdCompare, PrefilterPacketVlanIdMatch);
}

static bool PrefilterVlanIdIsPrefilterable(const Signature *s)
{
return PrefilterIsPrefilterableById(s, DETECT_VLAN_ID);
}

void DetectVlanIdRegister(void)
{
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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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";

sigmatch_table[DETECT_VLAN_ID].Match = DetectVlanIdMatch;
sigmatch_table[DETECT_VLAN_ID].Setup = DetectVlanIdSetup;
sigmatch_table[DETECT_VLAN_ID].Free = DetectVlanIdFree;
sigmatch_table[DETECT_VLAN_ID].SupportsPrefilter = PrefilterVlanIdIsPrefilterable;
sigmatch_table[DETECT_VLAN_ID].SetupPrefilter = PrefilterSetupVlanId;
}
Loading
Loading