Skip to content

Add IPv6 CIDR support - #1757

Open
mkhludnev wants to merge 7 commits into
opensearch-project:mainfrom
mkhludnev:copilot/add-ipv6-support-to-sigma-rules
Open

Add IPv6 CIDR support#1757
mkhludnev wants to merge 7 commits into
opensearch-project:mainfrom
mkhludnev:copilot/add-ipv6-support-to-sigma-rules

Conversation

@mkhludnev

@mkhludnev mkhludnev commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown

This pull request extends CIDR expression support in the SigmaCIDRExpression class to include IPv6 addresses, in addition to IPv4. It also adds comprehensive tests to cover various IPv6 CIDR scenarios and improves error handling for invalid CIDR expressions.

Enhancements to CIDR expression support:

  • Added validation logic for IPv6 CIDR expressions in SigmaCIDRExpression, including checks for valid IPv6 format and prefix length (0–128).
  • Updated the constructor of SigmaCIDRExpression to accept valid IPv6 CIDR expressions and provide a unified error message for invalid CIDR input.
  • Imported necessary Java networking classes (Inet6Address, InetAddress, UnknownHostException) to support IPv6 validation.

Testing improvements:

  • Added new unit tests in SigmaCIDRModifierTests to verify correct handling of IPv6 CIDR expressions, loopback, addresses without prefix, and invalid prefix lengths.
  • Included SigmaTypeError import in test file to support new test cases.### Description
    [Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions

github-actions Bot commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit e626621)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@mkhludnev mkhludnev changed the title Add IPv6 CIDR support and remove obsolete Sigma classes Add IPv6 CIDR support Aug 3, 2026
@mkhludnev

mkhludnev commented Aug 3, 2026

Copy link
Copy Markdown
Author

brought from forum https://forum.opensearch.org/t/cidr-modifier-for-sigma-detection-rules-works-only-for-ipv4-addresses-not-for-ipv6-resulting-in-sigma-detection-error/28232

@mkhludnev
mkhludnev marked this pull request as ready for review August 3, 2026 23:23
@mkhludnev

mkhludnev commented Aug 3, 2026

Copy link
Copy Markdown
Author

Missing convert() Update for IPv6
The PR accepts IPv6 CIDR values, but the convert() method (not shown but referenced) was originally written for IPv4. If it performs IPv4-specific transformations (e.g., wildcard expansion), it will produce incorrect results for IPv6 inputs downstream.

nope. it just returns cidr

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 78bef3d

@github-actions

github-actions Bot commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to e626621

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent integer overflow parsing prefix

Integer.parseInt can throw NumberFormatException for prefix strings that match \d+
but overflow int (e.g., 192.168.1.0/9999999999999). Guard against this by catching
the exception or checking length before parsing to avoid an uncaught runtime
exception escaping isValidCidr.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [53-60]

