5 ms·

Finding an arbitrary file upload vulnerability in a filesharing script

37 points by internetwache 9y ago

netsec_burn 9y ago
Nice find, I commented on your blog a method you can use to extend this vulnerability into RCE without admin interactivity.
- gruez 9y ago
  >Upload a .py file to the untrusted directory that has the same name of one of the imports used in the Python script. It will be executed the next time Python is called by the script, so you don't need to wait for interactivity on the admins part I have a feeling that only works on python2 because of absolute imports on python3.
  - deleted 9y ago
    [deleted]
    - deleted 9y ago
      [deleted]
  - gcr 9y ago
    Upload a script that replaces the old file source and wait for gunicorn or whatever to reload the app server.
    - gehaxelt 9y ago
      The script seems to ship with a rundimentary webserver by default, so I don't see a reason to wrap gunicorn et al. around it? However, you're right that overwriting the source is a possible attack vector.
- internetwache 9y ago
  Hi, thanks for commenting! I had cronjobs in mind, but as you said, they are not writeable. I also thought about uploading a .py file with an import's name, but wouldn't that require a __init__.py (in a subdirectory?) to be regarded as a valid module/import by python?
fake-name 9y ago
> I thought about a possible fix for a while, but in the end decided that the quickest and easiest fix would be to adjust the regular expressio ..... WHAT? Python has a suite of facilities exactly for this very kind of problem. Literally, the solution is "os.path.abspath(filename).startswith(os.path.abspath(dlfolder))" This should, in all cases, return true if the filename is within the download folder directory, and false for any other case.
- jwilk 9y ago
  It doesn't do the right thing if dlfolder doesn't end with a slash.
TempleOS 9y ago
it doesn't matter how long barney fife fucks around. God is perfectly just. God says... likableness dawdling immature vizier innovator toffee's hanker deployment's Rubin cusp's Mitty's frescoes phonologist cornstarch pacesetter curt pterodactyl's mason undoubtedly worked doc misfeasance's prudish engravers frocks debentures Sean's sleighing overslept chicory's comparability's warmest
based2 9y ago
https://www.owasp.org/index.php/Unrestricted_File_Upload https://www.owasp.org/index.php/Unrestricted_File_Upload
deleted 9y ago
[deleted]
jwilk 9y ago
Apparently the upload directory is the same as the script directory, which is not good. If you upload a file named "cgi.py", it will be imported next time someone runs the script. No directory traversal needed to exploit this.
- gehaxelt 9y ago
  Hi, I asssumed that the script would not be restarted, but you're totally right. Thanks for the comment :)
jwilk 9y ago
So I was curious what translate_path does: def translate_path(self, path): """Translate a /-separated PATH to the local filename syntax. Components that mean special things to the local file system (e.g. drive or directory names) are ignored. (XXX They should probably be diagnosed.) """ # abandon query parameters path = path.split('?',1)[0] path = path.split('#',1)[0] path = posixpath.normpath(urllib.unquote(path)) words = path.split('/') words = filter(None, words) path = os.getcwd() for word in words: drive, word = os.path.splitdrive(word) head, word = os.path.split(word) if word in (os.curdir, os.pardir): continue path = os.path.join(path, word) return path That... that doesn't make any sense.
- jwilk 9y ago
  Oh, and thanks to urllib.unquote, I can disguise slashes as "%2f", so the tightened regexp doesn't help.
  - jwilk 9y ago
    Never mind me, I misread how this function is used. Apparently this code was copied from stdlib, including a different directory traversal bug on Windows: https://bugs.python.org/issue26657 https://bugs.python.org/issue26657
ecdh 9y ago
This is hilarious -- I found this exact script and noted the exact same vulnerability last year. Good on you for posting about it.
- gehaxelt 9y ago
  Nice catch :) I thought the the amount of forks and stars warrants a short writeup.

37 points by internetwache 9y ago

Nice find, I commented on your blog a method you can use to extend this vulnerability into RCE without admin interactivity.

gruez 9y ago
>Upload a .py file to the untrusted directory that has the same name of one of the imports used in the Python script. It will be executed the next time Python is called by the script, so you don't need to wait for interactivity on the admins part I have a feeling that only works on python2 because of absolute imports on python3.
- deleted 9y ago
  [deleted]
  - deleted 9y ago
    [deleted]
- gcr 9y ago
  Upload a script that replaces the old file source and wait for gunicorn or whatever to reload the app server.
  - gehaxelt 9y ago
    The script seems to ship with a rundimentary webserver by default, so I don't see a reason to wrap gunicorn et al. around it? However, you're right that overwriting the source is a possible attack vector.
internetwache 9y ago
Hi, thanks for commenting! I had cronjobs in mind, but as you said, they are not writeable. I also thought about uploading a .py file with an import's name, but wouldn't that require a __init__.py (in a subdirectory?) to be regarded as a valid module/import by python?

fake-name 9y ago

> I thought about a possible fix for a while, but in the end decided that the quickest and easiest fix would be to adjust the regular expressio ..... WHAT? Python has a suite of facilities exactly for this very kind of problem. Literally, the solution is "os.path.abspath(filename).startswith(os.path.abspath(dlfolder))" This should, in all cases, return true if the filename is within the download folder directory, and false for any other case.

jwilk 9y ago
It doesn't do the right thing if dlfolder doesn't end with a slash.

TempleOS 9y ago

it doesn't matter how long barney fife fucks around. God is perfectly just. God says... likableness dawdling immature vizier innovator toffee's hanker deployment's Rubin cusp's Mitty's frescoes phonologist cornstarch pacesetter curt pterodactyl's mason undoubtedly worked doc misfeasance's prudish engravers frocks debentures Sean's sleighing overslept chicory's comparability's warmest

based2 9y ago

https://www.owasp.org/index.php/Unrestricted_File_Upload https://www.owasp.org/index.php/Unrestricted_File_Upload

deleted 9y ago

[deleted]

jwilk 9y ago

Apparently the upload directory is the same as the script directory, which is not good. If you upload a file named "cgi.py", it will be imported next time someone runs the script. No directory traversal needed to exploit this.

gehaxelt 9y ago
Hi, I asssumed that the script would not be restarted, but you're totally right. Thanks for the comment :)

jwilk 9y ago

So I was curious what translate_path does: def translate_path(self, path): """Translate a /-separated PATH to the local filename syntax. Components that mean special things to the local file system (e.g. drive or directory names) are ignored. (XXX They should probably be diagnosed.) """ # abandon query parameters path = path.split('?',1)[0] path = path.split('#',1)[0] path = posixpath.normpath(urllib.unquote(path)) words = path.split('/') words = filter(None, words) path = os.getcwd() for word in words: drive, word = os.path.splitdrive(word) head, word = os.path.split(word) if word in (os.curdir, os.pardir): continue path = os.path.join(path, word) return path That... that doesn't make any sense.

jwilk 9y ago
Oh, and thanks to urllib.unquote, I can disguise slashes as "%2f", so the tightened regexp doesn't help.
- jwilk 9y ago
  Never mind me, I misread how this function is used. Apparently this code was copied from stdlib, including a different directory traversal bug on Windows: https://bugs.python.org/issue26657 https://bugs.python.org/issue26657

ecdh 9y ago

This is hilarious -- I found this exact script and noted the exact same vulnerability last year. Good on you for posting about it.

gehaxelt 9y ago
Nice catch :) I thought the the amount of forks and stars warrants a short writeup.