wardi / django-filebrowser-no-grappelli

django-filebrowser for default Django admin site. Based on https://github.com/sehmaschine/django-filebrowser v3.1 (before it was in git) Consider using https://github.com/smacker/django-filebrowser-no-grappelli which is based on a newer version
Other
150 stars 114 forks source link

Sacyrity problem! directory listing. #37

Closed pahaz closed 12 years ago

pahaz commented 12 years ago

I can add "../" to directory paramets, and get all file listing.

wardi commented 12 years ago

Hmm no directory listing here but I can cause an unrelated error, which is worrying! Do you know if there is a fix upstream?

pahaz commented 12 years ago

Sorry, i can`t fix. But i can show how fix. Use this function! Then return absolute path if no Access error.

def _fix_listing(root, *path):
   path = os.path.join(root, *path)
   abspath = os.path.abspath(os.path.normpath(path))
   if not abspath.startswith(root):
       raise ValueError('Access Denited')
   return abspath
>>> _fix_listing("C:\\", "../../", "home")
'C:\\home'
>>> _fix_listing("C:\\dir", "../../", "home")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 5, in _fix_listing
ValueError: Access Denited
>>> _fix_listing("C:\\dir", "somedir/dir/../../", "home")
'C:\\dir\\home'
>>> _fix_listing("C:\\dir", "somedir/dir/../../home/")
'C:\\dir\\home'
>>> _fix_listing("C:\\dir", "somedir/dir/../../home")
'C:\\dir\\home'
wardi commented 12 years ago

I just pushed a fix like yours. Would you care to help me find other places that have the same problem? (uploads, directory creation, file version/thumbnail creation)

I tried to check the upstream code to see if the issue was fixed there, but the code has changed dramatically so it's not obvious to me.

pahaz commented 12 years ago

I am add some validations.

ROOT_DIR = os.path.abspath(os.path.join(fb_settings.MEDIA_ROOT, fb_settings.DIRECTORY))
# chroot directory for all file namitulation, and We must not allow to get access out of ROOT_DIR dirs.

I am add two function.

_check_access - Function for check access to path, and if access allow then return absolute path to file, else then raise exception (similar as we raise exception if path is None).

_check_name - this function for check correct naming dirs or files. Checking similar check in form (but in form check we can see problem with names as '../../name').

Maybe I misunderstood the logic of FOLDER_REGEX better not ispolzvat it.

This fix is needed for people who use django admin app for all users (no stuff). I have seen many such examples of the use django admin app.

I will try to reconsider the logic of correction, maybe I'm mistaken, during the week I will look again (sorry I did it in haste.)

And similar fix needed for admin app with grappelli.

I apologize for my English.

pahaz commented 12 years ago

If you really want to improve this app, you have to take any steps to resolve the issue. Because the vulnerability can at least look at the code project. And as the maximum gain full access to the operation system. I think you understand the danger of this problem.

wardi commented 12 years ago

Yes, this is a serious issue. I'll look at your other changes when I have a few minutes.

wardi commented 12 years ago

Your changes look pretty good. Creating a module-level ROOT_DIR does interfere with some dynamic sites monkeypatching I'm doing, so I'll make a small change there and do some testing.

pahaz commented 12 years ago

I am find real security error, when any user can upload any file in any dirs in os with the rights of web applications. Strange, veary strange.