Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#10188 closed (fixed)

HttpResponse does not filter CR/LF characters from headers

Reported by: Bob Thomas Owned by: rokclimb15
Component: HTTP handling Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

E-mail headers currently throw an exception when a CR or LF character is put into a header (see forbid_multi_line_headers in django/core/mail.py). HttpResponse should have the same or equivalent protection in place, probably in _convert_to_ascii. A basic patch based on forbid_multi_line_headers attached.

Attachments (2)

badheader.diff (827 bytes) - added by Bob Thomas 8 years ago.
10188.diff (2.2 KB) - added by Bob Thomas 8 years ago.
Patch with doc and tests

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by Bob Thomas

Attachment: badheader.diff added

comment:1 Changed 8 years ago by Rob Hudson <treborhudson@…>

Component: UncategorizedHTTP handling

comment:2 Changed 8 years ago by rokclimb15

Needs tests: set
Owner: changed from nobody to rokclimb15
Triage Stage: UnreviewedDesign decision needed

I think this is an excellent idea from a security perspective. This will eliminate the possibility of HTTP response splitting in Django apps. I will test this patch over the weekend and seek a design decision on whether or not it should be included.

comment:3 Changed 8 years ago by Jacob

milestone: 1.1
Triage Stage: Design decision neededAccepted

comment:4 Changed 8 years ago by Bob Thomas

I discussed security implications with Jacob already; I'd rather not add any details here :)

comment:5 Changed 8 years ago by rokclimb15

I have installed this patch on my production web server running many sites. No hiccups or 500 emails so far. Working on a couple of unit tests for this patch.

comment:6 Changed 8 years ago by mcroydon

Needs documentation: set

BadHeaderError should be documented.

Changed 8 years ago by Bob Thomas

Attachment: 10188.diff added

Patch with doc and tests

comment:7 Changed 8 years ago by Bob Thomas

Needs documentation: unset
Needs tests: unset

Added documentation and tests. I can't build the documentation right now, so I would appreciate it if someone else could check it.

comment:8 Changed 8 years ago by Jacob

Triage Stage: AcceptedReady for checkin

comment:9 Changed 8 years ago by Jacob

Resolution: fixed
Status: newclosed

(In [10709]) Fixed #10188: prevent newlines in HTTP headers. Thanks, bthomas.

comment:10 Changed 8 years ago by Jacob

(In [10711]) Fixed #10188: prevent newlines in HTTP headers. Thanks, bthomas.

comment:11 Changed 8 years ago by anonymous

Patch needs improvement: set
Resolution: fixed
Status: closedreopened

The patch seems to be incorrect since it's not checking whether value is iterable or not (which throws an exception).

Replacing

if '\n' in value or '\r' in value: 

with

if isinstance(value, str) and ('\n' in value or '\r' in value):

works for me.

comment:12 Changed 8 years ago by Alex Gaynor

Resolution: fixed
Status: reopenedclosed

Fixed in r10712.

comment:13 Changed 5 years ago by Jacob

milestone: 1.1

Milestone 1.1 deleted

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