feat(extensions): added some archive extensions as restricted

[gwen001]

Report: JW2SU88A

Added some archive extensions as restricted. However this certainly means that those type of files won't be downloadable anymore. Up to you !

.7z
.bz
.bz2
.gz
.rar
.tar
.tgz
.zip
.xz

[lifeforms]

I certainly wouldn't mind this!

One thing, did you mean .bz2 instead of .b2 ?

[gwen001]

Oh yes sorry !

Fixed.

[azurit]

This should not go into PL1.

[dune73]

Good thinking @azurit.

These additional file extensions - nice set @gwen001 - are all problematic, but they are more likely to cause false positives / be acceptable than the existing ones in the list (-> .bak, .dll etc.). So it would be natural to add these additional ones in a stricter sibling at PL2 perhaps. Yet this list being a CRS variable, splitting would be really tiring.

So I am a bit torn. What do you think @lifeforms?

[lifeforms]

HMMMM! I was confused. I thought we were already blocking .zip, and adding other archive types would add consistency - but it seems I added .zip to my custom config and it's not blocked in the CRS by default.

It sadly happens a lot with PHP developers that they leave .sql and .zip files laying around in the web root. :(

I think we should be consistent though. Either block all archives or don't block them.

But I can see how it would be problematic as a general measure, because there are valid use-cases for hosting these kinds of files.

[RedXanadu]

# Forbidden file extensions.
# Guards against unintended exposure of development/configuration files.

Are we arguing that anything ending in .tar and .bz2 are "development/configuration" files? Those are common, generic file formats to download. This seems like it would introduce many false positives.

Also, where do we stop? What about .xz? .zstd?

[lifeforms]

Yeah, good point.

Maybe we should just add them as an additional example in crs-setup.conf.example, so users can choose to block these files, but not include them in the defaults?

For instance:

# Forbidden file extensions.
# Guards against unintended exposure of development/configuration files.
# Default: .asa/ .asax/ .ascx/ .axd/ .backup/ .bak/ .bat/ .cdx/ .cer/ .cfg/ .cmd/ .com/ .config/ .conf/ .cs/ .csproj/ .csr/ .dat/ .db/ .dbf/ .dll/ .dos/ .htr/ .htw/ .ida/ .idc/ .idq/ .inc/ .ini/ .key/ .licx/ .lnk/ .log/ .mdb/ .old/ .pass/ .pdb/ .pol/ .printer/ .pwd/ .rdb/ .resources/ .resx/ .sql/ .swp/ .sys/ .vb/ .vbs/ .vbproj/ .vsdisco/ .webinfo/ .xsd/ .xsx/
# Example: To also block archives such as zip, tgz: .asa/ .asax/ .ascx/ .axd/ .backup/ .bak/ .bat/ .bz2/ ... .tar/ .tgz/ ... .zip/

[azurit]

Or create another rule which will add archive extensions:

# Forbidden file extensions - archives.
# Uncomment this rule to block archive extensions.
#SecAction \
# "id:900241,\
#  phase:1,\
#  nolog,\
#  pass,\
#  t:none,\
#  setvar:'tx.restricted_extensions=%{tx.restricted_extensions} .7z/ .bz/ .b2/ .gz/ .rar/ .tar/ .tgz/'"

[lifeforms]

I like that too. Also because I can disable that extra rule on a virtualhost when I do need to host archives.

[gwen001]

Much better that way I agree, easy/interesting to create more rules by themes: media (mpg, avi, ogg...), documents (doc, xls...).

[dune73]

Please notice that JW2SU88A is also covered in PR #2560.

[dune73]

CRS Bug Bounty PR assessment

• Rules affected (list rules): 920440
• Paranoia Level addressed (1, 2, 3, 4, full or explain): 1
• FTW passes (yes or no) : Yes
• Rule(s) picked for solution (correct or not-correct or explain) : OK
• Risk for false positives (irrelevant, adequate, substantial or explain) : OK
• Regular expression quality (inspirational, decent base, needs work, adequate or explain) : OK
• Documentation (needs work, adequate or explain) : OK
• Tests (none or some or adequate) : N/A  
• Verdict (Unusable, inspirational, usable, almost perfect or perfect) : Almost perfect

This is not meant to be final. As a CRS dev, feel free to comment below and edit this form directly. As committer or observer, feel free to comment below with feedback and we will think about updating the assessment accordingly.

[fzipi]

@lifeforms What's next here?

[RedXanadu]

@fzipi I think there are a couple of common Linux-y formats missing here (e.g. xz).

Is there a sensible way to add test cases here? The rule is commented-out by default :thinking:

[RedXanadu]

Actually, I see that the original bug bounty assessment said

• Tests (none or some or adequate) : N/A

so I guess we already concluded that this is impossible to test.

[RedXanadu]

Maybe this could be a useful source to use: https://en.wikipedia.org/wiki/List_of_archive_formats

[fzipi]

@gwen001 ☝️ ?

[RedXanadu]

@gwen001 How would you feel about adding the following file extensions:

.tbz2 .txz .cpio .iso .img .br .zst .cab .jar

which would bring the full list to:

.7z/ .br/ .bz/ .bz2/ .cab/ .cpio/ .gz/ .img/ .iso/ .jar/ .rar/ .tar/ .tbz2/ .tgz/ .txz/ .xz/ .zip/ .zst/

Also there's a question above about removing .axd.

Please note: If you're currently busy with other things and cannot continue with this issue, please let us know and we will take it over the finish line for you :)

[RedXanadu]

Oh, and we still need a new rule to make use of restricted_extensions_archives, right?

At the moment, restricted_extensions_archives is being defined but it isn't used anywhere (unless I've misunderstood).

