Security policy shows insecure example as "correct approach"

[Mike Edmunds]

The User input must be sanitized section of ​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.

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"],
            ["admin@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?

[Natalia Bidart]

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:

    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:

    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)

[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?

[Vishy]

That's totally fine Sahil, please go ahead.

[Sahil Advani]

Thanks, Vishy. I’ll take this up.

[Sahil Advani]

Submitted the proposed documentation changes in GitHub PR:

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

Please review the PR when convenient.