Opened 9 years ago

Closed 9 years ago

Last modified 5 years ago

#25905 closed Cleanup/optimization (fixed)

Unsafe usage of urljoin() within FileStorageSystem

Reported by: Aman Ali Owned by: Tobias Kunze
Component: File uploads/storage Version: 1.9
Severity: Normal Keywords: file, storage
Cc: aali@…, aman.ali@…, bhch Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Aman Ali)

It may be possible to override the hostname when displaying a link to a static/media file with an attacker-controlled filename. The url() method in FileStorageSystem uses urljoin(base,url) to combine the base_url (typically STATIC_URL or MEDIA_URL) to the filename. The urljoin function has an edge case where if the url parameter starts with "//", the base_url value is overwritten. Thus, if an attacker can set the name variable of a FileStorageSystem instance to "//www.evil.com", any url method call on that instance will return an external link pointing to the attacker's site. Creating a file of the name "\\www.evil.com" is also acceptable as the filepath_to_uri function (that is called on the filename before the urljoin call) converts all backslashes to forward-slashes. The latter example works better as it is a completely valid Linux file name.

This issue can't be exploited using framework-provided upload techniques (FileFields, ImageFields, etc) as they properly escape the filename. However, an application may directly initialize a FileStorageSystem using user-controlled data or allow users to modify the name attribute of an existing FileStorageSystem instance. In such cases, an attacker could convert the expected relative paths into absolute external URLs.

This issue can simply be patched by modifying the line found herehttps://github.com/django/django/blob/master/django/core/files/storage.py#L302 as follows:

return urljoin(self.base_url, filepath_to_uri(name).replace('//','/'))

This change filters the filename provided through the get_valid_filename function from django.utils.text. This function does a sufficient job of eliminating the ability to override the base_url.

Note: This issue was initially disclosed to the Django security team and was decided not to be treated as a security issue, but instead a bug.

Change History (8)

comment:1 by Aman Ali, 9 years ago

Cc: aman.ali@… added
Description: modified (diff)

comment:2 by Tim Graham, 9 years ago

Component: Core (Other)File uploads/storage
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:3 by Aman Ali, 9 years ago

Description: modified (diff)

comment:4 by Tim Graham, 9 years ago

Needs tests: set

A test is also needed. If you could send a pull request after adding that, it's easiest for review purposes.

comment:5 by Tobias Kunze, 9 years ago

Needs tests: unset
Owner: changed from nobody to Tobias Kunze
Status: newassigned

comment:6 by Russell Keith-Magee <russell@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In fdf5cd34:

Fixed #25905 -- Prevented leading slashes in urljoin() calls

Leading slashes in the second urljoin argument will return exactly that

argument, breaking FileSystemStorage.url behavior if called with a

parameter with leading slashes.

Also added test cases for null bytes and None. Thanks to Markus for

help and review.

comment:7 by bhch, 5 years ago

Cc: bhch added

The current fix strips off all the leading slashes thereby making it a relative path, to which urljoin later prepends a base url. This removes the possibility of serving a default file from the static url.

I think a better solution would be to remove more than 1 leading slashes, but not one.

Current implementation: url.lstrip('/').

Proposed: re.sub(r'/{2,}', '/', url).

This will allow us to serve a default file from static url.

comment:8 by Tobias Kunze, 5 years ago

Thank you for your feedback – as this ticket has been closed some time ago, the best way forward would be to open a new ticket. There you could describe the behaviour you would like to see as opposed to the current one, and reference this ticket as introducing current behaviour. A new ticket will run through the proper ticket lifecycle and (more importantly) will get the attention of other developers.

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