Or use the previous suggestion of adding the archive list to the restricted_extensions variable:

⋮
setvar:'tx.restricted_extensions=%{tx.restricted_extensions} .7z/ .bz/…
⋮

Actually, did we come to a conclusion on that? @azurit @lifeforms: Did we want to handle archive file formats (.zip, .tar, etc.) at PL2 in a separate rule? Or just handle everything in the existing rule at PL1, and do setvar:'tx.restricted_extensions=%{tx.restricted_extensions} .7z/ .bz/… to optionally add the extra file extensions?

It still feels like potentially it would cause a lot of falss positives for a PL1 rule.

[RedXanadu]

No decision yet on how we want to implement this. Added to this evening's meeting agenda to discuss and decide. I can then resolve this next Monday (25 July).

[dune73]

@RedXanadu The best option was @azurit's proposal at https://github.com/coreruleset/coreruleset/pull/2562#issuecomment-1128841375

The .axd should still be part of the original list.

Where I am unsure is whether the restricted file extensions extension rule by azurit should be enabled by default, or disabled by default. That should indeed be discussed at the meeting. Thanks for adding. One idea could be to enable this extension rule in PL2 and above. That would be a new config pattern: Variable initialization based on PL.

Also: Adding additional file extensions as you propose is a good thing.

[azurit]

Where I am unsure is whether the restricted file extensions extension rule by azurit should be enabled by default, or disabled by default.

As for me, definitely not enabled by default. At least in PL1.

[RedXanadu]

Decision: definitely add .axd back in; add this logic as a stricter sibling at PL2, i.e. a separate rule (easier to disable, and doesn't need to implement a new concept of variable initialisation per PL.)

I'll implement this before the next meeting.

[dune73]

Meeting decision (https://github.com/coreruleset/project-chat-archive/blob/master/chat-archive-2022-07-18.md):

We'll go with a new rule at PL2 and a separate variable.

[RedXanadu]

Ah-hah: the .axd question is not as simple as it first appeared…

It looks like we specifically removed .axd here: #1925

It was removed from rules/REQUEST-901-INITIALIZATION.conf but is still referred to in crs-setup.conf.example.

Do we want to keep it removed?

[RedXanadu]

Meeting decision:

Remove .axd for good, and add a note explaining why.

Then flag this PR for merging!

[RedXanadu]

Note added, as agreed.

I believe this is good to be reviewed and merged, now.

[RedXanadu]

Do not merge.

I think this patch needs more thought and discussion.

[dune73]

Could you elaborate, please, @RedXanadu. When we talked about it on Monday, it all seemed settled.

[RedXanadu]

@dune73 Sorry, yes; to elaborate:

The more I think about this PR the more uneasy it makes me. This PR is now complete, but I think it's the wrong solution to the original problem.

When I re-read the original report for JW2SU88A, it seems to me that blocking archive files like .gz doesn't resolve the original issue at all. The original issue was that we don't catch things like:

curl --data 'cmd=gunzip -c /var/log/something_sensitive.gz' ...

Blocking the file extension .gz does not resolve that. The real problem is that we were missing some (de)compression utilities from our Unix command lists, like gunzip, which is now addressed in #2712.

So: I wonder how useful this PR would be? It feels like it's a separate issue: do we want to block file extensions like .gz, .xz etc. or will that just cause lots of false positives? What problem is solved by blocking .gz etc.?

There was an argument that "if you set up a CMS or something and accidentally leave the install/config tarball lying around then someone could download it", but is that a strong enough argument? I'm not sure.