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

Any controller will accept an uploaded file even if there's no POST method and put it in scratch/ #656

Open
RobertSwirsky opened this issue Oct 10, 2016 · 3 comments
Labels

Comments

@RobertSwirsky
Copy link

RobertSwirsky commented Oct 10, 2016

I tried uploading a file to the default / handler in my application with the following script. (cat.jpg is just a file sitting in the same directory as this script)

# upload.py tests file open vulnerability in Erlang "ChicagoBoss" framework
# use "poster" package https://atlee.ca/software/poster/
# pip install poster

from poster.encode import multipart_encode
from poster.streaminghttp import register_openers
import urllib2

# Register the streaming http handlers with urllib2
register_openers()

# Start the multipart/form-data encoding of the file "DSC0001.jpg"
# "cat" is the name of the parameter, which is normally set
# via the "name" parameter of the HTML <input> tag.

# headers contains the necessary Content-Type and Content-Length
# datagen is a generator object that yields the encoded parameters
datagen, headers = multipart_encode({"cat": open("cat.jpg", "rb")})

# Create the Request object
request = urllib2.Request("http://10.0.1.195/", datagen, headers)
# Actually do the request, and get the response
print urllib2.urlopen(request).read()

There's no POST handler at route /

-module(beakconsole_main_controller, [Req]).
-compile(export_all).

before_(_) ->
    user_lib:require_login(Req).

index('GET', [], ChannelMaster) ->
    % return the name of this node (node) and a list of all connected nodes (nodes)
    { ok, [{channelmaster, ChannelMaster}, {node, node()}, {nodes, nodes()}] }.

error404('GET', []) ->
    { ok, []}.

A scratch directory is created:

[famserve@localhost beakconsole]$ ls -lat
total 280
drwxr-xr-x.  9 famserve famserve   4096 Oct 10 14:55 .
drwxr-xr-x.  2 web      web       4096 Oct 10 14:55 scratch

and the file is there:

[famserve@localhost beakconsole]$ ls -l scratch/
total 24
-rw-r--r--. 1 web web 22491 Oct 10 14:55 63-138-82-204-201-145-151-170-221-154-79-155-142-101-27-104
[famserve@localhost beakconsole]$ 

It would probably be a good idea to at least check to see if there's a Post Method before allowing this.

Also, the user wasn't logged in, so the before_ method actually redirected to my login controller. And the file still made it up!

[swirsky@Thrills-iMac test (develop)]$ python upload.py 


<form method="post">
Name:<br />
<input name="name" /><br /><br />

Password:<br />
<input name="password" type="password" /><br /><br />

<input type="submit" value="Log in" />
<input type="hidden" name="redirect" value="/main" />
</form>

I think I can handle this in the before_, but I didn't expect this. I noticed it when some systems we have in production were collecting all sorts of "malware" in this scratch directory. (Fortunately, none of this PhP malware has any power in this directory and in the Erlang Universe.)

@RobertSwirsky RobertSwirsky changed the title Any controller will accept an uploaded file even if there's no POST method and put it in scractch/ Any controller will accept an uploaded file even if there's no POST method and put it in scratch/ Oct 10, 2016
@RobertSwirsky
Copy link
Author

RobertSwirsky commented Oct 13, 2016

I think this is mainly a SimpleBridge issue, but there may be away to control SimpleBridge so it doesn't do this.

The quickest workaround I found was to add a 'POST' handler that deletes the file.

@drobakowski
Copy link
Contributor

drobakowski commented Nov 5, 2016

Yeah this one also surprised me. The same behaviour does also apply to simple_bridge_handler_sample.erl, which could be easily reproduced with the simple_bridge example server:

> make run_inets
...
> curl -X GET http://127.0.0.1:8000/peer_ip -F 'file1=@<pathh_to_file>'
...
> ls -l scratch/
total 3520
-rw-r--r--  1 david  staff   1.7M Nov  5 15:44 78-200-77-73-91-82-21-199-130-108-240-188-19-104-115-117

Not sure if it's some kind of default case misbehaviour or maybe even intentional. Maybe @choptastic could be so kind and give us some details on how it's intended?

@chatterbeak we came up with another workaround which seems IMHO to be a little bit more complete and prevents from forgetting some controller endpoints. We're using a boss_filter which looks similar to this one:

-module(some_boss_filter).

-export([
    after_filter/3
]).

after_filter({StatusCode, Payload, Headers}, _FilterConfig, RequestContext) ->
    case lists:keyfind(request, 1, RequestContext) of
        {request, Request} ->
            handle_request_cleanup(Request);
        false ->
            lager:error("Couldn't find request in context for final data cleanup", [RequestContext])
    end,
    {StatusCode, Payload, Headers};
after_filter(Other, _FilterConfig, _RequestContext) ->
    %% Will match for 500er errors
    Other.

%%--------------------------------------------------------------------
%%% Internal functions
%%--------------------------------------------------------------------

handle_request_cleanup(Request) ->
    lists:foreach(fun(Attachment) ->
        {LogLevel, DeleteResult} =
        case file:delete(sb_uploaded_file:temp_file(Attachment)) of
            {error, enoent} ->
                %% The file was already deleted from business logic so only a debug log is needed
                {debug, file_already_deleted};
            OtherReturn ->
                {notice, OtherReturn}
        end,
        lager:log(LogLevel, self(), "Deleting remaining temporary file ~p with name ~p, size ~p byte and "
                                    "field_name ~p from scratch folder; result: ~p", [
            sb_uploaded_file:temp_file(Attachment),
            sb_uploaded_file:original_name(Attachment),
            sb_uploaded_file:size(Attachment),
            sb_uploaded_file:field_name(Attachment),
            DeleteResult
        ])
    end, Request:post_files()).

and in in the boss.config:

[{boss, [
    ...
    {controller_filter_modules, [
         some_boss_filter
    ]},
    ...
]}]

@RobertSwirsky
Copy link
Author

Thanks for the ideas.

I noticed this because various "malicious" php scripts were appearing in the scratch directory on some of my ChicagoBoss sites. Fortunately, the couldn't really do harm. But if you look at the constant barrage of PHP exploit attempts that any public website sees, it's common for these exploit suites to try uploading files to "/" and seeing if they can get them to run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants