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

detect: add vlan.id keyword - v6 #12301

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions doc/userguide/rules/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,4 @@ Suricata Rules
differences-from-snort
multi-buffer-matching
tag
vlan-keywords
87 changes: 87 additions & 0 deletions doc/userguide/rules/vlan-keywords.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
VLAN Keywords
=============

.. role:: example-rule-action
.. role:: example-rule-header
.. role:: example-rule-options
.. role:: example-rule-emphasis

vlan.id
-------

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 layers if a packet contains multiple VLAN layers. However, if a
specific layer is defined, it will only match that layer.

This keyword supports ``all`` and ``count`` as arguments for ``layer``.
``all`` matches only if all VLAN layers match and ``count`` matches based on the number of layers.

Id values for vlan.id keyword:

======== ================================================
Value Description
======== ================================================
1 - 4094 Valid range for vlan id
0 - 3 Valid range of number of layers (with ``count``)
======== ================================================
Comment on lines +20 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use the .. table:: Sphinx directive and have neat table titles, here and for the following one:

Suggested change
Id values for vlan.id keyword:
======== ================================================
Value Description
======== ================================================
1 - 4094 Valid range for vlan id
0 - 3 Valid range of number of layers (with ``count``)
======== ================================================
.. table:: **Id values for vlan.id keyword**
======== ================================================
Value Description
======== ================================================
1 - 4094 Valid range for vlan id
0 - 3 Valid range of number of layers (with ``count``)
======== ================================================


Layer values for vlan.id keyword:

=============== ================================================
Value Description
=============== ================================================
[default] Match all layers
0 - 2 Match specific layer
``-3`` - ``-1`` Match specific layer with back to front indexing
all Match only if all layers match
count Match on the number of layers
=============== ================================================

vlan.id uses :ref:`unsigned 16-bit integer <rules-integer-keywords>`.

Syntax::

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

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

Choose a reason for hiding this comment

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

Suggested change
vlan.id:<300,0 # smaller than 300 at layer 0
vlan.id:<300,0 # less than 300 at layer 0

vlan.id:>=200,1 # greater or equal than 200 at layer 1
Copy link
Contributor

Choose a reason for hiding this comment

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

nit (missed this the other review x_x ):

Suggested change
vlan.id:>=200,1 # greater or equal than 200 at layer 1
vlan.id:>=200,1 # greater than or equal to 200 at layer 1


Example of a signature that would alert if any of the VLAN IDs is equal to 300:

.. container:: example-rule

alert ip any any -> any any (msg:"Vlan ID is equal to 300"; :example-rule-emphasis:`vlan.id:300;` sid:1;)

Example of a signature that would alert if the VLAN ID at layer 1 is equal to 300:

.. container:: example-rule

alert ip any any -> any any (msg:"Vlan ID is equal to 300 at layer 1"; :example-rule-emphasis:`vlan.id:300,1;` sid:1;)

Example of a signature that would alert if the VLAN ID at the last layer is equal to 400:

.. container:: example-rule

alert ip any any -> any any (msg:"Vlan ID is equal to 400 at the last layer"; :example-rule-emphasis:`vlan.id:400,-1;` sid:1;)

Example of a signature that would alert only if all the VLAN IDs are greater than 100:

.. container:: example-rule

alert ip any any -> any any (msg:"All Vlan IDs are greater than 100"; :example-rule-emphasis:`vlan.id:>100,all;` sid:1;)

Example of a signature that would alert if the packet has 3 VLAN layers:

.. container:: example-rule

alert ip any any -> any any (msg:"Packet has 3 VLAN layers"; :example-rule-emphasis:`vlan.id:3,count;` sid:1;)

It is also possible to use the vlan.id content as a fast_pattern by using the ``prefilter`` keyword, as shown in the following example.

.. container:: example-rule

alert ip any any -> any any (msg:"Vlan ID is equal to 200 at layer 1"; :example-rule-emphasis:`vlan.id:200,1; prefilter;` sid:1;)
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
199 changes: 199 additions & 0 deletions rust/src/detect/vlan_id.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
/* 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;

pub const DETECT_VLAN_ID_ANY: i8 = i8::MIN;
pub const DETECT_VLAN_ID_ALL: i8 = i8::MAX;
pub const DETECT_VLAN_ID_COUNT: i8 = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to transfer src/decode-vlan.h:#define VLAN_MAX_LAYERS 3 here and use VLAN_MAX_LAYERS instead of hardcoded 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I defined VLAN_MAX_LAYERS in the vlan_id.rs file like this: pub const VLAN_MAX_LAYERS: i8 = 3;
but I get this error message:

decode.h:503:22: error: ‘VLAN_MAX_LAYERS’ undeclared here (not in a function)
503 |     uint16_t vlan_id[VLAN_MAX_LAYERS];

And when I include rust.h in the decode.h file, this first error goes away, but I get several error messages similar to this one:

util-file.h:214:39: error: unknown type name ‘Flow’
214 | void FileDisableStoringForTransaction(Flow *f, const uint8_t direction, void *tx, uint64_t tx_id);

Copy link
Member

Choose a reason for hiding this comment

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

I think you need to maintain 2 copies of the constant. One on the C side and the other one on the Rust side. The important thing is to avoid using magic numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do not spend too much time on it if it gets too complex with include order.
Just do as Shivani said, and add some comments


#[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]).ok()?.1;
if parts.len() > 2 {
return None;
}
if du16.arg1 >= 0xFFF {
AkakiAlice marked this conversation as resolved.
Show resolved Hide resolved
// vlan id is encoded on 12 bits
return None;
}
let layer = if parts.len() == 2 {
if parts[1] == "all" {
Ok(DETECT_VLAN_ID_ALL)
} else if parts[1] == "count" {
Ok(DETECT_VLAN_ID_COUNT)
} else {
i8::from_str(parts[1])
AkakiAlice marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
Ok(DETECT_VLAN_ID_ANY)
}
.ok()?;
if layer == DETECT_VLAN_ID_COUNT && !(0..=3).contains(&du16.arg1) {
return None;
AkakiAlice marked this conversation as resolved.
Show resolved Hide resolved
}
if parts.len() == 2
&& layer != DETECT_VLAN_ID_ALL
&& layer != DETECT_VLAN_ID_COUNT
&& !(-3..=2).contains(&layer)
{
return None;
AkakiAlice marked this conversation as resolved.
Show resolved Hide resolved
}
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();
}

#[no_mangle]
pub unsafe extern "C" fn rs_detect_vlan_id_free(ctx: &mut DetectVlanIdData) {
// Just unbox...
std::mem::drop(Box::from_raw(ctx));
}

#[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: DETECT_VLAN_ID_ANY
}
);
assert_eq!(
detect_parse_vlan_id("300,all").unwrap(),
DetectVlanIdData {
du16: DetectUintData {
arg1: 300,
arg2: 0,
mode: DetectUintMode::DetectUintModeEqual,
},
layer: DETECT_VLAN_ID_ALL
}
);
assert_eq!(
detect_parse_vlan_id("3,count").unwrap(),
DetectVlanIdData {
du16: DetectUintData {
arg1: 3,
arg2: 0,
mode: DetectUintMode::DetectUintModeEqual,
},
layer: DETECT_VLAN_ID_COUNT
}
);
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());
Copy link
Contributor

Choose a reason for hiding this comment

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

test 1,2,3 errors ?

Copy link
Contributor

Choose a reason for hiding this comment

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

5, count and also 123,-4 are error cases not tested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by "test 1,2,3 errors" ?

Copy link
Member

Choose a reason for hiding this comment

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

pass 1,2,3 as args to the fn detect_parse_vlan_id and see if that errors out

Copy link
Contributor

Choose a reason for hiding this comment

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

yes assert!(detect_parse_vlan_id("1,2,3").is_none()); because parts.len() > 2

}
}
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
Loading
Loading