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

update: remove Lookahead regular expressions #4526

Merged
merged 5 commits into from
Oct 30, 2023
Merged

update: remove Lookahead regular expressions #4526

merged 5 commits into from
Oct 30, 2023

Conversation

fukusuket
Copy link
Contributor

@fukusuket fukusuket commented Oct 29, 2023

Summary of the Pull Request

Removed unnecessary Lookahead regular expressions because it is a syntax not supported by some programming languages like Golang, Rust.
I think this rule will not be hit in most cases because of the (?!.) at the end. So I removed (?!.) What do you think?
(If you want to clearly indicate that there is no character (other than a newline), I think $ would be fine?)

I'm sorry if I misunderstood the intent of this regular expression...

Changelog

update: Obfuscated IP Download Activity

Example Log Event

N/A

Fixed Issues

N/A

SigmaHQ Rule Creation Conventions

  • If your PR adds new rules, please consider following and applying these conventions

@github-actions github-actions bot added Rules Windows Pull request add/update windows related rules labels Oct 29, 2023
@fukusuket
Copy link
Contributor Author

fukusuket commented Oct 29, 2023

Lookahead regular expressions are not supported in some programming languages, so I think you can support more languages ​​by avoiding Lookahead use if possible.

For example, the code using Lookahead regex below will result in a compilation error.
Golang
https://go.dev/play/p/7QX4pADRfla

Rust
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7a8c3a81bc92ace403c3da42a583ae8f

I think it's difficult to support regular expressions for all languages... :(,
but I think it's better to use more supported regular expressions as much as possible.

@fukusuket fukusuket changed the title Fix remove unneeded look ahead regular expressions Fix remove unneeded Lookahead regular expressions Oct 29, 2023
@nasbench nasbench self-requested a review October 29, 2023 11:34
@nasbench
Copy link
Member

The positive look-ahead is somewhat necessary to prevent FP with normal IPs. For example:

This http://81.4.317 will match the regex in its current form and this http://81.4.317.123 will not. But if you remove the look-ahead, both will match and its an FP by the rule's logic.

  • With look-ahead

image

  • Without

image

@nasbench nasbench self-assigned this Oct 29, 2023
@fukusuket
Copy link
Contributor Author

fukusuket commented Oct 29, 2023

@nasbench
Thank you so much for review :) Sorry for my misunderstanding. I understand that Lookahead has an intention.
In the above case, wouldn't a simple $ work?

@fukusuket
Copy link
Contributor Author

fukusuket commented Oct 29, 2023

I may also be misunderstanding the specifications of |re modifier...

The regular expression below is
CommandLine|re: 'https?:\/\/[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,5}(?!\.)'

Is it assumed that it will be converted to the following regular expression?
.*https?:\/\/[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,5}(?!\.).*
(Is it expected to implicitly behave as a partial match regular expression?)

If I can write the normal IP address pattern as a not filter, I might be able to avoid using Lookahead.

@fukusuket fukusuket changed the title Fix remove unneeded Lookahead regular expressions update remove Lookahead regular expressions Oct 29, 2023
@fukusuket fukusuket changed the title update remove Lookahead regular expressions update: remove Lookahead regular expressions Oct 29, 2023
@fukusuket
Copy link
Contributor Author

fukusuket commented Oct 29, 2023

@nasbench
I added filter for normal IP. What do you think? I would appreciate it if you could review it.
If Lookahead regex is better, I'll close this PR!

@fukusuket
Copy link
Contributor Author

I found that there was some discussion about regular expression specifications in this issue.
SigmaHQ/sigma-specification#25

@nasbench
Copy link
Member

Hey @fukusuket thanks for the update. I like the filter idea that will make it more generic for different backends :)

Thanks.

@nasbench nasbench added the 2nd Review Needed PR need a second approval label Oct 30, 2023
@nasbench nasbench merged commit b92a0fe into SigmaHQ:master Oct 30, 2023
11 checks passed
@fukusuket fukusuket deleted the fix-remove-unneeded-regex-look-ahead branch October 30, 2023 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2nd Review Needed PR need a second approval Rules Windows Pull request add/update windows related rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants