[Django] #29837: Django refuses to send an email whose subject contains newlines
Django
-----------------------------------------+------------------------
Reporter: Álex Córcoles | Owner: nobody
Type: New feature | Status: new
Component: Core (Mail) | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Hi,
django.core.mail.send_mail raises an exception if the subject contains a
newline.
We have hit this problem because we have a piece of code that parses an
email using Python's libraries, extracts the subject and then re-sends an
email with the same subject.
If this email has a multi-line subject (this seems to be common, I believe
some bit in mail delivery code wordwraps headers), this code then fails
unexpectedly.
This is extremely hard to detect before it happens, so we would humbly
suggest that send_mail don't raise an error if passed a subject containing
a newline, but rather sends the email doing the necessary security
handling.
Kind regards,
Álex
--
Ticket URL: <https://code.djangoproject.com/ticket/29837>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+--------------------------------------
Reporter: Álex Córcoles | Owner: nobody
Type: New feature | Status: new
Component: Core (Mail) | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
What does "the necessary security hardening" mean?
--
Ticket URL: <https://code.djangoproject.com/ticket/29837#comment:1>
Django
-------------------------------+--------------------------------------
Component: Core (Mail) | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => invalid
Comment:
Hmmm. I seem to recall that allowing newlines leads to dangers of email
injection attacks… (I’d need to look that up).
[https://tools.ietf.org/html/rfc2822.html#section-2.2.3 RFC 2822 section
2.2.3] says:
>The process of moving from this folded multiple-line representation
>of a header field to its single line representation is called
>"unfolding". Unfolding is accomplished by simply removing any CRLF
>that is immediately followed by WSP. Each header field should be
>treated in its unfolded form for further syntactic and semantic
>evaluation.
i.e. I think you’re meant to remove the newlines before resending the
email.
I’m going to close on that basis.
--
Ticket URL: <https://code.djangoproject.com/ticket/29837#comment:2>
Django
-------------------------------+--------------------------------------
Type: New feature | Status: new
Component: Core (Mail) | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Álex Córcoles):
* status: closed => new
* resolution: invalid =>
Comment:
Yeah, I traced the code and I was that avoiding sending an email with
newlines had to do with security.
However, I think that raising an undocumented exception is not the right
answer.
One option would be to document this problem clearly in send_mail's docs.
I don't think that's going to save many people from possible runtime
problems, but at least the behavior is documented.
I would propose that send_mail removes itself the newlines instead of
pushing that responsibility to each of its callers. I think it's
consistent with all the ORM methods being able to avoid SQL injection
instead of raising an undocumented exception if they receive non-escaped
values.
--
Ticket URL: <https://code.djangoproject.com/ticket/29837#comment:3>
Django
-------------------------------+--------------------------------------
Type: New feature | Status: new
Component: Core (Mail) | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Putting newlines in email headers is known No-no, or it should be. (That
the transport layer might wrap headers as per the spec is something else
entirely.)
Adding behaviour to handle bad input isn't something I can see as
sensible.
What's the actual error you get? Yes, a PR with a better error message
would always be worth a look. (But pending such a PR this isn't an active
issue.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29837#comment:4>
Django
-------------------------------+--------------------------------------
Type: New feature | Status: new
Component: Core (Mail) | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Álex Córcoles):
Oh, the exception itself is quite clear:
https://github.com/django/django/blob/master/django/core/mail/message.py#L60
; it doesn't provide a rationale, but I don't think an exception should do
that- so I wouldn't really touch that, it's perfect as it stands.
I'd suggest documenting this behavior clearly in the docs (the docs hint
that it can throw SMTPExceptions, but this is a ValueError). I'd rather
join newlines, but if you guys decide this is good as it stands, I have
nothing more to say.
Thanks for your time,
Álex
--
Ticket URL: <https://code.djangoproject.com/ticket/29837#comment:5>
Django
-------------------------------+--------------------------------------
Component: Core (Mail) | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => wontfix
Comment:
I guess you didn't see the
[https://docs.djangoproject.com/en/dev/topics/email/#preventing-header-
injection Preventing header injection] section.
If you want to make a proposal about changing the behavior, the
DevelopersMailingList would be the place to do it. A more concrete
proposal than "the necessary security hardening" is needed. If there's a
consensus, then we'd reopen this ticket.
I didn't see anything in the original ticket (#1135) about the
implementation (vs. trying to do some sanitization). If other frameworks
have a different behavior, it may make sense to follow their pattern. I
agree it can be a bit cumbersome to catch `BadHeaderError` or write your
own sanitization.
--
Ticket URL: <https://code.djangoproject.com/ticket/29837#comment:6>
Django
-------------------------------+--------------------------------------
Component: Core (Mail) | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Álex Córcoles):
I stand corrected, indeed I had not seen that section which addresses my
concerns for documentation correctly.
As you say:
> I agree it can be a bit cumbersome to catch BadHeaderError or write your
own sanitization.
, I see you understand correctly the issue I'm having , so while I
disagree, I think pursuing this further is not using anyone's time
effectively.
Thanks for your time,
Álex
--
Ticket URL: <https://code.djangoproject.com/ticket/29837#comment:7>