Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#10188 closed (fixed)

HttpResponse does not filter CR/LF characters from headers

Reported by: bthomas 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 bthomas 7 years ago.
10188.diff (2.2 KB) - added by bthomas 6 years ago.
Patch with doc and tests

Download all attachments as: .zip

Change History (15)

Changed 7 years ago by bthomas

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

  • Component changed from Uncategorized to HTTP handling
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 7 years ago by rokclimb15

  • Needs tests set
  • Owner changed from nobody to rokclimb15
  • Triage Stage changed from Unreviewed to Design 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 7 years ago by jacob

  • milestone set to 1.1
  • Triage Stage changed from Design decision needed to Accepted

comment:4 Changed 7 years ago by bthomas

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

comment:5 Changed 7 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 6 years ago by mcroydon

  • Needs documentation set

BadHeaderError should be documented.

Changed 6 years ago by bthomas

Patch with doc and tests

comment:7 Changed 6 years ago by bthomas

  • 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 6 years ago by jacob

  • Triage Stage changed from Accepted to Ready for checkin

comment:9 Changed 6 years ago by jacob

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

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

comment:10 Changed 6 years ago by jacob

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

comment:11 Changed 6 years ago by anonymous

  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to reopened

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 6 years ago by Alex

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

Fixed in r10712.

comment:13 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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