Unverified Commit 6e32333b authored by Thom Wiggers's avatar Thom Wiggers 📐
Browse files

Protect photos.views._download from path traversal

Patches utils.snippets.sanitize_path and checks for the prefix of the path in the view

Closes #377
parent 85feadbc
import os import os
from django.core.exceptions import SuspiciousFileOperation
from django.conf import settings from django.conf import settings
from django.contrib.auth.decorators import login_required from django.contrib.auth.decorators import login_required
from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator
...@@ -107,10 +108,17 @@ def shared_album(request, slug, token): ...@@ -107,10 +108,17 @@ def shared_album(request, slug, token):
return _render_album_page(request, album) return _render_album_page(request, album)
def _download(request, path): def _download(request, original_path):
"""This function provides a layer of indirection for shared albums""" """This function provides a layer of indirection for shared albums"""
path = sanitize_path(path) photopath = os.path.join(settings.MEDIA_ROOT, 'photos')
path = os.path.join(settings.MEDIA_ROOT, 'photos', *path.split('/')[1:])
path = sanitize_path(original_path)
path = os.path.normpath(os.path.join(photopath, *path.split('/')[1:]))
if not os.path.commonprefix([photopath, path]).startswith(photopath):
raise SuspiciousFileOperation(
"Path traversal detected: someone tried to download "
"{}, input: {}".format(path, original_path))
if not os.path.isfile(path): if not os.path.isfile(path):
raise Http404("Photo not found.") raise Http404("Photo not found.")
return sendfile(request, path, attachment=True) return sendfile(request, path, attachment=True)
......
...@@ -34,6 +34,8 @@ def sanitize_path(path): ...@@ -34,6 +34,8 @@ def sanitize_path(path):
r""" r"""
Cleans up an insecure path, i.e. against directory traversal. Cleans up an insecure path, i.e. against directory traversal.
Still use os.path.commonprefix to check if the target is as expected
This code is partially copied from ``django.views.static``. This code is partially copied from ``django.views.static``.
>>> sanitize_path('//////') >>> sanitize_path('//////')
...@@ -45,10 +47,10 @@ def sanitize_path(path): ...@@ -45,10 +47,10 @@ def sanitize_path(path):
>>> sanitize_path('../.././test/') >>> sanitize_path('../.././test/')
'test' 'test'
>>> sanitize_path(r'..\..\..\test') >>> sanitize_path(r'..\..\..\test')
'test/' 'test'
""" """
path = os.path.normpath(unquote(path)) path = os.path.normpath(unquote(path).replace('\\', '/'))
path = path.lstrip('/') path = path.lstrip('/')
newpath = '' newpath = ''
for part in path.split('/'): for part in path.split('/'):
...@@ -60,7 +62,7 @@ def sanitize_path(path): ...@@ -60,7 +62,7 @@ def sanitize_path(path):
if part in (os.curdir, os.pardir): if part in (os.curdir, os.pardir):
# Strip '.' and '..' in path. # Strip '.' and '..' in path.
continue continue
newpath = os.path.join(newpath, part).replace('\\', '/') newpath = os.path.join(newpath, part)
return newpath return newpath
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment