Python: Promote Template Injection query from experimental

[joefarebrother]

Promotes the Template Injection query, originally submitted to the experimental query pack here.

Fixes a modeled Jinja2 sink to properly refer to Environment().from_string(); and promotes sinks from the other template frameworks included in the experimental version.
Excludes the Cheetah package as its documentation links are dead and its last update was 15 years ago.

[github-actions]

QHelp previews:

python/ql/src/Security/CWE-074/TemplateInjection.qhelp

Server Side Template Injection

A template from a server templating engine such as Jinja constructed from user input can allow the user to execute arbitrary code using certain template features. It can also allow for cross-site scripting.

Recommendation

Ensure that an untrusted value is not used to directly construct a template. Jinja also provides SandboxedEnvironment that prohibits access to unsafe methods and attributes. This can be used if constructing a template from user input is absolutely necessary.

Example

In the following case, template is used to generate a Jinja2 template string. This can lead to remote code execution.

from django.urls import path
from django.http import HttpResponse
from jinja2 import Template, escape


def a(request):
    template = request.GET['template']

    # BAD: Template is constructed from user input. 
    t = Template(template)

    name = request.GET['name']
    html = t.render(name=escape(name))
    return HttpResponse(html)


urlpatterns = [
    path('a', a),
]

The following is an example of a string that could be used to cause remote code execution when interpreted as a template:

{% for s in ().__class__.__base__.__subclasses__() %}{% if "warning" in s.__name__ %}{{s()._module.__builtins__['__import__']('os').system('cat /etc/passwd') }}{% endif %}{% endfor %}

In the following case, user input is not used to construct the template. Instead, it is only used as the parameters to render the template, which is safe.

from django.urls import path
from django.http import HttpResponse
from jinja2 import Template, escape


def a(request):
    # GOOD: Template is a constant, not constructed from user input
    t = Template("Hello, {{name}}!")

    name = request.GET['name']
    html = t.render(name=escape(name))
    return HttpResponse(html)


urlpatterns = [
    path('a', a),
]

In the following case, a SandboxedEnvironment is used, preventing remote code execution.

from django.urls import path
from django.http import HttpResponse
from jinja2 import escape
from jinja2.sandbox import SandboxedEnvironment


def a(request):
    env = SandboxedEnvironment()
    template = request.GET['template']

    # GOOD: A sandboxed environment is used to construct the template. 
    t = env.from_string(template)

    name = request.GET['name']
    html = t.render(name=escape(name))
    return HttpResponse(html)


urlpatterns = [
    path('a', a),
]

References

• Portswigger: Server-Side Template Injection.
• Common Weakness Enumeration: CWE-74.

[tausbn]

A few comments and suggestions here and there, but otherwise this looks good to me! 👍

[tausbn]

I don't see why we need to use type tracking here. It looks to me like classRef().getAnInstance() would do the same thing.

[joefarebrother]

Changed.
In what sort of situations do we want to use type tracking as opposed to API graphs?

[tausbn]

In general, I would suggest using API graphs wherever possible, only using type tracking when API graphs are not sufficient. One example of where we do this is in the modelling of pathlib, where we want to observe that the / operator returns a Path object. Since the API graph (at present) doesn't have any way to reference the step from a to a / b (say) because the latter doesn't have a node in the API graph, we have to use type tracking instead.

In the glorious future when we get API graphs à la Ruby, we should be able to do this without using type trackers. (Well, apart from the two very general type(back)trackers that the API graph calculation is built upon.)

[tausbn]

This also seems like something we could push into the API graph layer, to make it slightly more general.

[tausbn]

Thanks for the changes. This looks good to me! 👍

[joefarebrother]

Do we need a separate docs review from the docs team, or ok to merge as is?

[myarb]

👋🏼 @joefarebrother I'm not entirely sure if this requires a separate Docs review, so I've asked in #code-security-docs in Slack. cc @felicitymay or @saritai

[myarb]

👋🏼 @joefarebrother I've confirmed with Felicity that this needs a docs review. For the reasoning, I'll share her Slack message:

The user-facing content in an experimental query is not reviewed by the docs team before it's merged, so when an experimental query is promoted into the standard suite it always needs a docs review.

I've added this to the Docs Content review board for a writer to review.

[subatoi]

👍 Just some very minor suggestions—thank you

[subatoi]

Just reapproving in case it saves you time—thank you!

[joefarebrother]

Updated test outputs - just needs re-approval if tests now pass