Opened 6 years ago

Last modified 14 months ago

#14808 new Bug

i18n is not safe.

Reported by: Stephen Kelly Owned by: nobody
Component: Documentation Version: 1.2
Severity: Normal Keywords:
Cc: Claude Paroz, d1b Triage Stage: Accepted
Has patch: no 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 (10)

comment:1 Changed 6 years ago by Alex Gaynor

Description: modified (diff)
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Added formatting.

comment:2 Changed 6 years ago by Claude Paroz

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 Changed 6 years ago by Ramiro Morales

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

comment:4 Changed 6 years ago by Russell Keith-Magee

milestone: 1.3
Triage Stage: UnreviewedAccepted

comment:5 Changed 6 years ago by Stephen Kelly

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.

Last edited 14 months ago by Tim Graham (previous) (diff)

comment:6 Changed 5 years ago by anonymous

Severity: Normal
Type: Bug

comment:7 Changed 5 years ago by d1b

Cc: d1b added

comment:8 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:10 Changed 2 years ago by Aymeric Augustin

Component: Template systemDocumentation

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

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