[Django] #37388: Security policy shows insecure example as "correct approach"

[Django]

#37388: Security policy shows insecure example as "correct approach"
------------------------------+-----------------------------------------
Reporter: Mike Edmunds | Type: Uncategorized
Status: new | Component: Documentation
Version: 6.1 | Severity: Normal
Keywords: security | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+-----------------------------------------
The ''User input must be sanitized'' section of
[https://docs.djangoproject.com/en/dev/internals/security/#user-input-
must-be-sanitized Django's security policies] presents a "not considered
valid" vulnerability report using a raw query param as the `from_email` in
a `send_mail()` call. It then shows the code below as the "correct
approach."

But this code still uses user-supplied content as the `from_email`; it
just verifies it looks like an email address first. Sending email ''from''
an arbitrary address is at best a poor practice, and often a real
vulnerability (in the user's code, not Django). See #37162 where we fixed
similar problems in contact form examples throughout the docs.

{{{#!python
from django import forms
from django.core.mail import send_mail
from django.http import JsonResponse


class EmailForm(forms.Form):
email = forms.EmailField()


def my_proof_of_concept(request):
form = EmailForm(request.GET)
if form.is_valid():
send_mail(
"Email subject",
"Email body",
form.cleaned_data["email"],
["ad...@example.com"],
)
return JsonResponse(status=200)
return JsonResponse(form.errors, status=400)
}}}

We could try to rework this code as in the earlier ticket, but I'm not
sure sending email is an effective example for this particular security
reporting topic. (Django has long prevented email header injection, so the
original "not considered valid" code isn't really any more ''insecure''
than the "correct approach." It's just more likely to generate an uncaught
error on bad input.)

I'm not really sure about a replacement. Maybe something where raw user
input is interpolated directly into an html response, rather than being
escaped or rendered through a template?
--
Ticket URL: <https://code.djangoproject.com/ticket/37388>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37388: Security policy shows insecure example as "correct approach"

-------------------------------+------------------------------------
Reporter: Mike Edmunds | Owner: (none)
Type: Bug | Status: new
Component: Documentation | Version: 6.1
Severity: Normal | Resolution:
Keywords: security | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------+------------------------------------
Changes (by Natalia Bidart):

* stage: Unreviewed => Accepted
* type: Uncategorized => Bug

Comment:

Thank you Mike, I agree with your analysis. Note that both examples call
`JsonResponse(status=200)`, which raises `TypeError` since `data` is
required so this is also something to fix. Then, rather than an XSS, I'd
suggest an ORM example, since unsanitized user input in ORM queries is
what we get reported most often (by far!). Based on the reports we
actually get, I'd suggest replacing the email example with one using user
input as a storage path:

{{{#!python
def my_proof_of_concept(request):
file_name = request.GET.get("filename")
return HttpResponse(default_storage.open(f"user/{file_name}"))
}}}

and the `extra()` one with user input as a database function name:

{{{#!python
def my_proof_of_concept(request):
function = request.GET.get("function", "UPPER")
books = Book.objects.annotate(title_transformed=Func("title",
function=function))
return JsonResponse(list(books.values("title_transformed")),
safe=False)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37388#comment:1>

[Django]

#37388: Security policy shows insecure example as "correct approach"
-------------------------------+------------------------------------

Reporter: Mike Edmunds | Owner: Vishy
Type: Bug | Status: assigned

Component: Documentation | Version: 6.1
Severity: Normal | Resolution:
Keywords: security | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Changes (by Vishy):

* owner: (none) => Vishy
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/37388#comment:2>

[Django]

#37388: Security policy shows insecure example as "correct approach"
-------------------------------+------------------------------------
Reporter: Mike Edmunds | Owner: Vishy
Type: Bug | Status: assigned
Component: Documentation | Version: 6.1
Severity: Normal | Resolution:
Keywords: security | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Comment (by Sahil Advani):

Hi Vishy, I was investigating #37388 as a potential first contribution
before it was assigned. I’ve now gone through the discussion and Natalia’s
proposed changes. I’d be interested in implementing the documentation fix
if you haven’t started on it yet. Would you be okay with me taking the
patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/37388#comment:3>

[Django]

#37388: Security policy shows insecure example as "correct approach"
-------------------------------+------------------------------------

Reporter: Mike Edmunds | Owner: (none)
Type: Bug | Status: new

Component: Documentation | Version: 6.1
Severity: Normal | Resolution:
Keywords: security | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Changes (by Vishy):

* owner: Vishy => (none)
* status: assigned => new

Comment:

That's totally fine Sahil, please go ahead.
--
Ticket URL: <https://code.djangoproject.com/ticket/37388#comment:4>

[Django]

#37388: Security policy shows insecure example as "correct approach"

-------------------------------+----------------------------------------
Reporter: Mike Edmunds | Owner: Sahil Advani
Type: Bug | Status: assigned

Component: Documentation | Version: 6.1
Severity: Normal | Resolution:
Keywords: security | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------+----------------------------------------
Changes (by Sahil Advani):

* owner: (none) => Sahil Advani

* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/37388#comment:5>

[Django]

#37388: Security policy shows insecure example as "correct approach"
-------------------------------+----------------------------------------
Reporter: Mike Edmunds | Owner: Sahil Advani
Type: Bug | Status: assigned
Component: Documentation | Version: 6.1
Severity: Normal | Resolution:
Keywords: security | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------
Changes (by Sahil Advani):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/37388#comment:6>

[Django]

#37388: Security policy shows insecure example as "correct approach"
-------------------------------+----------------------------------------
Reporter: Mike Edmunds | Owner: Sahil Advani
Type: Bug | Status: assigned
Component: Documentation | Version: 6.1
Severity: Normal | Resolution:
Keywords: security | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------

Comment (by Sahil Advani):

Submitted the proposed documentation changes in GitHub PR:

https://github.com/django/django/pull/22072

Please review the PR when convenient.
--
Ticket URL: <https://code.djangoproject.com/ticket/37388#comment:7>

[Django]

#37388: Security policy shows insecure example as "correct approach"
-------------------------------+----------------------------------------
Reporter: Mike Edmunds | Owner: Sahil Advani
Type: Bug | Status: assigned
Component: Documentation | Version: 6.1
Severity: Normal | Resolution:
Keywords: security | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------

Changes (by Mike Edmunds):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/37388#comment:8>

[Django]

#37388: Security policy shows insecure example as "correct approach"
-------------------------------+----------------------------------------
Reporter: Mike Edmunds | Owner: Sahil Advani
Type: Bug | Status: assigned
Component: Documentation | Version: 6.1
Severity: Normal | Resolution:
Keywords: security | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------

Changes (by Sahil Advani):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/37388#comment:9>

[Django]

#37388: Security policy shows insecure example as "correct approach"

-------------------------------------+-------------------------------------

Reporter: Mike Edmunds | Owner: Sahil
| Advani
Type: Bug | Status: assigned
Component: Documentation | Version: 6.1
Severity: Normal | Resolution:

Keywords: security | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Mike Edmunds):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/37388#comment:10>