Code

Opened 3 years ago

Last modified 3 years ago

#14808 new Bug

i18n is not safe.

Reported by: steveire Owned by: nobody
Component: Template system Version: 1.2
Severity: Normal Keywords:
Cc: claudep, 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)

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.

Attachments (0)

Change History (9)

comment:1 Changed 3 years ago by Alex

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

Added formatting.

comment:2 Changed 3 years ago by claudep

  • Cc claudep 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 3 years ago by ramiro

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

comment:4 Changed 3 years ago by russellm

  • milestone 1.3 deleted
  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 3 years ago by steveire

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.

comment:6 Changed 3 years ago by anonymous

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 3 years ago by d1b

  • Cc d1b added

comment:8 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:9 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.