[Django] #34581: Filters should not implicitly mark unsafe strings as safe without escaping
Django
------------------------------------------------+------------------------
Reporter: Shai Berger | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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 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.
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34581>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Shai Berger | Owner: Sakshum
Type: | Gadyal
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sakshum Gadyal):
* owner: nobody => Sakshum Gadyal
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34581#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: Sakshum
Type: | Gadyal
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Accepting following conversation in the security mailing list.
--
Ticket URL: <https://code.djangoproject.com/ticket/34581#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Manzar
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Sakshum Gadyal => Faishal Manzar
Comment:
Hey i would like to work o it
--
Ticket URL: <https://code.djangoproject.com/ticket/34581#comment:3>
Django
Reporter: Shai Berger | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Faishal Manzar):
* owner: Faishal Manzar => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/34581#comment:4>
Django
--------------------------------------+------------------------------------
Reporter: Shai Berger | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by 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,
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34581#comment:5>
Django
Reporter: Shai Berger | Owner:
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by omerimzali):
* owner: (none) => omerimzali
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34581#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner:
Type: | omerimzali
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> 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 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.
>
> 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.
New description:
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.
--
Comment (by Shai Berger):
Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34581#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner:
Type: | omerimzali
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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 [comment:7 Shai Berger]:
--
Ticket URL: <https://code.djangoproject.com/ticket/34581#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner:
Type: | omerimzali
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Shai Berger):
Replying to [comment:8 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}}}
[https://docs.djangoproject.com/en/4.2/howto/custom-template-tags
/#filters-and-auto-escaping 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34581#comment:9>