Opened 3 years ago

Closed 3 years ago

#24239 closed Bug (duplicate)

Possible problem in CVE-2015-0219 fix for 1.4.x

Reported by: Raphaël Hertzog Owned by: nobody
Component: HTTP handling Version: 1.4
Severity: Normal Keywords:
Cc: carl@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Looking at https://github.com/django/django/commit/4f6fffc1dc429f1ad428ecf8e6620739e8837450 you will see that the commit adds a get_environ() method to the WSGIRequestHandler class.

But in fact, the class already has such a method, so this change effectively replaces the initial get_environ() implementation with another one that relies on ancestors providing the bulk of the code while the original implementation does everything alone.

I would have expected this to blow up in some interesting ways... I'm not using 1.4 myself so I haven't verified. I discovered this while trying to backport this patch for an even older version of Django (1.2 in Debian Squeeze).

Attachments (1)

0001-1.4.x-Fixed-24239-merge-both-WSGIRequestHandler.get_.patch (2.3 KB) - added by Raphaël Hertzog 3 years ago.
Suggested patch

Download all attachments as: .zip

Change History (6)

Changed 3 years ago by Raphaël Hertzog

Suggested patch

comment:1 Changed 3 years ago by Raphaël Hertzog

Has patch: set

comment:2 Changed 3 years ago by Carl Meyer

Component: UncategorizedHTTP handling
Triage Stage: UnreviewedAccepted

Hi - thanks for the report! This was a mistake in backporting that patch to 1.4. However, I'm pretty sure in practice it's harmless - the custom implementation of get_environ() there has been unnecessary for a long time (in fact, I'm not sure why it was ever necessary at all). It was simply removed (without requiring any compensating changes) in https://github.com/django/django/commit/681550ca6d5ff3e591a769643e37fd0b50f29c91, which is why it doesn't exist in more recent Django versions. Relying on the standard-library get_environ() implementation is better.

If this issue had been caught before the release, I would say your fix is the right one, for stability reasons. But now that we have a 1.4 release in the wild which effectively is using the stdlib's get_environ() anyway (and nobody has reported a practical problem with that, nor would I expect any), I think we should just remove the custom get_environ() in the 1.4.x branch.

comment:3 Changed 3 years ago by Carl Meyer

For historical interest - the custom copy of get_environ() was introduced in the earliest days of Django (in https://github.com/django/django/commit/b68c478a), in the initial commit that created runserver. There was no explanation in that commit of why the method was copied. I suppose (again, just for historical interest) one could compare it with the get_environ() method present in whatever stdlib was current in July 2005 to see what, if any, changes Adrian made to it.

comment:4 Changed 3 years ago by Carl Meyer

Actually, scratch that, the reason is clear. In July 2005, wsgiref was a third-party library, it wasn't in the stdlib yet. So Adrian was just copying code from it - at that time Django's WSGIRequestHandler inherited from BaseHTTPHandler, which didn't have a get_environ(), of course. So the custom get_environ() method never existed in order to correct any issue in wsgiref - it was a straight-up copy and has been superfluous ever since Django's WSGIRequestHandler began inheriting from wsgiref.simple_server.WSGIRequestHandler.

comment:5 Changed 3 years ago by Tim Graham

Resolution: duplicate
Status: newclosed

Removed the copied method as suggested in #24238.

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