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

[SECURITY] Web Shell Upload #979

Closed
ybelenko opened this issue May 28, 2019 · 13 comments

Comments

@ybelenko
Copy link
Contributor

commented May 28, 2019

Feature Request

About audited Directus version.
It has been cloned from suite repo.
Latest commit directus/directus@1d151a9

Description:

An attacker can exploit the file upload function (which lacks efficient restrictions and validations) by uploading web shell to uploads storage directory. Thus, the attacker can take control of the accounts and workstations of application users and administrators and the server itself.

The attacker does not need a very high level of technological knowledge to perform this attack. However, he does need specially crafted malicious files.

Business risk:

A malicious user can complete take over the application and the server.

Technical details:

The upload functionality allows to a user to upload image as a avatar in its profile, however because there are no any validations of the uploaded file types and because of uploaded directory has a execute permissions this functionality can be abused by a malicious user in order to upload web shell and take over the whole system.

In the screenshots bellow it is possible to see that web shell file was uploaded to the sever via avatar upload functionality. As it is possible to see an attacker was able to run commands on the backend server.

As the application sets cookie PHPSESSID – it is possible to understand that the server-side programming language is PHP.

img1

It was also possible to see that every user has a direct access to the uploaded file:

img2

And

img3

Once, an attacker knows which type of shell he needs the next step is uploading web shell itself:

img4

It is possible to see that an attacker is able to run commands on the backend server itself, thus can completely compromise the server.

What problem does this feature solve?

Fixes security hole.

How do you think this should be implemented?

  • The uploaded file types must be restricted only to the necessary file types and be validated on the server side.
  • Uploaded directory should not have any execute permission and all the script handlers should be removed from these directories.

Would you be willing to work on this?

Maybe, with help/guidance from Directus team.

@benhaynes benhaynes added this to the Priority Features milestone May 28, 2019

@ybelenko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

After some investigation it turns out that file permissions doesn't forbid browser execution. I've changed permissions of uploaded php file with chmod('shell.php', 0600). I did this manually via FTP also. shell.php file is still accessable and still executes php code.

It's weird that this lines in .htaccess doesn't disable php execution:

<FilesMatch "\.(php|phps|php5|htm|shtml|xhtml|cgi.+)$">
ForceType text/plain
</FilesMatch>

First solution from web search is How to show PHP files as plain text in Apache. So I tried that:

<FilesMatch "\.(php|phps|php5|htm|shtml|xhtml|cgi.+)$">
    php_flag engine off 
    ForceType text/plain
</FilesMatch>

PHP works. Next I checked precise example from post:

<FilesMatch "\.(php|phps|php5|htm|shtml|xhtml|cgi.+)$">
    php_flag engine off 
    #This will prevent apache from executing *.php-files

    AddType text/plain php
    #this wil display php-files in browser (if not, browser will want to download file!)
</FilesMatch>

Doesn't help, PHP executes. As last solution I've forbid access to PHP files:

<FilesMatch "\.(php|phps|php5|htm|shtml|xhtml|cgi.+)$">
    Order Deny,Allow
    Deny from All
</FilesMatch>

This one gives 403 forbidden response and finally PHP script hasn't been executed in browser.

@pgl

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Also, that <FilesMatch directive should be updated to include .phtml. It's deprecated as a suffix, but a lot of people still have their servers configured like this.

And, what about Nginx or other servers?

pgl added a commit to pgl/directus-api that referenced this issue Jun 26, 2019

bjgajjar added a commit that referenced this issue Jun 27, 2019

@bjgajjar bjgajjar added this to Done in v2.2.1 Jun 27, 2019

@pgl

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

Hey, sorry, but this needs to stay open as the issue is still there for Nginx and other servers that don't support .htaccess files. Really there needs to be some proper validation on upload. (But also the .htaccess file needed updating as well.)

@bjgajjar

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@pgl Thanks for the clarification but our main support is apache only. So, for now, I guess we can consider this as a close.

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

It would be nice to track these alt-stack issues in a way that doesn't clog up our normal ticket flow. We can use the label, close them, and add them to a project board (similar to Feature Requests).

Thoughts @bjgajjar @rijkvanzanten ?

@rijkvanzanten

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

that works for me 👍

@mimamuh

This comment has been minimized.

Copy link

commented Jun 28, 2019

PS: Your current docker setup runs on nginx, not apache. Even if it is stale and will be overdone I just wanted to let you know. People use it as a reference to probably build their own setup based on that.

@pgl

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

The documentation also doesn't mention that Apache is the only fully supported setup method.

@rijkvanzanten

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Directus is developed and tested on the LAMP stack, and therefore this is the only officially supported environment. Alternate stacks (NGINX, Caddy, MariaDB, Percona, etc) may also work but you should proceed at your own risk

https://docs.directus.io/getting-started/installation.html#requirements

Re: Docker, I believe the updated docker setup will rely on Apache (@WoLfulus?)

@rijkvanzanten

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Seeing that static files are served from the webserver directly–which means it bypasses the Directus API–there's nothing we can do in the API itself to mitigate this issue for every possible webserver 🤔

Would there be any possibility to have a PHP endpoint for files instead of relying directly on the webserver @directus/api-team? That way we might be able to fix it regardless of webserver.

Generally speaking: the less reliance on Apache proprietary stuff the better

@pgl

This comment has been minimized.

@pgl

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

I think a proper solution would be filtering files on upload - have a whitelist of allowed filetypes, basically.

@benhaynes

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

I think passing all files through the backend/API would give us a lot of control and flexibility — but we'd need to make absolutely sure that this wouldn't add too much overhead, latency, issues, etc. Also, it would be nice if this could be bypassed to access files traditionally (if all are set to public).

Thanks @pgl — I've updated the intro to that page to be more clear! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants
You can’t perform that action at this time.