Opened 17 years ago
Closed 17 years ago
#4131 closed (fixed)
addslashes isn't sufficient to protect literal strings in embedded JavaScript code
Reported by: | Owned by: | Jacob | |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Keywords: | ||
Cc: | metajack@…, me@…, sam@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
When creating literal strings in embedded JavaScript code, the addslashes filter is used to escape characters significant to JavaScript:
<script>
var x = "{{x|addslashes}}";
blah(x);
</script>
But if the variable x includes the string "</script>", then this script block is ended too early, and the page is broken.
Attached is a patch that also escapes the </ sequence to ensure that this can't happen.
Attachments (3)
Change History (25)
by , 17 years ago
Attachment: | patch_addslashes.txt added |
---|
comment:1 by , 17 years ago
But should this really be in addslashes? It's a pretty specific edge-case, and may not be desirable anywhere else - e.g. non-Javascript strings.
Seems like a custom filter made to escape </script> would be better, IMO :)
comment:2 by , 17 years ago
Has patch: | set |
---|---|
Summary: | [patch] addslashes isn't sufficient to protect literal strings in embedded JavaScript code → addslashes isn't sufficient to protect literal strings in embedded JavaScript code |
comment:3 by , 17 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Design decision needed |
It does sound like a corner case, but isn't escaping things for javascript the primary usage for addslashes?
comment:4 by , 17 years ago
It is a corner case, but there's no harm in escaping </ even in non-javascript settings. Given the current difficulty of making Django apps XSS-safe, I'd think we'd want to take an advantage we could.
The ticket has been marked needs_better_patch. Is there some action I should be taking?
comment:5 by , 17 years ago
(edited, with attribution) It is a corner case, but there's no harm in escaping </ even in non-javascript settings. Given the current difficulty of making Django apps XSS-safe, I'd think we'd want to take any advantage we could.
The ticket has been marked needs_better_patch. Is there some action I should be taking?
comment:6 by , 17 years ago
There potentially could be harm in escaping </
in a non-javascript setting where your escaped \/
doesn't resolve to /
.
However, I agree that XSS-safety should be a priority. Perhaps a specific filter for javascript "escaping"?
comment:7 by , 17 years ago
I'm attaching another patch which escapes all things not allowed in a JS string literal per ECMA 262 7.8.4 and also incorporates Ned's change for </script>.
Please do consider changing addslashes to include this or consider adding another filter to handle all embedded strings in JS.
by , 17 years ago
Attachment: | addslash_escape.diff added |
---|
comment:8 by , 17 years ago
If this change is not made to addslashes, a warning should be added to its documentation to the effect that it is insufficient to protect strings in javascript.
comment:9 by , 17 years ago
Cc: | added |
---|
I just ran into this bug today (specifically not escaping \n) and this patch works great. Would love to see this in the main tree.
comment:10 by , 17 years ago
Patch needs improvement: | unset |
---|
Unflagging "Patch needs improvement" since I attached a new patch quite a while ago, and the ticket has had no action on it since then.
comment:11 by , 17 years ago
Needs tests: | set |
---|
comment:12 by , 17 years ago
Needs tests: | unset |
---|
addslashes
as it stands is useful for things other than javascript, which may not accept \/
, \n
; csv for example (see http://www.djangoproject.com/documentation/outputting_csv/ for a prominent example).
I'm attaching a patch which adds this code as a new filter "escapejs", along with docs and tests.
Also, I question the removal in Jeremy's patch of carriage returns, so they're being converted to \r
.
comment:13 by , 17 years ago
Cc: | added |
---|
comment:14 by , 17 years ago
Looks good, Andy.
Small change required to documentation: *form feed* ``\f`` to ``\f``
should read *form feed* to ``\f``
Perhaps you could bring this up in django-dev to get a design decision.
comment:15 by , 17 years ago
Sweet, Andy.
FWIW, ECMA 262 7.8.4, to paraphrase, says you can't have a LineTerminator inside a string:
SourceCharacter but not double-quote " or backslash \ or LineTerminator \ EscapeSequence
I'm not sure what to make of that last "\ EscapeSequence".
LineTerminator is defined in 7.3 as LF, CR, LS, PS:
\u000A Line Feed <LF> \u000D Carriage Return <CR> \u2028 Line separator <LS> \u2029 Paragraph separator <PS>
In any case, I agree it's a bit ambiguous, and probably isn't a common issue. I'd much rather see escapejs go in as-is than debate whether it's completely correct according to a mis-worded spec. At the end of the day, this is about not breaking web pages and avoiding security problems, which I'm sure the patch does better than current trunk. :)
Will you bring it up on django-dev, or should I?
comment:16 by , 17 years ago
Isn't that talking about the literal characters though, not the escapes?
So the literal character represented by \u000D
or \n
wouldn't be allowed, yet '\n'
is?
comment:17 by , 17 years ago
Oh. Hah. Yeah, Andy's right, that's just a bug in my patch. Andy's is correct. I just read his comment and mistook his meaning.
comment:18 by , 17 years ago
@SmileyChris: Patch updated, docs fixed.
@jdunck: Can you bring it up on dj-d? I was just trying my hand at a bit of triaging on a handful of tickets, and saw that this one could do with a quick (and easy) tidy.
comment:19 by , 17 years ago
comment:20 by , 17 years ago
Cc: | added |
---|
comment:21 by , 17 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Triage Stage: | Design decision needed → Ready for checkin |
comment:22 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
The patch!