Opened 7 years ago

Closed 4 years ago

#10147 closed Bug (needsinfo)

Possible wrong check in django.utils._os

Reported by: rsmoorthy 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 follow-up: Changed 7 years ago by kmtracey

  • Has patch unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement 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?

comment:2 in reply to: ↑ 1 Changed 7 years ago by kmtracey

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 Changed 6 years ago by jacob

  • Triage Stage changed from Unreviewed to Design decision needed

comment:4 Changed 4 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 4 years ago by jacob

  • Easy pickings unset
  • Resolution set to needsinfo
  • Status changed from new to closed
  • 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