Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#22486 closed Bug (fixed)

urlresolvers.reverse() security fix (1.5.6) breaks when a view is a partial

Reported by: rcoup Owned by: timo
Component: Core (URLs) Version: master
Severity: Release blocker Keywords:
Cc: luke@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The changes in the recent security fix for reverse() Fixed a remote code execution vulnerabilty in URL reversing (CVE-2014-0472) break when any view in the urlpatterns is a functools partial.

Django 1.5.6 / Py2.6.5 / Linux.

pseudo-code:

# myapp/views.py
def my_view(request, **kwargs):
     return HttpResponse(str(kwargs))

my_view2 = functools.partial(my_view, some_arg=123)

# myapp/urls.py
urlpatterns = patterns('my_app.views',
   ('^some_view/$', 'normal_view'),  # a normal function/class view
   ('^breaks/$', 'my_view2'),        # partial-wrapped view
)

Then

>>> from django.core.urlresolvers import reverse
>>> reverse('my_app.some_view')
django/core/urlresolvers.pyc in reverse(viewname, urlconf, args, kwargs, prefix, current_app)
    514             resolver = get_ns_resolver(ns_pattern, resolver)
    515
--> 516     return iri_to_uri(resolver._reverse_with_prefix(view, prefix, *args, **kwargs))
    517
    518 reverse_lazy = lazy(reverse, str)

django/core/urlresolvers.pyc in _reverse_with_prefix(self, lookup_view, _prefix, *args, **kwargs)
    393
    394         if not self._populated:
--> 395             self._populate()
    396
    397         try:

django/core/urlresolvers.pyc in _populate(self)
    285                 else:
    286                     parent = normalize(pattern.regex.pattern)
--> 287                     for name in pattern.reverse_dict:
    288                         for matches, pat, defaults in pattern.reverse_dict.getlist(name):
    289                             new_matches = []

django/core/urlresolvers.pyc in reverse_dict(self)
    310         language_code = get_language()
    311         if language_code not in self._reverse_dict:
--> 312             self._populate()
    313         return self._reverse_dict[language_code]
    314

django/core/urlresolvers.pyc in _populate(self)
    271                 callback = pattern._callback
    272                 if not hasattr(callback, '__name__'):
--> 273                     lookup_str = callback.__module__ + "." + callback.__class__.__name__
    274                 else:
    275                     lookup_str = callback.__module__ + "." + callback.__name__

AttributeError: 'functools.partial' object has no attribute '__module__'

Changing the view to use functools.update_wrapper() (as it probably should) and it works okay:

my_view2 = functools.partial(my_view, some_arg=123)
functools.update_wrapper(my_view2, my_view)

Change History (11)

comment:1 Changed 12 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Are you suggesting a change in Django itself or would a documentation update that notes this restriction be sufficient?

comment:2 Changed 12 months ago by prestontimmons

Although partials aren't reversible by the "path.to.view" syntax, I don't think Django should blow up on them. Some parts of my work code base use partials extensively.

A simple fix might be to create lookup_str from the original function whenever a pattern callback is a partial. The behavior would end up the same as if update_wrapper were called.

A possible patch is here: https://github.com/django/django/pull/2601/

What do you think?

Last edited 12 months ago by prestontimmons (previous) (diff)

comment:3 Changed 12 months ago by timo

  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.5 to 1.4

Looks good. We should probably backport this to all the branches where we applied the security fix.

comment:4 Changed 12 months ago by lfaraone

  • Cc luke@… added

comment:5 Changed 12 months ago by anonymous

  • Severity changed from Normal to Release blocker
  • Version changed from 1.4 to master

comment:6 Changed 12 months ago by timo

  • Owner changed from nobody to timo
  • Status changed from new to assigned

comment:7 Changed 12 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 3c06b2f2a341922db4294b91117c7b1c34119a8c:

Fixed #22486 -- Restored the ability to reverse views created using functools.partial.

Regression in 8b93b31487d6d3b0fcbbd0498991ea0db9088054.

Thanks rcoup for the report.

comment:8 Changed 12 months ago by Tim Graham <timograham@…>

In e192f131033d68e1b018263aee6fe8804a923b22:

[1.7.x] Fixed #22486 -- Restored the ability to reverse views created using functools.partial.

Regression in 8b93b31487d6d3b0fcbbd0498991ea0db9088054.

Thanks rcoup for the report.

Backport of 3c06b2f2a3 from master

comment:9 Changed 12 months ago by Tim Graham <timograham@…>

In 6915220ff9d6eeb2a669421d06bce9403ed6480c:

[1.6.x] Fixed #22486 -- Restored the ability to reverse views created using functools.partial.

Regression in 8b93b31487d6d3b0fcbbd0498991ea0db9088054.

Thanks rcoup for the report.

Backport of 3c06b2f2a3 from master

comment:10 Changed 12 months ago by Tim Graham <timograham@…>

In 19bd6b9477e8f09f867640f72f3eb335cebe1d6a:

[1.5.x] Fixed #22486 -- Restored the ability to reverse views created using functools.partial.

Regression in 8b93b31487d6d3b0fcbbd0498991ea0db9088054.

Thanks rcoup for the report.

Backport of 3c06b2f2a3 from master

comment:11 Changed 12 months ago by Tim Graham <timograham@…>

In b91c385e324f1cb94d20e2ad146372c259d51d3b:

[1.4.x] Fixed #22486 -- Restored the ability to reverse views created using functools.partial.

Regression in 8b93b31.

Thanks rcoup for the report.

Backport of 3c06b2f2a3 from master

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