Opened 13 years ago

Closed 5 years ago

Last modified 5 years ago

#14808 closed Bug (fixed)

i18n is not safe.

Reported by: Stephen Kelly Owned by: Tobias Kunze
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: Claude Paroz, d1b Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Alex Gaynor)

This was reported to security@, but was considered a regular bug, not something requiring a security release because it is not a practical attack vector.

If an attacker knows that the string "Today" (several others from the
fr-fr catalog) appears in a Template, they can switch to the french
locale to get an unescaped '.

In [1]: from django.template import Template, Context, mark_safe
In [2]: c = Context()
In [3]: t = Template('{% load i18n %}{% trans "Today" %}')
In [4]: t.render(c)
Out[4]: u"Aujourd'hui"

Note: the ' should be escaped to '

In [5]:
In [6]: c = Context({"today_var": mark_safe("Today")})
In [7]: t = Template('{% load i18n %}{% trans today_var %}')
In [8]: t.render(c)
Out[8]: u"Aujourd'hui"

I recommend this patch:

Index: django/utils/translation/trans_real.py
===================================================================
--- django/utils/translation/trans_real.py      (revision 14499)
+++ django/utils/translation/trans_real.py      (working copy)
@@ -9,7 +9,6 @@
 from cStringIO import StringIO

 from django.utils.importlib import import_module
-from django.utils.safestring import mark_safe, SafeData
 from django.utils.thread_support import currentThread

 # Translations are cached in a dictionary for every language+app tuple.
@@ -272,8 +271,7 @@
            from django.conf import settings
            _default = translation(settings.LANGUAGE_CODE)
        result = getattr(_default, translation_function)(eol_message)
-    if isinstance(message, SafeData):
-        return mark_safe(result)
+    # The result of a i18n call is not safe.
    return result

Result:

In [1]: from django.template import Template, Context, mark_safe
In [2]: c = Context()
In [3]: t = Template('{% load i18n %}{% trans "Today" %}')
In [4]: t.render(c)
Out[4]: u'Aujourd'hui'
In [5]:
In [6]: c = Context({"today_var": mark_safe("Today")})
In [7]: t = Template('{% load i18n %}{% trans today_var %}')
In [8]: t.render(c)
Out[8]: u'Aujourd'hui'

Additionally, the documentation should be updated to say that strings
marked for translation in templates should not be marked as safe:

{% trans "Today & Tomorrow"|safe|upper %} // Looks safe, but gets
translated to French with a '.

This is only actually safe if i18n is not used, which is why
trans_null still has the ifinstance(SafeData) stuff.

With the patch above and the template {% trans today_and_tomo_var %}

If USE_I18N is off:

context.insert("today_and_tomo_var", mark_safe("Today & Tomorrow"))

// Ok renders "Today & Tomorrow"

If USE_I18N is on:

context.insert("today_and_tomo_var", mark_safe("Today & Tomorrow"))

// Not Ok. Renders "Today & Tomorrow" because the safe-ness is removed.

To solve that django users would need to be advised *not* to
pre-mark_safe translatable strings before putting them in the Context
*if* USE_I18N is true.

If USE_I18N is on:

context.insert("today_and_tomo_var", "Today & Tomorrow")
// Ok. Renders "Today & Tomorrow" because
_render_value_in_context escapes the non-safe data.

The many ways strings could be dealt with make this a tricky issue to solve.

Change History (14)

comment:1 by Alex Gaynor, 13 years ago

Description: modified (diff)

Added formatting.

comment:2 by Claude Paroz, 13 years ago

Cc: Claude Paroz added

I'm not sure this can be fixed at all. What about a translatable string which contains markup? Example: "<b>Today</b>", translated in the French po file by "<b>Aujourd'hui</b>".

If there is anything that can cause security problems in the translated strings, shouldn't they be fixed in the po file in the first place?

comment:3 by Ramiro Morales, 13 years ago

See the discussion in #10449, I think it is closely related to this ticket.

comment:4 by Russell Keith-Magee, 13 years ago

milestone: 1.3
Triage Stage: UnreviewedAccepted

comment:5 by Stephen Kelly, 13 years ago

claudep:

I'm not sure this can be fixed at all. What about a translatable string which contains markup? Example: "<b>Today</b>", translated in the French po file by "<b>Aujourd'hui</b>".

That should be translated as "<b>Aujourd&#39;hui</b>" because it it should be safe html output. Safe html in, safe html out. If the input string contains html, the translated output should be safe. That requires translators to know about such things of course though, or for template authors to use context telling the translators that the output should be safe html.

I don't think this issue can be solved within fixes to .po files though.

Version 0, edited 13 years ago by Stephen Kelly (next)

comment:6 by anonymous, 13 years ago

Severity: Normal
Type: Bug

comment:7 by d1b, 13 years ago

Cc: d1b added

comment:8 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:10 by Aymeric Augustin, 10 years ago

Component: Template systemDocumentation

I believe the only way forwards is to explain the current behavior and add warnings in the documentation.

comment:11 by Tobias Kunze, 5 years ago

Has patch: set
Owner: changed from nobody to Tobias Kunze
Status: newassigned

comment:12 by Mariusz Felisiak, 5 years ago

Version: 1.2master

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 7d49ad7:

Fixed #14808 -- Doc'd that trans and blocktrans tags don't escape translations.

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In b51842a:

[2.2.x] Fixed #14808 -- Doc'd that trans and blocktrans tags don't escape translations.

Backport of 7d49ad76562e8c0597a0eb66046ab423b12888d8 from master

Note: See TracTickets for help on using tickets.
Back to Top