Skip to content

Commit

Permalink
Fix a possible security risk presented by path traversal.
Browse files Browse the repository at this point in the history
  • Loading branch information
@rnevet
rnevet committed Feb 12, 2018
1 parent fcd3fbb commit d64475c
Showing 1 changed file with 11 additions and 1 deletion.
12 changes: 11 additions & 1 deletion modules/WebServer.py
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# coding=utf-8
import threading
import os

server = None
web_server_ip = "0.0.0.0"
Expand Down Expand Up @@ -53,14 +54,23 @@ def start_web_server():

# Do not attempt to fix code warnings in the below class, it is perfect.
class QuietHandler(SimpleHTTPServer.SimpleHTTPRequestHandler):
real_server_path = os.path.realpath(web_server_template)

# quiet server logs
def log_message(self, format, *args):
return

# serve from www folder under current working dir
# serve from web_server_template folder under current working dir
def translate_path(self, path):
return SimpleHTTPServer.SimpleHTTPRequestHandler.translate_path(self, '/' + web_server_template + path)

def send_head(self):
local_path = self.translate_path(self.path)
if os.path.commonprefix((os.path.realpath(local_path), self.real_server_path)) != self.real_server_path:
self.send_error(404, "These aren't the droids you're looking for")
return None
return SimpleHTTPServer.SimpleHTTPRequestHandler.send_head(self)

global server
SocketServer.TCPServer.allow_reuse_address = True
server = SocketServer.TCPServer((host, port), QuietHandler)
Expand Down

13 comments on commit d64475c

@mazhead
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this commit the bot (bitfinex) will not operate. Will end up only with the 404 mentioned above.
I am not able to pintpoint what exactly it is breaking but reverting to a commit before this one fixes it.

@M-Igashi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change really needed? It makes webserver stop working. Please review again.

@rnevet
Copy link
Collaborator Author

@rnevet rnevet commented on d64475c Mar 10, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is needed, without it the webserver allows you to access the Exchange secret and keys. This has nothing to do with which exchange you are using it on.

can you provide information regarding your environment to try and reproduce the issue you are facing?

@utdrmac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the exploit? I just tried accessing my default.cfg and it says 404.

@M-Igashi
Copy link
Contributor

@M-Igashi M-Igashi commented on d64475c Mar 11, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review the code again, many people including me suffer from webserver error which this PR merge.

Ubuntu16.04, running 2 docker images for bitfinex and poloniex.
With this code set, webserver returns "These aren't the droids you're looking for".

@mazhead
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better explanation what this was intended to fix would be needed. I personally run just one docker image with the bot on local lan without ngnix in front.

@M-Igashi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's due to symbolic link lost in os.path.realpath on multi market setting. I'm not quite sure this os import is appropriate.

@M-Igashi
Copy link
Contributor

@M-Igashi M-Igashi commented on d64475c Mar 11, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made PR to fix the problem. abspath should be fine to resolve the path in this security patch context. Tested in multi market config file and separate docker images, and it works good.

@rnevet
Copy link
Collaborator Author

@rnevet rnevet commented on d64475c Mar 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mazhead @utdrmac could you check the PR to see if it resolves the issue you are facing?
#615

@utdrmac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rnevet I'm not experiencing any issues. You said this change is needed but I don't see a use case where the exploit is an issue. How are you viewing API keys under the current code base?

@mazhead
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest version works.
Thank you both @rnevet and @M-Igashi !

@M-Igashi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@utdrmac I reviewed the code during making PR, and found the original .py code referred the template path by wildcard relative path finder to www directory, which may potentially face exploitation. Then I was convinced that this patch was necessary.

@rnevet
Copy link
Collaborator Author

@rnevet rnevet commented on d64475c Mar 12, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@utdrmac basically by using relative path inside the url you could reach parent directories, I managed to reproduce this before patching it. If you wish to have further details ping me on chat.

@M-Igashi Thanks for the PR!

Please sign in to comment.