search

exponentcms / exponent-cms

Content Management, Simple.
exponentcms.org
GNU General Public License v2.0
60 stars 24 forks source link

Security? Unrestricted File Upload Vulnerability in expFile.php #1445

Closed exponentcms closed 4 years ago

exponentcms commented 4 years ago

Allows unauthenticated users to upload executable files.

Example: curl -F "upload=@/attack.php" [exponent]/framework/modules/file/connector/uploader.php

exponentcms commented 4 years ago

This condition can NOT exist in recent versions, esp v2.4.0 plus patches since uploaded (php) files can NOT be executed (on apache based servers). Yes the file can be uploaded, but not executed.

exponentcms commented 4 years ago

I disagree. This condition exists right now on exponentcms.org.

I checked out the master branch https://github.com/exponentcms/exponent-cms/tree/master, installed it on Apache/2.4.18 (Ubuntu) running PHP 7.0.8 and was able to upload executable PHP files.

"AllowOverride" is set to "All".

Loaded Modules: core_module (static) so_module (static) watchdog_module (static) http_module (static) log_config_module (static) logio_module (static) version_module (static) unixd_module (static) access_compat_module (shared) alias_module (shared) auth_basic_module (shared) authn_core_module (shared) authn_file_module (shared) authz_core_module (shared) authz_host_module (shared) authz_user_module (shared) autoindex_module (shared) deflate_module (shared) dir_module (shared) env_module (shared) filter_module (shared) mime_module (shared) mpm_prefork_module (shared) negotiation_module (shared) php7_module (shared) setenvif_module (shared) status_module (shared)

Might be relevant: https://blog.remirepo.net/post/2013/01/13/PHP-and-Apache-SetHandler-vs-AddHandler

Actually, the .htaccess file shouldn't be necessary at all since you already have a whitelist of allowed extensions in the code.

But it doesn't work, due to errors e.g. in line 442 in framework/modules/file/models/expFile.php: $handle->file_new_name_ext = ''; which prevents checks for malicious file extensions later on in line 3051 in framework/modules/pixidou/includes/class.upload/class.upload.php: if (!is_null($this->file_new_name_ext)) { // rename file ext

Also, the (IMHO dangerous) .htaccess blacklist approach used by FilesMatch is missing PHP extensions such as ".php5".

exponentcms commented 4 years ago

First, the .php5 (&.php7) extension issue was actually addressed earlier today in the pre-release code for the next patch/release.

The class.upload.php is a 3rd party library used by the deprecated File Manager/Pixidou editor, which we piggy-backed on to perform image resize on upload (prior to the implementation of the new file manager elFinder). I also believe that the code in class.upload.php (to which you refer) is not used to filter filetypes, but to convert images and ensure the correct filetype is output? We do have a couple of different 3rd party libraries and even methods within expFile which might be suitable for our image resizing on upload (elFinder has it built-in)

Perhaps it would be good to add optional file upload by filetype blacklisting but it would also require tying it to several pieces of code to become a system wide implementation and prevent file uploads using the many methods we currently support.

exponentcms commented 4 years ago

Since I saw the recent commits:

The .htaccess file extension blacklist is not the right way to fix this vulnerability.

It might work for you / your specific apache configuration. But:

Conclusion: As long as you don't add your specific apache configuration to the installation requirements, blacklists just won't work. Whitelists are the only way to go (see e.g. Wordpress).

Regarding pixidou:

If configured properly, it matches file names against a build-in blacklist in an attempt to prevent executable files from being uploaded. See e.g. line 2945: preg_match('/.(php|php5|php4|php3|phtml|pl|py|cgi|asp|js)$/i', $this->file_src_name).

Still, as this is a blacklist and blacklists are generally insecure, it won't solve anything.

exponentcms commented 4 years ago

(from [0dd7790352cabef4637ac826f22755038ffac152]) integrate class.upload to help process all file uploads and filter executables; moves class.upload from pixidou to /external; filters uploads through elFinder to same mime types as class.upload; better error response to XHR upload [#1402 state:resolved] https://github.com/exponentcms/exponent-cms/commit/0dd7790352cabef4637ac826f22755038ffac152

exponentcms commented 4 years ago

Lighthouse URL: https://exponentcms.lighthouseapp.com/projects/61783/tickets/1402