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

MatchedRules design can be changed to store matches per rule. #183

Closed
piyushroshan opened this issue Feb 23, 2022 · 3 comments · Fixed by #220
Closed

MatchedRules design can be changed to store matches per rule. #183

piyushroshan opened this issue Feb 23, 2022 · 3 comments · Fixed by #220
Labels
enhancement New feature or request v3

Comments

@piyushroshan
Copy link
Contributor

Summary

MatchedRules design can be changed to store matches per rule.
Currently every match, especially multiMatch may add many matches. However getting unique rule matches would be an unnecessary extra task.
Storing the matches per rule and adding all matches mapped together would provide a way to do Anomaly Scoring like Modsec and better visualization of matches. It would also reduce memory use by not deduplicating other fields of MatchedRule.

Basic example

If the MatchedRules structure is changed as follows, this will give interesting observation.

type MatchedRule struct {
	// Full request uri unparsed
	URI string
	// Transaction id
	ID string
	// Is disruptive
	Disruptive bool
	// Server IP address
	ServerIPAddress string
	// Client IP address
	ClientIPAddress string
	// A slice of matched variables
	MatchedDatas []MatchData <--- List of MatchedData
	// A reference to the triggered rule
	Rule *Rule
}

type MatchData struct {
	// variable name stored for cache
	VariableName string
	// Variable
	Variable variables.RuleVariable
	// Key of the variable, blank if no key is required
	Key string
	// Value of the current VARIABLE:KEY
	Value string
	// Macro expanded message
	Message string  <-- Moved here from MatchedRule 
	// Macro expanded logdata
	Data string  <-- Moved here from MatchedRule 
}

Motivation

Better results, handling multi matches on a rules. Anomaly observation.

@jptosso jptosso added the enhancement New feature or request label Feb 24, 2022
@piyushroshan piyushroshan changed the title MatchedRules desing can be changed to store matches per rule. MatchedRules design can be changed to store matches per rule. Feb 24, 2022
@jptosso
Copy link
Member

jptosso commented Mar 2, 2022

Matched Rule is sent to the log callback to generate audit logs, if we do this the user would have to read the matched rule and iterate over the matched data. I think it is possible but we are already far from ModSecurity behavior, they use the log action to instantly write the log, while we mark the rule as "loggable" and in future versions this could change

@piyushroshan
Copy link
Contributor Author

We have already seen issues of logs being flooded in modsecurity from multimatch. There is no point in sending many log events for one rule match. Its the rule and any of the matched data that is important. The handling could be left to the users if they want to print each one.

@syinwu
Copy link
Member

syinwu commented Mar 3, 2022

I think this is a good idea, it can reduce the memory usage of MatchedRules. We can keep the format of the log as it is.

@jptosso jptosso added the v3 label Mar 30, 2022
@piyushroshan piyushroshan mentioned this issue Apr 11, 2022
3 tasks
@jptosso jptosso linked a pull request Apr 11, 2022 that will close this issue
3 tasks
@syinwu syinwu removed a link to a pull request Apr 14, 2022
3 tasks
@syinwu syinwu linked a pull request Apr 14, 2022 that will close this issue
3 tasks
jptosso pushed a commit that referenced this issue Apr 18, 2022
* Rule matches optimization

* Preserve pre chain matches

* Spell correction

* Review comments & SubChain rule msg cases

* Remove unwanted comments

Co-authored-by: bxlxx.wu <bxlxx.wu@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v3
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants