Skip to content

Dataset match subdomain/v2 - #15073

Closed
antoineaboufayssal wants to merge 2 commits into
OISF:mainfrom
antoineaboufayssal:dataset-match-subdomain/v2
Closed

Dataset match subdomain/v2#15073
antoineaboufayssal wants to merge 2 commits into
OISF:mainfrom
antoineaboufayssal:dataset-match-subdomain/v2

Conversation

@antoineaboufayssal

Copy link
Copy Markdown
Contributor

Previously #15058

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Describe changes:

  • add match subdomain option for datasets

SV_BRANCH=OISF/suricata-verify#2979

Ticket: 8385

Add a new match subdomain option that enables blocking a domain and all its subdomains using datasets.
@codecov

codecov Bot commented Mar 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.08696% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.60%. Comparing base (6587e36) to head (4bff9c5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15073   +/-   ##
=======================================
  Coverage   82.59%   82.60%           
=======================================
  Files         990      990           
  Lines      271761   271806   +45     
=======================================
+ Hits       224465   224522   +57     
+ Misses      47296    47284   -12     
Flag Coverage Δ
fuzzcorpus 61.02% <17.39%> (-0.01%) ⬇️
livemode 18.35% <10.86%> (-0.02%) ⬇️
netns 18.35% <0.00%> (-0.02%) ⬇️
pcap 45.22% <0.00%> (-0.04%) ⬇️
suricata-verify 66.13% <76.08%> (-0.01%) ⬇️
unittests 58.82% <0.00%> (-0.02%) ⬇️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@catenacyber catenacyber left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the work

CI : ✅
Code : good
Commits segmentation : I would squash but ok
Commit messages : nice
Git ID set : looks fine for me
CLA : you already contributed
Doc update : ok
Redmine ticket : ok
Rustfmt : no rust
Tests : cool
Dependencies added: none

Comment thread src/detect-dataset.c
#define DETECT_DATASET_CMD_ISNOTSET 2
#define DETECT_DATASET_CMD_ISSET 3

#define DATASET_SUBDOMAIN_MAX_LOOKUPS 126

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, where did you find this constant ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A valid domain name is limited to 253 bytes (whether it's dns, http host, tls sni, ...), giving at most 126 labels (1 char + 1 dot per label). Buffers like http.host are not length enforced, but anything beyond that limit is malformed or malicious and shouldn't match the dataset entry.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool

A valid domain name is limited to 253 bytes

Where did you find this ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

RFC 1035 defines it for DNS. (it's actually 255 bytes but a length byte and a null terminator byte are needed, hence the 253)

So maybe my statement up there wasn't 100% accurate :) While an http.host can exceed the 253 limit, it would not be a "globally resolvable domain". Perhaps used for an internal/private network?

@antoineaboufayssal

Copy link
Copy Markdown
Contributor Author

@victorjulien @catenacyber any updates on this?

also would it be possible to backport it?

@catenacyber

Copy link
Copy Markdown
Contributor

Thanks for your work, it is fine for me. We are waiting on Victor's review...

@victorjulien victorjulien self-assigned this May 21, 2026
@victorjulien victorjulien added this to the 9.0 milestone Jun 10, 2026

.. container:: example-rule

reject dns any any -> any any (msg:"Blocked domain"; dns.query; dotprefix; dataset:isset,blocked-domains, type string, match subdomain, load blocked-domains.lst; sid:8000003; rev:1;)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is leading to a CI failure for Check rules doc: Error: detect-dataset: failed to set up dataset 'blocked-domains'. [DetectDatasetSetup:detect-dataset.c:640]

https://github.com/OISF/suricata/actions/runs/27315514634/job/80695160734?pr=15601#step:22:12

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it because of load blocked-domains.lst? It's trying to find the file relative to the path of the rule file itself. Examples 1 and 2 use state instead of load.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this just may show a limitation of the check. I don't think it is aware of how datasets needs a file to load here. I'd say the check is wrong, not this doc example.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here is a fix #15605

@victorjulien

Copy link
Copy Markdown
Member

Merged in #15601, thanks!

@antoineaboufayssal
antoineaboufayssal deleted the dataset-match-subdomain/v2 branch June 12, 2026 15:54
@jufajardini

Copy link
Copy Markdown
Contributor

@victorjulien @catenacyber any updates on this?

also would it be possible to backport it?

Hi @antoineaboufayssal if you could prepare a backport patch for this so we can review and test, we'll consider backporting it :)

@antoineaboufayssal

Copy link
Copy Markdown
Contributor Author

Hi @antoineaboufayssal if you could prepare a backport patch for this so we can review and test, we'll consider backporting it :)

done! #16024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants