Filters should not implicitly mark unsafe strings as safe without escaping

[Shai Berger]

Consider the following template code snippet:

{% autoescape off %}
{{ some_list|join:some_var }}
{% endautoescape %}

As noted in #34574, the current implementation joins the members of some_list without escaping them, but marks the end result as safe (since #34578, the joiner some_var is not escaped either -- but in most common use, it is a literal in the template, so it is safe to begin with).

Given that we already have a safeseq filter, and #34577 is introducing escapeseq as well, I think we should change the behavior of join and other related filters (an initial list that was given in the related discussion in the security team includes linenumbers, linebreaks, and linebreaksbr, but there are probably others) so they don't mark unsafe strings as safe without escaping them.

The details change by the specific filter: The join filter may output an unsafe string when all of its inputs are unsafe. The linebreaks filter cannot -- it introduces HTML tags into its output, so this output must be marked safe. The join filter, too, should preserve safety -- so if some input (in the sequence or the joiner) is safe, again, the output must be safe.

When a filter gets an unsafe input, must produce a safe output, and autoescape is on, there's no real problem -- the input can be escaped. This is current behavior, mostly (join escapes input and marks the output safe, even if it isn't forced to).

But when autoescaping is off, inputs are not escaped -- however, the output, which usually includes their content verbatim, is still marked safe.

I suggest a general rule: A filter which fulfills these three conditions, should error out:

• it needs to produce safe output
• some of its input is unsafe,
• the context is such that it should not escape the input

This is a hardening, not a security fix, so it should follow a normal deprecation cycle: The actual change should only happen after the next LTS release.

This would affect many filters, and is backwards-incompatible, but I think it would make the escaping more consistent and predictable, and it would make Django more safe-by-default for some less-common use-cases.

[Natalia Bidart]

Accepting following conversation in the security mailing list.

[Faishal Manzar]

Hey i would like to work on it

[omerimzali]

I'd like to work on this ticket.

And I have 2 questions.

"I suggest a general rule: A filter which gets unsafe input, should not escape it, and yet needs to produce safe output, should error out. This is a hardening, not a security fix, so it should follow a normal deprecation cycle: The actual change should only happen after the next LTS release."

A filter which gets unsafe input should not escape it. I totally agree but do we mean it only when autoescape off? {% autoescape off %}

How should I check "All filters which can get unsafe input". I know Django at least has 4 of them below. What's the right way to find others? Should this patch contains all of them.

join
linenumbers,
linebreaks,
linebreaksbr,

[Shai Berger]

Replying to omerimzali:

"I suggest a general rule: A filter which gets unsafe input, should not escape it, and yet needs to produce safe output, should error out. This is a hardening, not a security fix, so it should follow a normal deprecation cycle: The actual change should only happen after the next LTS release."

A filter which gets unsafe input should not escape it. I totally agree but do we mean it only when autoescape off? {% autoescape off %}

I guess this phrasing was unclear, and I changed it in hope it is clearer now: The "should not change it" part is not part of "how a filter should generally behave", but a further condition. A filter for which all three conditions are true, should error out. "autoescape off" means that filters, generally, should not escape the input.

How should I check "All filters which can get unsafe input". I know Django at least has 4 of them below. What's the right way to find others? Should this patch contains all of them.

join
linenumbers,
linebreaks,
linebreaksbr,

All filters can get unsafe input. Whether the filter actually got safe or unsafe input is only decided at runtime, and cannot be known in advance. The interesting question is, "does the filter need to produce safe output", and this changes from filter to filter: The last three you listed always need to produce safe output, because they need to include HTML to break lines; but for some filters, like join, this too is only decided at runtime -- join needs to produce safe output if (and only if) any of its inputs (the list members and the joiner) are safe. I can't think of any way to find which filters are relevant (and when), except to go over them one by one.

I hope this clarifies the issue.

[omerimzali]

Thank you for all clarifications.

For Join filter:
First case: Totally safe inputs the list members and the joiner are safe, it will be produce safe output. (There's no unsafe input and there no need to escape in anyway so it doesn't contain any of our conditions)
Second case: When it has any of its inputs (the list members and the joiner) are safe but also some of the inputs are unsafe, it still needs to produce safe output.
Third case: All of the inputs are unsafe, it can produce unsafe output.

For Second case, we have 3 conditions to check:

• it needs to produce safe output: For this case yes it needs to produce safe output because some of the inputs are safe.
• some of its input is unsafe: Yes some of the inputs are not safe,
• the context is such that it should not escape the input: How can we be sure it should not escape the input? is {% autoescape off %} line enough to assume this? Could be any other configurations or common behaviours for some of the filters to not escape the input?

If we check this 3 conditions it should error out.

And for the 3rd case, if all of the inputs are unsafe, it doesn't need to produce a safe ouput so source doesn't need to error out. Is it correct?

Replying to Shai Berger:

Replying to omerimzali:

"I suggest a general rule: A filter which gets unsafe input, should not escape it, and yet needs to produce safe output, should error out. This is a hardening, not a security fix, so it should follow a normal deprecation cycle: The actual change should only happen after the next LTS release."

A filter which gets unsafe input should not escape it. I totally agree but do we mean it only when autoescape off? {% autoescape off %}

I guess this phrasing was unclear, and I changed it in hope it is clearer now: The "should not change it" part is not part of "how a filter should generally behave", but a further condition. A filter for which all three conditions are true, should error out. "autoescape off" means that filters, generally, should not escape the input.

How should I check "All filters which can get unsafe input". I know Django at least has 4 of them below. What's the right way to find others? Should this patch contains all of them.

join
linenumbers,
linebreaks,
linebreaksbr,

All filters can get unsafe input. Whether the filter actually got safe or unsafe input is only decided at runtime, and cannot be known in advance. The interesting question is, "does the filter need to produce safe output", and this changes from filter to filter: The last three you listed always need to produce safe output, because they need to include HTML to break lines; but for some filters, like join, this too is only decided at runtime -- join needs to produce safe output if (and only if) any of its inputs (the list members and the joiner) are safe. I can't think of any way to find which filters are relevant (and when), except to go over them one by one.

I hope this clarifies the issue.

[Shai Berger]

Replying to omerimzali:

For Join filter:
First case: Totally safe inputs the list members and the joiner are safe, it will be produce safe output. (There's no unsafe input and there no need to escape in anyway so it doesn't contain any of our conditions)
Second case: When it has any of its inputs (the list members and the joiner) are safe but also some of the inputs are unsafe, it still needs to produce safe output.
Third case: All of the inputs are unsafe, it can produce unsafe output.

Correct.

For Second case, we have 3 conditions to check:
[...]

• the context is such that it should not escape the input: How can we be sure it should not escape the input? is {% autoescape off %} line enough to assume this? Could be any other configurations or common behaviours for some of the filters to not escape the input?

Please see the details about needs_autoescape ​here.

If we check this 3 conditions it should error out.

And for the 3rd case, if all of the inputs are unsafe, it doesn't need to produce a safe ouput so source doesn't need to error out. Is it correct?

Right. No need to error, and if autoescape is off, there's no need to escape. If autoescape is on, though, I would still escape the output even when all inputs are unsafe -- so that in the majority of cases, there is no change in behavior.