Opened 15 years ago

Closed 12 years ago

#10147 closed Bug (needsinfo)

Possible wrong check in django.utils._os

Reported by: Moorthy RS Owned by: nobody
Component: File uploads/storage Version: 1.0
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have a model that accepts an Image. And I have specified the upload_dir to /home/myproject/media/upload. And whenever I try to save an image, it gives an error "SuspiciousOperation: Attempted access to /home/myproject/media/upload/image.png denied". Problem appears only when used with Apache, but with the development server, there were no errors. I searched and found that the only suggestion was a possible missing or extra "/" separator. I tried specifying without and with the leading slash, but with no luck.

I debugged around and found this code, responsible for the error:

    if not final_path.startswith(base_path) \
       or final_path[base_path_len:base_path_len+1] not in ('', sep):
        raise ValueError('the joined path is located outside of the base path'
                         ' component')

I found base_path was a "/" and final_path was "/home/myproject/media/upload". And hence "final_path.startswith(base_path)" returns true and "final_path[base_path_len:base_path_len+1] returns 'h' which IS not in empty string or sep. So the if condition should fail, but it was succeeding. I changed the code to this snippet (by adding appropriate paranthesis like "if not (condition)") and it started working correctly!

    if not (final_path.startswith(base_path) \
       or final_path[base_path_len:base_path_len+1] not in ('', sep)):
        raise ValueError('the joined path is located outside of the base path'
                         ' component')

I am a newbie in python (and as well in django), but I see the lack of paranthesis has got the precedence going wrong, and an unexpected result.

This needs to be corrected, as indicated above.

Change History (5)

comment:1 by Karen Tracey, 15 years ago

Has patch: unset

No, the problem is not caused by missing parentheses. Consider the comment above that code block, which explains what the check is doing:

    # Ensure final_path starts with base_path and that the next character after
    # the final path is os.sep (or nothing, in which case final_path must be
    # equal to base_path).
    if not final_path.startswith(base_path) \
       or final_path[base_path_len:base_path_len+1] not in ('', sep):
        raise ValueError('the joined path is located outside of the base path'
                         ' component')

So, the code wishes to proceed with the all-clear if: [A and B]

Negating that to check for the error condition is: [not (A and B)], or equivalently [(not A) or (not B)], which is how it is coded.

Your proposed change is to make that [not (A or B)], which is not the same thing at all. Consider how it would work with some paths other than base_path="/", as compared to the original:

>>> # check1 -- as originally implemented
...
>>> def check1(base_path, final_path):
...    base_path_len = len(base_path)
...    if not final_path.startswith(base_path) \
...       or final_path[base_path_len:base_path_len+1] not in ('', '/'):
...       raise Exception('joined path is located outside of base path')
...    return 'OK'
...
>>> # final path being somewhere under base path is OK
...
>>> check1("/home/myproject", "/home/myproject/file1.gif")
'OK'
>>> # final path being entirely outside of base path is not OK
...
>>> check1("/home/myproject", "/home/yourproject/file1.gif")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 5, in check1
Exception: joined path is located outside of base path
>>> # not OK even if the names start with the same prefix -- the final path must
... # have a path seperator after the end of the common part to be "under" base path
...
>>> check1("/home/myproject", "/home/myproject_alt/file1.gif")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 5, in check1
Exception: joined path is located outside of base path
>>>
>>> # check2 -- with proposed change
...
>>> def check2(base_path, final_path):
...    base_path_len = len(base_path)
...    if not (final_path.startswith(base_path) \
...       or final_path[base_path_len:base_path_len+1] not in ('', '/')):
...       raise Exception('joined path is located outside of base path!')
...    return 'OK'
...
>>> # final path being somewhere under base path is OK
...
>>> check2("/home/myproject", "/home/myproject/file1.gif")
'OK'
>>> # OK, that one worked
...
>>> # final path being entirely outside of base path is not OK
...
>>> check2("/home/myproject", "/home/yourproject/file1.gif")
'OK'
>>> # Wrong answer!
...
>>> # not OK even if the names start with the same prefix -- the final path must
... # have a path seperator after the end of the common part to be "under" base path
...
>>> check2("/home/myproject", "/home/myproject_alt/file1.gif")
'OK'
>>> # Wrong answer!
...
>>>

The problem you have found has to do with how the code behaves specifically when base_path="/", which is not really a normal case, so I'm not entirely sure what should be done about it. Apparently you have MEDIA_ROOT set to "/"? That means you are saying it is OK for web users to upload files anywhere in the file system. Are you sure you want to be doing that?

in reply to:  1 comment:2 by Karen Tracey, 15 years ago

Replying to kmtracey:

Your proposed change is to make that [not (A or B)], which is not the same thing at all. Consider how it would work with some paths other than base_path="/", as compared to the original:

Typo here, the proposed change is to [not (A or not B)], still not the same thing.

comment:3 by Jacob, 15 years ago

Triage Stage: UnreviewedDesign decision needed

comment:4 by Chris Beaven, 13 years ago

Severity: Normal
Type: Bug

comment:5 by Jacob, 12 years ago

Easy pickings: unset
Resolution: needsinfo
Status: newclosed
UI/UX: unset

I think we need more info from the original poster to know if it is indeed MEDIA_ROOT = / causing this, or something else.

rsmoorthy, if you can provide more information (responding to kmtracy's questions) then please feel free to re-open.

Thanks!

Note: See TracTickets for help on using tickets.
Back to Top