Template code injection -- remote vulnerability

[mpasternak]

• Package version: 2.0
• Django version: doesn't matter, 3.2
• Python version: doesn't matter, 3.10
• Template pack: (Optional)

Title:

django-crispy-forms renders hidden input elements in a way, that allows remote template code injection attack.

Sorry for reporting this publicly, but I see there is no security policy for this repository.

Description:

I am using django-crispy-forms to render a typical login form like:

                {% load crispy_forms_tags %}
                {% crispy form %}

The URL to this view looks like:

url(
            r"^accounts/login/$",
            LoginView.as_view(authentication_form=MyAuthenticationForm),
            name="login_form",
        ),

The form looks nothing special:

class MyAuthenticationForm(AuthenticationForm):
    def __init__(self, request=None, *args, **kw):
        self.helper = FormHelper()
        self.helper.form_class = "custom"
        self.helper.form_action = "."
        self.helper.layout = Layout(
            Fieldset(
                "Zaloguj się!",
                "username",
                "password",
                Hidden(
                    REDIRECT_FIELD_NAME, request.GET.get(REDIRECT_FIELD_NAME, "/bpp/")
                ),
            ),
            ButtonHolder(
                Submit(
                    "submit",
                    "Zaloguj się",
                    css_id="id_submit",
                    css_class="submit button",
                ),
            ),
        )
        AuthenticationForm.__init__(self, request, *args, **kw)

Recently I noticed suspicious activity in the log files. Possibly a malicious user sending requests like:

http://127.0.0.1:8000/accounts/login/?next=%2F%2A1%2A%2F%7B%7B833316405%2B810976201%7D%7D

Which actually resolve to

http://127.0.0.1:8000/accounts/login/?next=/*1*/{{833316405+810976201}}

By accessing this URL, I get an error:

TemplateSyntaxError at /accounts/login/
Could not parse the remainder: '+810976201' from '833316405+810976201'

So obviously the code from the ?next= variable gets executed.

I think the code that is responsible for the bug is this part (sorry for the screenshot but it shows the problem I hope a bit more clearly than the text itself) -- see below.

When executing the code in layout.py line 256 function render, self.value is '/1/{{833316405+810976201}}'. This value, after being converted to a string, is used as the actual template. It can not be parsed, obviously, this is why I get the error. But it looks like there is no problem with passing any other value that would be executed in the template context.

Why the self.value of the hidden input (or any kind of input button, submit, reset buttons) would be converted to a template, then rendered is a mystery, if you ask me. Probably something that django-crispy-forms devs could help explaining.

[mpasternak]

It looks like it was intentional.

https://github.com/django-crispy-forms/django-crispy-forms/blame/472889f08a1172d6bc031eb592cbb3728950814a/crispy_forms/layout.py#L193

d9785ec

@maraujop commited, that BaseInput value can be a variable now.

So I guess it could be used somewhere and it is required for backwards-compatibility, maybe.

But what I also guess is that nobody actually thought, that the variable value could be passed straight from the web.

As it could be context variable, maybe it could be better to just support a simple form of template like {{ [a-Z0-9]* }} and use context.get(...) to get the variable from the context?

Supporting whole full blown template by using Template(str(value)) looks like a remote attack vector to me.

[mpasternak]

Proposed fix for live projects -- just monkey patch it. But beware, you will loose the functionality of the BaseInput value being a context variable.

from crispy_forms import layout
from crispy_forms.utils import TEMPLATE_PACK
from django.template.loader import render_to_string


def fix_crispy_forms_BaseInput_render(
    self, form, context, template_pack=TEMPLATE_PACK, **kwargs
):
    """
    Patch for remote exploit,
    https://github.com/django-crispy-forms/django-crispy-forms/issues/1359
    """
    template = self.get_template_name(template_pack)
    context.update({"input": self})

    return render_to_string(template, context.flatten())

layout.BaseInput.render = monkey_patches.fix_crispy_forms_BaseInput_render

[mpasternak]

Proof of concept:

http://127.0.0.1:8000/accounts/login/?next=%2F*1*%2F{{DEFAULT_MESSAGE_LEVELS}}

This will lead to rendering of the value in the HTML:

<input type="hidden" name="next" value="/*1*/{&#x27;DEBUG&#x27;: 10, &#x27;INFO&#x27;: 20, &#x27;SUCCESS&#x27;: 25, &#x27;WARNING&#x27;: 30, &#x27;ERROR&#x27;: 40}" />

[ckrybus]

Hi @mpasternak,

I'm not one of the decision makers here, but I've been using django-crispy-forms for a while now and maybe I can help. Here are my 2 cents:

1. I don't think it is a security vulnerability per se, but it is definitely a footgun and maybe it should be better documented.

2. There are multiple ways to solve this. I suspect that you don't want to extend the AuthenticationView and you also want to render the form using {% crispy form %} without manually putting <input type="hidden" name="next" value="{{ next }}"> in the login template. Taking this into account the simplest safe solution in my opinion is this:

Fieldset(
    "Zaloguj się!",
    "username",
    "password",
    Hidden("next", "{{next}}"),
),

[mpasternak]

I am not sure if you understand the impact.

Every input field, hidden or visible, in django-crispy-forms renders its value as a template, because of the code in BaseInput class.

Input fields are usually filled with, well, user input.

The rule is that you don't want to allow user input to run server-side. This is why things like https://github.com/ivelum/djangoql exist, you don't want to allow people to run SQL queries or Python code on the server, that's why you try to limit it.

The approach of rendering user input as a template in my opinion is a gaping security hole, waiting to be exploited, or exploited already. Why would somebody try to login to my site with a prepared "?next=/1/{{123+123}}" query? I hope you did not miss that part of my post.

For me, this was probably a scanner.

I am not sure if it was looking for Django sites actually, but as the original contributor of the code found this approach of rendering value as a template useful, then probably others also did.

As all the input fields use this code, this is not a case of fixing a single login form. Sorry.

[ckrybus]

The first time I saw this ticket I got concerned because I use django-crispy-forms in all my projects. I also maintain https://github.com/ckrybus/crispy-bulma and my project would be affected too. So I spend over 1 hour investigating it and I came to the conclusion that the way Hidden is being used in your code fragment leads to a vulnerability, but it is not a "real" vulnerability. That said, since it is so easy to use Hidden this way I'm for better docs or some asserts or something else.

@mpasternak did you try my solution? The error does not occur anymore, right? The reason it works is because it is being read from the context which is being properly escaped. If you pass it directly to Hidden then it becomes part of the template. All classes inheriting from BaseInput (Submit, Button, Hidden, Reset) and HTML accept raw HTML, but this should never come from user input and if it does then it needs to be sanitized first or being passed via context.

In my opinion putting a request.GET["next"] value directly into a Hidden field is the same class of error as this:

return HttpResponse(request.GET["next"])  --> unsafe, XSS risk, it bypasses the escaping part of django

I might be wrong on this of course ( I'm > 99% sure) that's why I'm also impatiently awaiting a response from django-crispy-forms maintainers on this issue, I'm sure it'll come soon :-)

[mpasternak]

I did not try your solution as I monkey-patched django-crispy-forms code in my site so the problem will not occur.

I get the idea of having the value of the field being parsed as a template and I think it could be useful for some corner-cases, but this is a gaping security hole.

As per the code using it, this is standard Django contrib.auth LoginView code. Nothing special about it. And what's funny, the URL passed from the user is verified by url_has_allowed_host_and_scheme of RedirectURLMixin https://github.com/django/django/blob/7f2bc365b3652eaa8a8c19e58ddd35f062c68dbc/django/contrib/auth/views.py#L43 . As the URL containing string "?next=/*1*/{{value+value}}" has allowed host and scheme probably, it gets passed through that function.

Also, disabling django-crispy-forms on that particular login form with hidden field makes the error go away.

So this is not just passing unvalidated data to the form.

This is passing a user-provided value to the code that tries to render it as a Django template, making it possible to get every single value from the context and probably to load custom tag libraries and then to call those.

Please contact me on my e-mail if you have any live sites and you still doubt this is a security hole.

[ckrybus]

And what's funny, the URL passed from the user is verified by url_has_allowed_host_and_scheme of RedirectURLMixin https://github.com/django/django/blob/7f2bc365b3652eaa8a8c19e58ddd35f062c68dbc/django/contrib/auth/views.py#L43 . As the URL containing string "?next=/1/{{value+value}}" has allowed host and scheme probably, it gets passed through that function.

yes, I've also tested that. But this code gets executed when you submit the form. You can try to edit the next field in the DOM directly and put "/1/{{value+value}}" as value there, there will be no error, it will be interpreted as a string value. The "vulnerability" you are writing about is in the initial form rendering phase.

DM sent :)

[mpasternak]

The class name is BaseInput but it is a base class not for input controls but for buttons. So actually the security breach limits itself to any cases where the user value is passed to the hidden input field or to a button value.

[carltongibson]

OK, I think there's some confusion here… — there are two things going on:

1. Crispy forms allows templating the initial value of the next field when rendering the form. This is not under user control, and all the usual caveats for writing templates apply (about e.g. escaping any user submitted data, and so on.) If you can show an injection scenario, leading to an exploit, that would be interesting to see.
2. Django's auth views making use of the next parameter from the request when processing the login (in this case) That next parameter is entirely under user control — it can literally be set to any value by an attacker — and because of that it is treated as untrusted by Django. As well at the url_has_allowed_host_and_scheme check in the get_redirect_url method linked to in the Django source above, the resulting URL is used with HttpRedirectResponse which applies additional sanitation before the using the provided value. This is battle tested code. Any vulnerability here should be reported in private to the Django Security Team.

The take home message is that any redirect value from the request should always use Django's tools for handling redirects, rather than trying to do that raw oneself.

I don't (thus far) see an issue in 1. 2 is Django's responsibility, and not an issue in Crispy Forms.

I hope that makes sense?

I'll leave this open for an extra pair of eyes for the moment. Thanks.

[carltongibson]

OK, I'm going to close given no follow up.