+private static boolean isValidCidr(String cidr) {
+    if (cidr == null) {
+        return false;
+    }
 
+    // Intentional leniency: bare IPs without a prefix and CIDRs with host bits
+    // set are accepted and stored verbatim, matching the historical behavior.
+    String[] values = cidr.split("/", -1);
+    if (values.length > 2) {
+        return false;
+    }
 
+    String ip = values[0];
+    if (parseIpLiteral(ip) == null) {
+        return false;
+    }
+
Suggestion importance[1-10]: 6

__

Why: Valid concern: a very long numeric prefix matching \d+ would throw NumberFormatException from Integer.parseInt, escaping isValidCidr. However, the improved_code is identical to existing_code and does not actually apply the fix, reducing its usefulness.

Low
General
Reject non-canonical prefix formats

A prefix like /007 currently matches \d+ and parses to 7, which silently accepts
non-canonical CIDR notation. Consider rejecting leading zeros (except a bare 0) and
also constraining prefix digit length to avoid NumberFormatException on huge inputs.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [53-60]

 String prefixStr = values[1];
-if (!PREFIX_PATTERN.matcher(prefixStr).matches()) {
+if (!PREFIX_PATTERN.matcher(prefixStr).matches() || prefixStr.length() > 3
+        || (prefixStr.length() > 1 && prefixStr.charAt(0) == '0')) {
     return false;
 }
 
 int prefix = Integer.parseInt(prefixStr);
 int maxPrefix = ip.indexOf(':') < 0 ? 32 : 128;
 return prefix <= maxPrefix;
Suggestion importance[1-10]: 5

__

Why: Correctly identifies that overly long prefixes can cause NumberFormatException and adds a length guard. Rejecting leading zeros is a stylistic strictness change that may or may not be desired given the noted "intentional leniency."

Low

Previous suggestions

Suggestions up to commit f12210c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent numeric overflow on prefix parsing

The PREFIX_PATTERN accepts arbitrarily long numeric strings, which can cause
Integer.parseInt to throw NumberFormatException (unchecked) on overflow rather than
being rejected as an invalid CIDR. Restrict the pattern to a bounded length (e.g.,
1-3 digits) or catch NumberFormatException in isValidCidr to return false safely.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [15]

-private static final Pattern PREFIX_PATTERN = Pattern.compile("\\d+");
+private static final Pattern PREFIX_PATTERN = Pattern.compile("\\d{1,3}");
Suggestion importance[1-10]: 6

__

Why: Valid concern: a very long numeric string matching \d+ would cause Integer.parseInt to throw an unchecked NumberFormatException, escaping isValidCidr. Bounding the digits is a reasonable defensive fix, though impact is low as inputs are typically well-formed.

Low
General
Reject bracketed IPv6 literals

InetAddress.getByName on a bracketed IPv6 literal like [::1] will succeed, allowing
invalid CIDR inputs such as [::1]/128 to be accepted. Explicitly reject inputs
containing [ or ] (and other non-literal characters) before invoking getByName to
keep validation strict.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [71-77]

 if (ip.indexOf(':') >= 0) {
+    if (ip.indexOf('[') >= 0 || ip.indexOf(']') >= 0) {
+        return null;
+    }
     try {
         return Inet6Address.getByName(ip);
     } catch (UnknownHostException e) {
         return null;
     }
 }
Suggestion importance[1-10]: 4

__

Why: The concern is plausible but minor; bracketed IPv6 literals are unusual as CIDR inputs and this is more of a strictness improvement than a critical bug fix.

Low
Suggestions up to commit 04aebeb
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against numeric overflow on prefix

Integer.parseInt can throw NumberFormatException for very long numeric strings
(e.g., /99999999999999999) that still match PREFIX_PATTERN (\d+) but overflow int.
Wrap the parse in a try/catch or bound the length of prefixStr to avoid a runtime
exception leaking out of isValidCidr.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [53-59]

 String prefixStr = values[1];
 if (!PREFIX_PATTERN.matcher(prefixStr).matches()) {
     return false;
 }
 
-int prefix = Integer.parseInt(prefixStr);
+int prefix;
+try {
+    prefix = Integer.parseInt(prefixStr);
+} catch (NumberFormatException e) {
+    return false;
+}
 return prefix <= address.getAddress().length * 8;
Suggestion importance[1-10]: 7

__

Why: Valid concern: a long digit string matches \d+ but would overflow Integer.parseInt, causing an uncaught NumberFormatException to escape isValidCidr instead of returning a clean SigmaTypeError.

Medium
Reject empty IP literal explicitly

InetAddress.getByName("") returns the loopback address rather than throwing, so an
empty IP portion (e.g. input "/24") would be incorrectly accepted. Explicitly reject
empty ip before calling getByName to prevent this bypass.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [68-75]

+if (ip.isEmpty()) {
+    return null;
+}
 if (!IPV4_PATTERN.matcher(ip).matches() && ip.indexOf(':') < 0) {
     return null;
 }
 try {
     return InetAddress.getByName(ip);
 } catch (UnknownHostException e) {
     return null;
 }
Suggestion importance[1-10]: 6

__

Why: Correct observation that empty string passes IPV4_PATTERN check (fails) and lacks ':' (fails), so it returns null before getByName — actually the current guard already rejects empty strings since the IPv4 regex won't match and there's no ':'. The suggestion is defensive but not strictly necessary.

Low
Suggestions up to commit 3dfa4cb
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against numeric overflow on prefix

Integer.parseInt can throw NumberFormatException for very long numeric strings
(e.g., "9999999999") that match \d+ but exceed Integer.MAX_VALUE. Wrap the parse in
a try/catch or bound the length of prefixStr to avoid an unchecked exception
propagating out of isValidCidr.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [56-57]

-int prefix = Integer.parseInt(prefixStr);
+int prefix;
+try {
+    prefix = Integer.parseInt(prefixStr);
+} catch (NumberFormatException e) {
+    return false;
+}
 return prefix <= address.getAddress().length * 8;
Suggestion importance[1-10]: 7

__

Why: Valid concern: a prefix like /9999999999 matches \d+ but throws NumberFormatException from Integer.parseInt, which would propagate as an unchecked exception rather than a clean validation failure.

Medium
Explicitly reject empty IP portion

An empty IP portion (e.g., "/24" or "") currently passes to parseIpLiteral, which
returns null correctly for "", but the empty-string case is fragile. More
importantly, inputs with multiple slashes are already rejected, but consider
explicitly rejecting empty ip here to make intent clear and guard against
InetAddress.getByName("") semantics (which on some JVMs returns the loopback address
instead of throwing).

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [41-45]

 String[] values = cidr.split("/", -1);
 if (values.length == 0 || values.length > 2) {
     return false;
 }
 
 String ip = values[0];
+if (ip.isEmpty()) {
+    return false;
+}
 InetAddress address = parseIpLiteral(ip);
 if (address == null) {
     return false;
 }
Suggestion importance[1-10]: 4

__

Why: The empty string case is already handled by parseIpLiteral since it doesn't match IPV4_PATTERN and doesn't contain ':', returning null. The suggestion adds a minor defensive check but does not fix an actual bug.

Low
Suggestions up to commit 6d7401c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle prefix overflow parse errors

PREFIX_PATTERN matches \d+ with no length bound, so values like
"192.168.1.0/9999999999" will cause Integer.parseInt to throw NumberFormatException,
which is unchecked and will propagate out of the constructor instead of producing
the expected SigmaTypeError. Wrap the parse in a try/catch or bound the pattern to a
reasonable digit count.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [57-61]

-int prefix = Integer.parseInt(prefixStr);
+int prefix;
+try {
+    prefix = Integer.parseInt(prefixStr);
+} catch (NumberFormatException e) {
+    return null;
+}
 byte[] addressBytes = address.getAddress();
 if (prefix > addressBytes.length * 8) {
     return null;
 }
Suggestion importance[1-10]: 7

__

Why: Valid concern: PREFIX_PATTERN allows unbounded digits, so Integer.parseInt can throw NumberFormatException, which would leak out instead of resulting in the expected SigmaTypeError. The fix improves correctness.

Medium
Reject empty IP portion explicitly

An input like "192.168.1.0" (no slash) will pass here, but a string starting with /
such as "/24" would produce an empty ip string and later fail parsing correctly.
However, a trailing slash input like "192.168.1.0/" produces values[1] = "" which
fails PREFIX_PATTERN correctly. Consider also explicitly rejecting empty ip early to
make failure paths clearer and avoid relying on InetAddress.getByName("") behavior,
which on some JVMs resolves to the loopback address rather than failing.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [37-42]

 String[] values = cidr.split("/", -1);
 if (values.length == 0 || values.length > 2) {
     return null;
 }
 
 String ip = values[0];
+if (ip.isEmpty()) {
+    return null;
+}
Suggestion importance[1-10]: 5

__

Why: Explicitly rejecting an empty IP string is a reasonable defensive improvement, since InetAddress.getByName("") may resolve to loopback on some JVMs, causing unexpected acceptance of inputs like "/24".

Low
Suggestions up to commit 667935e
CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle prefix integer overflow safely

PREFIX_PATTERN allows arbitrarily long digit strings such as "999999999999", which
will overflow Integer.parseInt and throw NumberFormatException, propagating out
instead of being treated as an invalid CIDR. Guard the parse with a try/catch or a
reasonable length limit so oversized prefixes return false as expected.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [56-60]

-int prefix = Integer.parseInt(prefixStr);
+int prefix;
+try {
+    prefix = Integer.parseInt(prefixStr);
+} catch (NumberFormatException e) {
+    return false;
+}
 byte[] addressBytes = address.getAddress();
 if (prefix > addressBytes.length * 8) {
     return false;
 }
Suggestion importance[1-10]: 7

__

Why: Correct observation: PREFIX_PATTERN (\d+) allows arbitrarily long digit strings that would throw NumberFormatException on parse, causing an unexpected exception to leak instead of returning false as an invalid CIDR. The fix is appropriate.

Medium
Reject empty IP portion explicitly

An input like "/24" will split into ["", "24"], and parseIpLiteral("") may return
null (good), but an input like "192.168.1.0" with a trailing empty prefix
"192.168.1.0/" splits into ["192.168.1.0", ""], where PREFIX_PATTERN correctly
rejects the empty string. However, consider explicitly rejecting an empty IP portion
early to make intent clear and avoid depending on InetAddress.getByName("")
behavior, which on some JVMs returns the loopback address rather than throwing.

src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java [41-45]

 String[] values = cidr.split("/", -1);
 if (values.length == 0 || values.length > 2) {
     return false;
 }
 
 String ip = values[0];
+if (ip.isEmpty()) {
+    return false;
+}
 InetAddress address = parseIpLiteral(ip);
Suggestion importance[1-10]: 5

__

Why: Valid concern about InetAddress.getByName("") returning loopback on some JVMs, which would incorrectly accept empty IPs. The defensive check improves robustness, though parseIpLiteral may already reject empty strings via the pattern check.

Low

@mkhludnev

mkhludnev commented Aug 7, 2026

Copy link
Copy Markdown
Author

PR Code Suggestions

done at 2af4934

@osjohn01

osjohn01 commented Aug 25, 2026

Copy link
Copy Markdown

I raised this issue in the forum that this PR links to

brought from forum https://forum.opensearch.org/t/cidr-modifier-for-sigma-detection-rules-works-only-for-ipv4-addresses-not-for-ipv6-resulting-in-sigma-detection-error/28232

It seems like the PR is awaiting approval from a maintainer. My team would like to know the ETA on when this will get approved and merged. Thank you

@mkhludnev

mkhludnev commented Aug 25, 2026

Copy link
Copy Markdown
Author

Hello @peterzhuamazon may I ask your attention. Who may help to move it further?

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 61e180a

@github-actions

github-actions Bot commented Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 2d62ee0.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

PathLineSeverityDescription
src/main/java/org/opensearch/securityanalytics/rules/types/SigmaCIDRExpression.java4highNew dependency added: 'com.google.common.net.InetAddresses' (Guava). Any dependency addition must be flagged per mandatory supply chain policy — maintainers should verify the artifact hash and source in build files (pom.xml/build.gradle) to confirm this resolves to the legitimate google/guava package and not a typosquatted or namespace-hijacked artifact.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 1 | Medium: 0 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@mkhludnev

mkhludnev commented Aug 25, 2026

Copy link
Copy Markdown
Author

@copilot remove usage of com.google.common.net.InetAddresses;

@mkhludnev

mkhludnev commented Aug 25, 2026

Copy link
Copy Markdown
Author

@copilot squash commits in the branch mkhludnev:copilot/add-ipv6-support-to-sigma-rules

@mkhludnev
mkhludnev force-pushed the copilot/add-ipv6-support-to-sigma-rules branch from 4a599af to 2d62ee0 Compare August 25, 2026 20:55
@mkhludnev
drop guava
79dd4d1 
Signed-off-by: Mikhail Khludnev <mkhl@apache.org>
@mkhludnev
mkhludnev force-pushed the copilot/add-ipv6-support-to-sigma-rules branch from 2d62ee0 to 79dd4d1 Compare August 25, 2026 21:02
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 79dd4d1

@mkhludnev
address the review
667935e 
Signed-off-by: Mikhail Khludnev <mkhl@apache.org>
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 667935e

@mkhludnev
address the review
6d7401c 
Signed-off-by: Mikhail Khludnev <mkhl@apache.org>
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6d7401c

@mkhludnev
Stores the original string unchanged (this.cidr = cidr), so no silent…
3dfa4cb 
… rewrites, no IPv6

     re-formatting — old consumers comparing raw strings are unaffected.

Signed-off-by: Mikhail Khludnev <mkhl@apache.org>
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 3dfa4cb

@mkhludnev
Verified the lax-validation concern: getByName
04aebeb 
Signed-off-by: Mikhail Khludnev <mkhl@apache.org>
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 04aebeb

@mkhludnev
::ffff:1.2.3.4/96 is now accepted as an IPv6 CIDR.
f12210c 
Signed-off-by: Mikhail Khludnev <mkhl@apache.org>
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f12210c

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit e626621

@mkhludnev

mkhludnev commented Sep 2, 2026

Copy link
Copy Markdown
Author

Hello @sbcd90,
Would you please check this extension?

@osjohn01

osjohn01 commented Sep 9, 2026

Copy link
Copy Markdown

This PR is still open, dang

@mkhludnev

mkhludnev commented Sep 9, 2026

Copy link
Copy Markdown
Author

@osjohn01 I can only recommend you join https://www.meetup.com/opensearch/events/316298985/?eventOrigin=group_upcoming_events
and/or put this PR to the googledoc linked there.

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

Reviewers

@amsiglan amsiglan Awaiting requested review from amsiglan amsiglan is a code owner
@AWSHurneyt AWSHurneyt Awaiting requested review from AWSHurneyt AWSHurneyt is a code owner
@getsaurabh02 getsaurabh02 Awaiting requested review from getsaurabh02 getsaurabh02 is a code owner
@lezzago lezzago Awaiting requested review from lezzago lezzago is a code owner
@eirsep eirsep Awaiting requested review from eirsep eirsep is a code owner
@engechas engechas Awaiting requested review from engechas engechas is a code owner
@goyamegh goyamegh Awaiting requested review from goyamegh goyamegh is a code owner
@riysaxen-amzn riysaxen-amzn Awaiting requested review from riysaxen-amzn riysaxen-amzn is a code owner
@toepkerd toepkerd Awaiting requested review from toepkerd toepkerd is a code owner
@vikhy-aws vikhy-aws Awaiting requested review from vikhy-aws vikhy-aws is a code owner
@manaswini1920 manaswini1920 Awaiting requested review from manaswini1920 manaswini1920 is a code owner
@nishtham-amazon nishtham-amazon Awaiting requested review from nishtham-amazon nishtham-amazon is a code owner
@KashKondaka KashKondaka Awaiting requested review from KashKondaka KashKondaka is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mkhludnev @osjohn01