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

Include log marker name in example CRS test rule #64

Closed
anuraaga opened this issue Aug 31, 2022 · 7 comments · Fixed by #73
Closed

Include log marker name in example CRS test rule #64

anuraaga opened this issue Aug 31, 2022 · 7 comments · Fixed by #73
Assignees

Comments

@anuraaga
Copy link
Contributor

I noticed that when looking for a log marker, it also checks for the marker name

https://github.com/fzipi/go-ftw/blob/f5f64a16b3d2bebea600ea070ffd5baa7f36213c/waflog/read.go#L114

The example rule for logging the marker doesn't output the name though

https://github.com/fzipi/go-ftw#how-log-parsing-works

Presumably there is some assumption that the log message automatically includes header names - but would the rule be more generic by including the name in it, e.g.

# Write the value from the X-CRS-Test header as a marker to the log
SecRule REQUEST_HEADERS:X-CRS-Test "@rx ^.*$" \
  "id:999999,\
  phase:1,\
  log,\
  msg:'X-CRS-Test %{MATCHED_VAR}',\
  pass,\
  t:none"
@fzipi
Copy link
Member

fzipi commented Sep 1, 2022

The name can be changed with the logmarkerheadername config var, and I presume that is what is checking for. But I'll let @theseion answer this one :)

@anuraaga
Copy link
Contributor Author

anuraaga commented Sep 2, 2022

@fzipi Yup, for this example rule the name is also encoded in the REQUEST_HEADERS line at top so changing the name would require the user to be aware and update either way. So I guess adding to the msg line in addition wouldn't be anything new in that respect.

@theseion
Copy link
Collaborator

theseion commented Sep 3, 2022

Correct. @anuraaga do you agree that we can close the issue?

@anuraaga
Copy link
Contributor Author

anuraaga commented Sep 5, 2022

No sorry if wasn't clear - I think @fzipi's point is that the logmarkerheadername is configurable so the example rule won't work if it's reconfigured. That is already the case though so wouldn't be a reason not to change the example I think.

But assuming the default name, currently the example rule doesn't log the name while ftw looks for it. By updating the rule with my suggestion, that possible issue goes away to make it easier to get started - notably when first trying out ftw it was failing with the missing log marker message even though the rule was working, and looking at the code I found the check for the name is there too

@theseion
Copy link
Collaborator

theseion commented Sep 6, 2022

Sorry, I'm still confused. When the rule is triggered with the example rule (which, incidentally is the real rule we use in CRS) the following shows up in the log:

[Tue Sep 06 13:43:51.130630 2022] [:error] [pid 284:tid 281472015524224] [client 172.18.0.1:64810] [client 172.18.0.1] ModSecurity: Warning. Pattern match "^.*$" at REQUEST_HEADERS:X-CRS-Test. [file "/etc/modsecurity.d/owasp-crs/crs-setup.conf"] [line "699"] [id "999999"] [msg "fb0d90ac-915e-4d59-a949-792d6a278c06"] [tag "modsecurity"] [hostname "localhost"] [uri "/"] [unique_id "YxdOlyRe92sSWMB-4Xte3gAAAIo"]

You can see that it contains the header name so there's no need to put it in the message.

@anuraaga
Copy link
Contributor Author

anuraaga commented Sep 7, 2022

Ah sorry for the confusion @theseion - I am testing Coraza with ftw which doesn't have the Pattern match type of logging. Perhaps this is a point to improve in Coraza - but given the rule spec itself does dictate what is logged in the log operation, perhaps it could be decoupled from implementation details of the logger by including the required name in the rule itself.

@theseion
Copy link
Collaborator

theseion commented Sep 7, 2022

Ah yes, that's a good point. I had just assumed that the log would look the same for all engines. Would you mind opening a pull request with your proposal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants