-
-
Notifications
You must be signed in to change notification settings - Fork 401
feat: added detection for RCE via Referer header #3993
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
base: main
Are you sure you want to change the base?
Conversation
📊 Quantitative test results for language: |
tests/regression/tests/REQUEST-932-APPLICATION-ATTACK-RCE/932207.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
tests/regression/tests/REQUEST-932-APPLICATION-ATTACK-RCE/932207.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
@theseion LGTM :) |
Will check, need a little time though. |
@Xhoenix You are doing double URL decode using |
Also, i don't think we need such a complex rule for this. According to RFC 9110, |
I checked the RFC 9110 and Referer header must not contain fragments. I think we should just update rule |
The RFC does not reflect our reality. You can easily send a Referer header with a fragment and then it's up to the server how it handles that URL. We need to anticipate attacks that abuse this. |
Do you know about any client which is doing it? |
No. But it's easy to do manually. I'm not worried about benign clients but about deliberate attack requests. |
I don't get you. This applies to every single attack - all of them are easy to send manualy but that's not a reason to NOT block it. |
If I had read your comment carefully, @azurit, instead of just skimming it, I would have realised that you are suggesting the same thing. I'm sorry. I was under the impression that you were arguing for ignoring fragments in the Referer header. Please ignore what I've said above.
That's not a bad idea @Xhoenix. |
In #3485 we saw that fragments in the Referer header are common and we decided. We also saw that we needed to address the Referer header separately from other rules, because the semantics of the header can lead to many FPs with the detections used in other rules. Similarly, fragments can contain almost anything and need to be handled separately to reduce FPs to a minimum. The new rule now only looks at the fragment, while 932206 only looks at values that do not look like URLs. What's still missing is a rule that looks at URLs (skipping the fragment) in the Referer header. |
I think rule 932200 covers that. This is after all based off of 932200. |
932200 doesn't check the Referer header. |
I checked and rule 932205 does check the Referer header in the URL part. |
Should we close this PR? |
Fixes #3498