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

Malicious file upload detection #1045

Closed
wants to merge 35 commits into
base: v3.1/dev
from

Conversation

Projects
None yet
7 participants
@spartantri
Copy link
Collaborator

spartantri commented Mar 19, 2018

Replacement for 994

csanders-git and others added some commits Sep 20, 2017

Update .travis.yml
Update to support v3.1
Merge pull request #978 from SpiderLabs/readme-minors
Update date and add Travis badges
Merge pull request #979 from ihacku/patch-1
Update REQUEST-920-PROTOCOL-ENFORCEMENT.conf

@spartantri spartantri changed the title PR994 remake Malicious file upload detection Mar 19, 2018

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Mar 19, 2018

haha, this is the easier solution.

spartantri added some commits Mar 20, 2018

spartantri added some commits Mar 21, 2018

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Mar 22, 2018

wow this is coming along nicely

@spartantri spartantri referenced this pull request Mar 22, 2018

Closed

Hybrid Paranoia Mode #1050

@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Mar 22, 2018

The basic stuff is ready, I will try to write the tests somewhere in April, if the ftw issue with sending nulls still there I will keep the tests to just a very few use cases.

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Mar 22, 2018

can you refreash me on that ftw issue?

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Mar 22, 2018

This PR now mixes a proposal for a hybrid paranoia mode with malicious file upload detection. See the discussion in #1050. The way things stand now, this PR here should not be merged.

@spartantri spartantri referenced this pull request Mar 22, 2018

Closed

April monthly meeting agenda #1051

0 of 9 tasks complete
@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Mar 22, 2018

Thank you for the stripping of the paranoia mode stuff. This nullifies my comments above.

I have not tested this, but it looks good. A thing worth fixing is the uneven use of empty lines in the file. Please take a look at existing rule files to adopt that style. But that's really formal nitpicking by somebody who did not look into the rules themselves.

@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Mar 22, 2018

@csanders-git it is not nulls it is \xac and \xed ftw issue 16 so it should not be a problem to write rules, I will just skip those if it was nulls that would be a problem as it is often used.

@dune73 will take a look to style stuff, I suggest adding the empty line usage to CONTRIBUTING.md, this is not homogeneous, e.g. randomly opened 913 and there is no defined style and the travis checks do not complain on this either to flag something that is not 'in style'.

@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Jun 14, 2018

Adding comments to 901180 causes conflicts I do not know why so I removed them

# The REQUEST_FILENAME used for uploading files must be declared here using a # before and after it as delimiters
# Use lowercase only values
@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Jun 14, 2018

Does anyone know what is failing with travis?

@franbuehler

This comment has been minimized.

Copy link
Collaborator

franbuehler commented Jun 15, 2018

Hi @spartantri

Here are a few answers (not all yet):

"The setting of tx.protected_uploads in rule 900270 does not work, because the rule id 901180 (INITIALIZATION) overrides it. A "@eq 0" check should be added to 901180."
What rule is that? I don't get the 901180, SecAction with '@eq 0'

The crs-setup.conf is meant to adjust variables for your own installation. If a variable is not set in crs-setup.conf the 901-INITIALIZATION file sets a default value.
This default value (901) should only be set, if the variable is empty, if the variable is not set in crs-setup.conf.
Like here (example):

# Default Outbound Anomaly Threshold Level (rule 900110 in setup.conf)
SecRule &TX:outbound_anomaly_score_threshold "@eq 0" \
    "id:901110,\
    phase:1,\
    pass,\
    nolog,\
    setvar:'tx.outbound_anomaly_score_threshold=4'"

-> SecRule instead of SecAction
This is necessary because the rules in 901 are run after the crs-setup.conf.
If you have a look at the modsec_debug.log, you see that in your configuration, the variable tx.protected_uploads set in 900270 is always overriden by 901180.
This never should have worked in your tests.

This is also the answer for one of your next questions:

"Rule 914240: comment where protected uris can be configured: 900270."
You mean 901180?

No, configurations are done in 900270 (crs-setup.conf), default initialization are done in 901-INITIALIZATION, but only if variable is empty: @eq 0.
Also see comment in 901:

# This file REQUEST-901-INITIALIZATION.conf initializes the Core Rules
# and performs preparatory actions. It also fixes errors and omissions
# of variable definitions in the file crs-setup.conf.
# The setup.conf can and should be edited by the user, this file
# is part of the CRS installation and should not be altered.

spartantri added some commits Jun 20, 2018

@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Jun 20, 2018

BTW if this is too conflicting we can let only the executable part and leave everything after the SecMarker "BEGIN-REQUEST-914-FILE-UPLOAD-IMAGE" out for next iteration so we do not delay the RC

@franbuehler

This comment has been minimized.

Copy link
Collaborator

franbuehler commented Jul 2, 2018

That's a good proposal. I think it is a good idea to postpone the rules after BEGIN-REQUEST-914-FILE-UPLOAD-IMAGE to a later release.
I have the feeling, that this PR is not very well tested. That it needs not only a review but some more work.
What's your opinion, @spartantri? If you say, everything is tested and ready for a new review, I will retest.

@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Jul 3, 2018

I have little time to do any testing as I have too many trips in the coming weeks so I prefer to simply commenting all those rules and we see afterwards like in October after my traveling spree is over. So we go to RC with what is already well tested and do not cause any more delays than what we already have.

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 3, 2018

getting a bunch of these btw AH00526: Syntax error on line 308 of /etc/apache2/modsecurity.d/owasp-crs/rules/REQUEST-914-FILE-DETECTION.conf:
ModSecurity: SkipAfter actions can only be specified by chain starter rules.
Action 'restart' failed.

@csanders-git

This comment has been minimized.

Copy link
Collaborator

csanders-git commented Jul 4, 2018

@spartantri that's fine, thank you for all you're already doing for the project! Enjoy your trips. When you get a chance, rebase this against the v3.2/dev branch. We'll be updating it to match RC1 when it comes out so feel free to wait a bit but it'll be needed down the line.

spartantri added some commits Jul 25, 2018

@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Aug 2, 2018

Hi @franbuehler, @csanders-git, I just got back to France after my tour over America and as discussed in the summit, I've disabled all image and document upload checks and left enabled only the exe, dll and elf file upload detection.

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Aug 7, 2018

Today is the end of the merge window for 3.1. I've tried to clean up this PR today and keep only the ELF/EXE/DLL rules.

However, when testing them, I came up failing with my first .exe file (putty.exe). It starts with 4D 5A 00 00, while the rule assumes that an exe starts with only 4D 5A 50 or 4D 5A 90. According to Wikibooks the exe header is structured as follows:

 struct DOS_Header 
 {
     // short is 2 bytes, long is 4 bytes
     char signature[2] = { 'M', 'Z' };
     short lastsize;
...

This means that bytes 02 and 03 can basically be everything, forcing us to detect EXE files only by the MZ magic header. That seems to me a controversial reduction in specificity, as I'm afraid this sequence will turn up often as as false positive. Time is now running out and I'm not comfortable making this change. So I guess it won't make it in 3.1.

I kept my attempt in https://github.com/lifeforms/owasp-modsecurity-crs/blob/spartantri-fileupload/rules/REQUEST-914-FILE-DETECTION.conf

Some other changes I made versus this PR:

  • renumber to our convention starting at 914100 increasing by +1 for stricter siblings
  • made metadata including severity (CRITICAL) the same between 914100 and 914101
  • merge REQUEST_BODY / ARGS rules to reduce number of rules/maintenance
  • updated tx.paranoia_level to tx.executing_paranoia_level
  • conformed paranoia level skips to format in other files
  • minor tweaks to comments
  • removed logdata, because it led to unreadable/chopped off audit logs

I tested using: curl -d @putty.exe http://localhost/

We will need to revisit this for 3.2.

@lifeforms lifeforms added this to the CRS v3.2.0 milestone Aug 7, 2018

@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Aug 8, 2018

We can improve the regex and add the null but \x00 cause it to break maybe using \0 instead could work

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Aug 8, 2018

I don't think a null will be enough, I think we can't assume anything about the third byte of the .exe

But definitely we should figure out how to match null bytes in regexps.. You would think that \x00 should work but no. Maybe we can try octal? If that also breaks then we might have to bother Felipe...

@spartantri

This comment has been minimized.

Copy link
Collaborator

spartantri commented Aug 8, 2018

@lifeforms nulls are supposed to be forbidden, it is the only forbidden char in PL1, that's why I didn't bother to make nulls work since the 920270 rule would hit it but I definitively won't block based only on MZ at least not in PL1. As you said the third byte may change so it is hard to tell based on that only byte without false positives that it is or is not a windows exe. However this is the first rule batch, and I think that there still a lot of work to do on this section, maybe liaising in exiftool or something like that as external script to check could be a cool feature for PL3 or 4 (as optional rule of course) can reduce false negatives.

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Dec 3, 2018

After discussion, sadly it seems most appropriate that we don't continue with this PR. :( First it is very big and therefore hard to review meaningfully. Second, as mentioned in my earlier comment, I already tried reducing the merge only to executable uploads detection, but found problems with the .exe MZ header signature. Therefore it seems to be a more prickly problem than we assumed, and we should only address it in small steps.

We could restart with only the part for detecting ELF headers, which might be a more tractable problem and could still be very useful to our users. Then if that is merged, we could think of trying a next stab at detecting .exe uploads again, but we should do it carefully to not cause false positives.

(PS: we can also do other things to detect against Windows exe uploads; for instance we already have a rule triggering on .php file name uploads, we could block .exe as well)

My thanks and apologies to everyone who worked hard on this PR, but I'll close it in its current form. To be continued...?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment