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

Make sure `accept_file_types` is an INDEXED array, and disallow certain filetypes to be whitelisted #7768

Merged
merged 10 commits into from Apr 11, 2019

Blacklist some common PHP file extensions

  • Loading branch information...
bobdenotter committed Apr 3, 2019
commit 91187aef36363a870d60b0a3c1bf8507af34c9e4
@@ -308,6 +308,10 @@ protected function parseGeneral()
// as a regex-like string, and we switched to an array. If we find the old style, fall back to the defaults.
unset($general['accept_file_types']);
}
// Just.. Say no to these.
This conversation was marked as resolved by JarJak

This comment has been minimized.

Copy link
@JarJak

JarJak Apr 4, 2019

Member

Say yes to these and no to any others

This comment has been minimized.

Copy link
@bobdenotter

bobdenotter Apr 4, 2019

Author Member

That's not feasible.. the entire idea about the accept_file_types is that it is the whitelist of acceptible filetypes. We can impossibly list all the extensions that people might want to put in their own lists. Simply not feasible.

This actively removes a few known bad actors, as a minor extra layer to guard against mishaps. Just like in our allowed_tags we let the user set which tags are allowed, but regardless we remove <script> from that list. Same thing applies here.

This comment has been minimized.

Copy link
@xiaohutai

xiaohutai Apr 4, 2019

Member

(1)

@JarJak is correct, the resulting list is a list of file types that are allowed. A better comment would be something like:

// Remove problematic file types

or maybe:

// Remove blacklisted file types

(2)

The logic is incomplete: it removes php, but not PHP. You can add PHP to accept_file_types and then upload php files.

This comment has been minimized.

Copy link
@JarJak

JarJak Apr 5, 2019

Member

(2) it can just do case-insensitive check

This comment has been minimized.

Copy link
@bobdenotter

bobdenotter Apr 5, 2019

Author Member

In my latest version of this PR, this is a non-issue, because it's handled correctly :-)

$general['accept_file_types'] = array_diff($general['accept_file_types'], ['sh', 'asp', 'cgi', 'php', 'php3', 'ph3', 'php4', 'ph4', 'php5', 'ph5' ,'phtm', 'phtml']);
This conversation was marked as resolved by xiaohutai

This comment has been minimized.

Copy link
@xiaohutai

xiaohutai Apr 4, 2019

Member
  1. Isn't more transparent if there's only opt-in whitelist via accept_file_types in config.yml. Now there's a hard-coded blacklist in code that's not really apparent that it exists. If you really need these values I'd prefer defining these in config.yml too. But I don't see the point since a whitelist is most likely sufficient already.

If users do want to add php, isn't it really their fault/problem for opening the gates?

  1. I prefer arrays declarations to be like this:
[
    'sh',
    'asp',
    'cgi',
    'php',
    'php3',
    'ph3',
    'php4',
    'ph4',
    'php5',
    'ph5',
    'phtm',
    'phtml',
]

(one per line, with trailing comma)

Otherwise, there's an space/comma inconsitency at ph5' ,

-'ph5' ,'phtm',
+'ph5', 'phtm',

This comment has been minimized.

Copy link
@JarJak

JarJak Apr 4, 2019

Member

I agree that whitelist is better than blacklist

This comment has been minimized.

Copy link
@bobdenotter

bobdenotter Apr 4, 2019

Author Member

If users do want to add php, isn't it really their fault/problem for opening the gates?

It's about escalation. Allowing this could possibly enable would-be attackers to exploit another non-critical issue, pair it with this, and gain RCE. This is just one small extra layer for a little bit of extra security.

If we want to get into semantics: This is not blacklisting, it is actively removing some items from the whitelist, effectively making the whitelist more strict.

I reformatted it, to look slightly more readable.

This comment has been minimized.

Copy link
@xiaohutai

xiaohutai Apr 4, 2019

Member

I usually prefer one line per item just for adding/removing items not marking everything at the same time (also easier to blame). But in hindsight, that might be a bit overkill for this rather simple list.

I'll resolve this unless @JarJak finds this explode solution unacceptable 😂

// accept uppercase and lowercase file extensions.
$general['accept_file_types'] = array_values(
This conversation was marked as resolved by JarJak

This comment has been minimized.

Copy link
@JarJak

JarJak Apr 4, 2019

Member

use collection (bolt has one)

This comment has been minimized.

Copy link
@bobdenotter

bobdenotter Apr 4, 2019

Author Member

@JarJak You could have done this when you introduced this bit of code in #7751. I'm fine with it the way it is now.

This comment has been minimized.

Copy link
@JarJak

JarJak Apr 5, 2019

Member

Yes, you are right.. I just didn't know Bolt have it's own Collection implementation when I did this :)

This comment has been minimized.

Copy link
@bobdenotter

bobdenotter Apr 5, 2019

Author Member

Ah, ok.. It's been taken care of now. :-)

array_unique(
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.