tags:

views:

44

answers:

1

I'm using the following code to serve uploaded files from a login secured view in a django app.

Do you think that there is a security vulnerability in this code? I'm a bit concerned about that the user could place arbitrary strings in the url after the upload/ and this is directly mapped to the local filesystem.

Actually I don't think that it is a vulnerability issue, since the access to the filesystem is restricted to the files in the folder defined with the UPLOAD_LOCATION setting.

UPLOAD_LOCATION = is set to a not publicly available folder on the webserver

url(r'^upload/(?P<file_url>[/,.,\s,_,\-,\w]+)', 'project_name.views.serve_upload_files', name='project_detail'),

@login_required
def serve_upload_files(request, file_url):
    import os.path
    import mimetypes
    mimetypes.init()

    try:
        file_path = settings.UPLOAD_LOCATION + '/' + file_url
        fsock = open(file_path,"r")
        file_name = os.path.basename(file_path)
        file_size = os.path.getsize(file_path)
        print "file size is: " + str(file_size)
        mime_type_guess = mimetypes.guess_type(file_name)
        if mime_type_guess is not None:
            response = HttpResponse(fsock, mimetype=mime_type_guess[0])
        response['Content-Disposition'] = 'attachment; filename=' + file_name
        #response.write(file)             
    except IOError:
        response = HttpResponseNotFound()
    return response

EDIT: Updated the source according Ignacio Vazquez-Abrams comments:

 import os.path
 import mimetypes

  @login_required
  def serve_upload_files(request, file_url):
    mimetypes.init()
    try:
        file_path = os.path.join(settings.UPLOAD_LOCATION, file_url)
        #collapse possibly available up-level references
        file_path = os.path.normpath(file_path)
        #check if file path still begins with settings.UPLOAD_LOCATION, otherwise the user tampered around with up-level references in the url
        #for example this input: http://127.0.0.1:8000/upload/..\test_upload.txt results having the user access to a folder one-level higher than the upload folder
        #AND check if the common_prefix ends with a dir separator, Because although '/foo/barbaz' starts with '/foo/bar'
        common_prefix = os.path.commonprefix([settings.UPLOAD_LOCATION, file_path])
        if common_prefix == settings.UPLOAD_LOCATION and common_prefix.endswith(os.sep):
            fsock = open(file_path,"r")
            file_name = os.path.basename(file_path)
            mime_type_guess = mimetypes.guess_type(file_name)
            if mime_type_guess is not None:
                response = HttpResponse(fsock, mimetype=mime_type_guess[0])
                response['Content-Disposition'] = 'attachment; filename=' + file_name
            else:
                response = HttpResponseNotFound() 
        else:
            print "wrong directory"
            response = HttpResponseNotFound()           
    except IOError:
        response = HttpResponseNotFound()
    return response
+5  A: 

A few tips:

  1. Use os.path.join() to join the path together.
  2. Use os.path.normpath() to get the actual path with no ".." references.
  3. Use os.path.commonprefix() against UPLOAD_LOCATION and the generated path, and verify that the result starts with UPLOAD_LOCATION.
  4. Make sure that UPLOAD_LOCATION ends with a dir separator.

TL;DR: Use os.path.

Ignacio Vazquez-Abrams
Thanks, I will try these hints out and will paste the new code in my question. Why is Nr. 4 important?
Tom Tom
Because although '/foo/barbaz' starts with '/foo/bar', it is not under it.
Ignacio Vazquez-Abrams
I reworked the source. Looks much saver now. what do you think?
Tom Tom
That covers the security stuff. There's still style issues like moving the imports and init to the top of the file and combining the `if` s using `and`, but otherwise it looks good.
Ignacio Vazquez-Abrams
Changed it! Thanks a lot for your effort. Feels now much more secure with the reworked download view.
Tom Tom