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

pmFromFile and matching problems #1167

Closed
quenenni opened this issue Jun 22, 2016 · 7 comments
Closed

pmFromFile and matching problems #1167

quenenni opened this issue Jun 22, 2016 · 7 comments
Assignees
Labels
RIP - Type - Usage Related with usage (not a bug)
Milestone

Comments

@quenenni
Copy link

Server: Debian Wheezy
libapache2-mod-security2: v2.8.0-2~bpo70+1
modsec rules: last from git


A simple test.php file including this code:

test
<img src="wp-content/themes/expound/screenshot.png">

I have a rule to catch all static files and for them skip my next custom rules.
This one works:

SecRule REQUEST_FILENAME "\.(ico|png|jpg|jpeg|gif|tiff|ods|fods|odt|fodt|odp|fodp|odg|doc|docx|xls|xlsx|rtf|csv|ppt|pptx|pps|ppsx|pdf|txt|css|js|ogg|ogm|mp4|flac|ape|wav|mkv|mpg|mpeg|swf|flv|mov|avi|wma)$"  "phase:1,id:2100,log,pass,skipAfter:END_CUSTOM_MARKER"

This one doesn't:

SecRule REQUEST_FILENAME "@pmFromFile /etc/modsecurity/liste_extensions.txt"    "phase:1,id:2100,nolog,pass,skipAfter:END_CUSTOM_MARKER"

Well, it works only when I put the complete filename in it (screenshot.png), but doesn't work when I try to put only file extensions:

png
or
.png
or
.*.png
or
png/
or
.png/

From what I read in the reference manual (https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual#pmFromFile -> point 4), it should be able to match all files finishing by png.. or am I mistaken.

Thanks

@csanders-git
Copy link

I'll start with the thing I first noticed and maybe it will help. This is in Phase 1, too my understanding it shouldn't be since your inspecting file data. https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual#phase-request-body

@quenenni
Copy link
Author

Thanks for your help @csanders-git

From my understanding, modsecurity has the filenames at phase 1. It should or I don't understand how it can know what to process. Maybe not all arguments as the manual says, but the filename requested yes.

The fact that my first example works in phase 1 shows that.

The difference with my second example is the use of the action pmFromFile instead of putting all extensions directly in the rule.

And if I put the complete filename in the file, it works too.

The problem seems that modsecurity doesn't take each line from the file and check if it can find that pattern in the filename but only do a string comparison.

But from the manual and the section about false positives, putting 'png' on a line should trigger a 'return 1' for filenames like 'anything.png' or 'png.blabla'.
And putting 'png/' in the file should trigger a 'return 1' for filenames finishing by png, what I would like to have.

@csanders-git
Copy link

i'm thinking (but not entirely sure) that you are thinking about this backwards. For example. if the INPUT from modsecurity was 1.2.3.1 (REMOTE_ADDR for instance) and we did a pmf such as:
SecRule REMOTE_ADDR "@pmf blacklist.conf" "id:1"
then if 1.2.3.100 was in the FILE it would match.
To restate if the input is a subset of an element in the file it will ALSO match (in addition to being exact)
My thought is that you are trying to go the other way if the element in the file is a subset of the input your are expecting a match. Not sure that works... but without testing i don't know for sure.

@quenenni
Copy link
Author

quenenni commented Jun 27, 2016

I see your point.
But I don't think it's correct. Not from what I understand from the manual and the example given (but I agree it's a bit confusing):

Because this operator does not check for boundaries when matching, false positives are possible in some cases. For example, if you want to use @pm for IP address matching, the phrase 1.2.3.4 will potentially match more than one IP address (e.g., it will also match 1.2.3.40 or 1.2.3.41). To avoid the false positives, you can use your own boundaries in phrases. For example, use /1.2.3.4/ instead of just 1.2.3.4. Then, in your rules, also add the boundaries where appropriate. You will find a complete example in the example.

For me, "the phrase 1.2.3.4" means 1.2.3.4 in the file.
And "match more than one IP address (e.g., it will also match 1.2.3.40 or 1.2.3.41)" means if the INPUT from modsec (the ip address that does the request) is 1.2.3.40...

What gives me some certainty is the the § explains boundaries with the 1.2.3.4 ip address. So /1.2.3.4/ can only be in the file

Then... 1.2.3.4 -> triggered by request ip address 1.2.3.4 & 1.2.3.40
But as the § says, boundaries can be used on both sides (/1.2.3.4/). We can then assume the ip address 11.2.3.4 would trigger also the rule.
Then:

  • png should be triggered by filenames png & xxx.png & png.doc
  • /png should be triggered by filenames png & png.doc
  • png/ should be triggered by filenames png & xxx.png
  • /png/ should be triggered only by filename png

As I said, all of this is a bit confusing.

@quenenni
Copy link
Author

quenenni commented Jul 1, 2016

I did several tests to see where the problem is.
The difference with before is now I'm using the CRS v3 (instead of v2.2.9).
But libapache2-mod-security2 is the same version, so it should have the same result.

The rule:

SecRule REQUEST_FILENAME "@pmFromFile /etc/modsecurity-conf/liste_extensions.txt"    "phase:1,id:2100,nolog,pass,skipAfter:END_CUSTOM_MARKER"
  .. do xxxx
SecMarker END_CUSTOM_MARKER

In my file, I tried this:

  1. aascreenshot.png
  2. /aascreenshot.png
  3. aascreenshot.png/
  4. /aascreenshot.png/
  5. screenshot.png
  6. /screenshot.png
  7. screenshot.png/
  8. /screenshot.png/
  9. eenshot.png
  10. /eenshot.png
  11. eenshot.png/
  12. /eenshot.png/

And here are the results:

  1. do xxxx -> correct with my theory / incorrect with yours
  2. do xxxx -> correct with both our theories
  3. do xxxx -> correct with my theory / incorrect with yours
  4. do xxxx -> correct with both our theories
  5. do NOT xxxx -> correct with both our theories
  6. do NOT xxxx -> correct with both our theories
  7. do NOT xxxx -> correct with both our theories
  8. do xxxx -> incorrect with both our theories
  9. do NOT xxxx -> correct with my theory / incorrect with yours
  10. do xxxx -> correct with both our theories
  11. do xxxx -> correct with your theory / incorrect with mine
  12. do xxxx -> correct with both our theories

These results are very very confusing.
Unfortunately for me, the only one I want to use (11) is the only one where my theory is wrong :)
But I'd say there's something wrong with the action pmFromFile, whatever theory is the right one.

@HenryOrh
Copy link

I was just wondering if there were any news on this.

Despite the wording of the Reference Manual, I have likewise noted that, when using @pmFromFile, an entry of 'aaa' in the file will not match 'aaabbb' in input.

Indeed, CRS3.0 seems to assume that phrases in a file will be subject to exact match rather than substring. For example, REQUEST-933-APPLICATION-ATTACK-PHP.conf uses php-function-names-933151.data which has numerous examples of successive phrases where one is a substring of the others, for example:
date_parse
data_parse_from_format

If the @pmFromFile actually worked the way the Manual suggests (not checking for boundaries and thus acting like a substring checker) then there would be no point to having both lines (the first of the two would be sufficient).

@zimmerle zimmerle self-assigned this Feb 23, 2018
@zimmerle zimmerle added this to the v3.0.1 milestone Feb 23, 2018
@zimmerle zimmerle modified the milestones: v3.0.1, v3.0.2 Apr 2, 2018
@zimmerle zimmerle modified the milestones: v3.0.3, v2.9.3 Sep 25, 2018
@victorhora victorhora added the RIP - Type - Usage Related with usage (not a bug) label Oct 2, 2018
@victorhora
Copy link
Contributor

I was just wondering if there were any news on this.

Despite the wording of the Reference Manual, I have likewise noted that, when using @pmFromFile, an entry of 'aaa' in the file will not match 'aaabbb' in input.

Indeed, CRS3.0 seems to assume that phrases in a file will be subject to exact match rather than substring. For example, REQUEST-933-APPLICATION-ATTACK-PHP.conf uses php-function-names-933151.data which has numerous examples of successive phrases where one is a substring of the others, for example:
date_parse
data_parse_from_format

If the @pmFromFile actually worked the way the Manual suggests (not checking for boundaries and thus acting like a substring checker) then there would be no point to having both lines (the first of the two would be sufficient).

I couldn't reproduce this on the latest code in v2.9 branch master:

[01/Oct/2018:19:50:51 --0400] [localhost/sid#7fbacf5d9820][rid#7fbac40350a0][/index.html][5] Rule 7fbacf5e20f0: SecRule "ARGS:pm" "@pmf /tmp/files.txt" "phase:4,status:403,tag:'DROP on phase 4!',id:25,deny,log,auditlog"
[01/Oct/2018:19:50:51 --0400] [localhost/sid#7fbacf5d9820][rid#7fbac40350a0][/index.html][4] Transformation completed in 0 usec.
[01/Oct/2018:19:50:51 --0400] [localhost/sid#7fbacf5d9820][rid#7fbac40350a0][/index.html][4] Executing operator "pmf" with param "/tmp/files.txt" against ARGS:pm.
[01/Oct/2018:19:50:51 --0400] [localhost/sid#7fbacf5d9820][rid#7fbac40350a0][/index.html][9] Target value: "aaabbb"
[01/Oct/2018:19:50:51 --0400] [localhost/sid#7fbacf5d9820][rid#7fbac40350a0][/index.html][4] Operator completed in 1 usec.
[01/Oct/2018:19:50:51 --0400] [localhost/sid#7fbacf5d9820][rid#7fbac40350a0][/index.html][4] Rule returned 1.
[01/Oct/2018:19:50:51 --0400] [localhost/sid#7fbacf5d9820][rid#7fbac40350a0][/index.html][9] Match, intercepted -> returning.
[01/Oct/2018:19:50:51 --0400] [localhost/sid#7fbacf5d9820][rid#7fbac40350a0][/index.html][1] Access denied with code 403 (phase 4). Matched phrase "aaa" at ARGS:pm. [file "/etc/modsecurity/modsecurity.conf"] [line "567"] [id "25"]

Same for v3.x:

[153843777148.107283] [] [4] Initializing transaction
[153843777148.107283] [] [4] Transaction context created.
[153843777148.107283] [] [4] Starting phase CONNECTION. (SecRules 0)
[153843777148.107283] [] [9] This phase consists of 0 rule(s).
[153843777148.107283] [] [4] Starting phase URI. (SecRules 0 + 1/2)
[153843777148.107283] [/?pm=aaabbb] [4] Adding request argument (GET): name "pm", value "aaabbb"
[153843777148.107283] [/?pm=aaabbb] [4] Starting phase REQUEST_HEADERS.  (SecRules 1)
[153843777148.107283] [/?pm=aaabbb] [9] This phase consists of 1 rule(s).
[153843777148.107283] [/?pm=aaabbb] [4] (Rule: 25) Executing operator "PmFromFile" with param "/tmp/files.txt" against ARGS:pm.
[153843777148.107283] [/?pm=aaabbb] [9] Target value: "aaabbb" (Variable: ARGS:pm)
[153843777148.107283] [/?pm=aaabbb] [9] Matched vars updated.
[153843777148.107283] [/?pm=aaabbb] [4] Running [independent] (non-disruptive) action: log
[153843777148.107283] [/?pm=aaabbb] [9] Saving transaction to logs
[153843777148.107283] [/?pm=aaabbb] [4] Rule returned 1.
[153843777148.107283] [/?pm=aaabbb] [4] Running (disruptive)     action: deny
[153843777148.107283] [/?pm=aaabbb] [8] Running action deny
[153843777148.107283] [/?pm=aaabbb] [4] Running (non-disruptive) action: auditlog
[153843777148.107283] [/?pm=aaabbb] [8] Skipping this phase as this request was already intercepted.
[153843777148.107283] [/?pm=aaabbb] [4] Starting phase RESPONSE_HEADERS. (SecRules 3)
[153843777148.107283] [/?pm=aaabbb] [9] This phase consists of 0 rule(s).
[153843777148.107283] [/?pm=aaabbb] [9] Appending response body: 116 bytes. Limit set to: 524288.000000
[153843777148.107283] [/?pm=aaabbb] [9] Appending response body: 170 bytes. Limit set to: 524288.000000
[153843777148.107283] [/?pm=aaabbb] [4] Starting phase RESPONSE_BODY. (SecRules 4)
[153843777148.107283] [/?pm=aaabbb] [9] This phase consists of 0 rule(s).
[153843777148.107283] [/?pm=aaabbb] [4] Starting phase LOGGING. (SecRules 5)
[153843777148.107283] [/?pm=aaabbb] [9] This phase consists of 0 rule(s).
[153843777148.107283] [/?pm=aaabbb] [8] Checking if this request is suitable to be saved as an audit log.
[153843777148.107283] [/?pm=aaabbb] [8] Checking if this request is relevant to be part of the audit logs.
[153843777148.107283] [/?pm=aaabbb] [5] Saving this request as part of the audit logs.
[153843777148.107283] [/?pm=aaabbb] [8] Request was relevant to be saved. Parts: 6014

Please let us know if the issue persist on the current versions and we can investigate further. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RIP - Type - Usage Related with usage (not a bug)
Projects
None yet
Development

No branches or pull requests

5 participants