Opened 15 years ago

Closed 15 years ago

Last modified 2 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: no UI/UX: no

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 15 years ago.
10188.diff (2.2 KB ) - added by Bob Thomas 15 years ago.
Patch with doc and tests

Download all attachments as: .zip

Change History (16)

by Bob Thomas, 15 years ago

Attachment: badheader.diff added

comment:1 by Rob Hudson <treborhudson@…>, 15 years ago

Component: UncategorizedHTTP handling

comment:2 by rokclimb15, 15 years ago

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 by Jacob, 15 years ago

milestone: 1.1
Triage Stage: Design decision neededAccepted

comment:4 by Bob Thomas, 15 years ago

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

comment:5 by rokclimb15, 15 years ago

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 by mcroydon, 15 years ago

Needs documentation: set

BadHeaderError should be documented.

by Bob Thomas, 15 years ago

Attachment: 10188.diff added

Patch with doc and tests

comment:7 by Bob Thomas, 15 years ago

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 by Jacob, 15 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Jacob, 15 years ago

Resolution: fixed
Status: newclosed

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

comment:10 by Jacob, 15 years ago

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

comment:11 by anonymous, 15 years ago

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 by Alex Gaynor, 15 years ago

Resolution: fixed
Status: reopenedclosed

Fixed in r10712.

comment:13 by Jacob, 12 years ago

milestone: 1.1

Milestone 1.1 deleted

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Easy pickings: unset
UI/UX: unset

In 9bde906:

Refs #10188 -- Added tests for BadHeaderErrors when HTTP header with newlines cannot be encoded/decoded.

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