1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
|
From 0787ee55dd0d42dd8bef97d4e7ed7584f44c16e9 Mon Sep 17 00:00:00 2001
From: Roland Shoemaker <roland@golang.org>
Date: Wed, 14 Feb 2024 17:18:36 -0800
Subject: [PATCH 2/4] [release-branch.go1.21] html/template: escape additional
tokens in MarshalJSON errors
Escape "</script" and "<!--" in errors returned from MarshalJSON errors
when attempting to marshal types in script blocks. This prevents any
user controlled content from prematurely terminating the script block.
Updates #65697
Fixes #65968
Change-Id: Icf0e26c54ea7d9c1deed0bff11b6506c99ddef1b
Reviewed-on: https://go-review.googlesource.com/c/go/+/564196
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
(cherry picked from commit ccbc725f2d678255df1bd326fa511a492aa3a0aa)
Reviewed-on: https://go-review.googlesource.com/c/go/+/567515
Reviewed-by: Carlos Amedee <carlos@golang.org>
---
src/html/template/js.go | 22 ++++++++-
src/html/template/js_test.go | 96 ++++++++++++++++++++----------------
2 files changed, 74 insertions(+), 44 deletions(-)
diff --git a/src/html/template/js.go b/src/html/template/js.go
index 4e05c145572..f4d1303bebe 100644
--- a/src/html/template/js.go
+++ b/src/html/template/js.go
@@ -171,13 +171,31 @@ func jsValEscaper(args ...any) string {
// cyclic data. This may be an unacceptable DoS risk.
b, err := json.Marshal(a)
if err != nil {
- // Put a space before comment so that if it is flush against
+ // While the standard JSON marshaller does not include user controlled
+ // information in the error message, if a type has a MarshalJSON method,
+ // the content of the error message is not guaranteed. Since we insert
+ // the error into the template, as part of a comment, we attempt to
+ // prevent the error from either terminating the comment, or the script
+ // block itself.
+ //
+ // In particular we:
+ // * replace "*/" comment end tokens with "* /", which does not
+ // terminate the comment
+ // * replace "</script" with "\x3C/script", and "<!--" with
+ // "\x3C!--", which prevents confusing script block termination
+ // semantics
+ //
+ // We also put a space before the comment so that if it is flush against
// a division operator it is not turned into a line comment:
// x/{{y}}
// turning into
// x//* error marshaling y:
// second line of error message */null
- return fmt.Sprintf(" /* %s */null ", strings.ReplaceAll(err.Error(), "*/", "* /"))
+ errStr := err.Error()
+ errStr = strings.ReplaceAll(errStr, "*/", "* /")
+ errStr = strings.ReplaceAll(errStr, "</script", `\x3C/script`)
+ errStr = strings.ReplaceAll(errStr, "<!--", `\x3C!--`)
+ return fmt.Sprintf(" /* %s */null ", errStr)
}
// TODO: maybe post-process output to prevent it from containing
diff --git a/src/html/template/js_test.go b/src/html/template/js_test.go
index 259dcfbdc5b..17cedcec05f 100644
--- a/src/html/template/js_test.go
+++ b/src/html/template/js_test.go
@@ -5,6 +5,7 @@
package template
import (
+ "errors"
"math"
"strings"
"testing"
@@ -103,61 +104,72 @@ func TestNextJsCtx(t *testing.T) {
}
}
+type jsonErrType struct{}
+
+func (e *jsonErrType) MarshalJSON() ([]byte, error) {
+ return nil, errors.New("beep */ boop </script blip <!--")
+}
+
func TestJSValEscaper(t *testing.T) {
tests := []struct {
- x any
- js string
+ x any
+ js string
+ skipNest bool
}{
- {int(42), " 42 "},
- {uint(42), " 42 "},
- {int16(42), " 42 "},
- {uint16(42), " 42 "},
- {int32(-42), " -42 "},
- {uint32(42), " 42 "},
- {int16(-42), " -42 "},
- {uint16(42), " 42 "},
- {int64(-42), " -42 "},
- {uint64(42), " 42 "},
- {uint64(1) << 53, " 9007199254740992 "},
+ {int(42), " 42 ", false},
+ {uint(42), " 42 ", false},
+ {int16(42), " 42 ", false},
+ {uint16(42), " 42 ", false},
+ {int32(-42), " -42 ", false},
+ {uint32(42), " 42 ", false},
+ {int16(-42), " -42 ", false},
+ {uint16(42), " 42 ", false},
+ {int64(-42), " -42 ", false},
+ {uint64(42), " 42 ", false},
+ {uint64(1) << 53, " 9007199254740992 ", false},
// ulp(1 << 53) > 1 so this loses precision in JS
// but it is still a representable integer literal.
- {uint64(1)<<53 + 1, " 9007199254740993 "},
- {float32(1.0), " 1 "},
- {float32(-1.0), " -1 "},
- {float32(0.5), " 0.5 "},
- {float32(-0.5), " -0.5 "},
- {float32(1.0) / float32(256), " 0.00390625 "},
- {float32(0), " 0 "},
- {math.Copysign(0, -1), " -0 "},
- {float64(1.0), " 1 "},
- {float64(-1.0), " -1 "},
- {float64(0.5), " 0.5 "},
- {float64(-0.5), " -0.5 "},
- {float64(0), " 0 "},
- {math.Copysign(0, -1), " -0 "},
- {"", `""`},
- {"foo", `"foo"`},
+ {uint64(1)<<53 + 1, " 9007199254740993 ", false},
+ {float32(1.0), " 1 ", false},
+ {float32(-1.0), " -1 ", false},
+ {float32(0.5), " 0.5 ", false},
+ {float32(-0.5), " -0.5 ", false},
+ {float32(1.0) / float32(256), " 0.00390625 ", false},
+ {float32(0), " 0 ", false},
+ {math.Copysign(0, -1), " -0 ", false},
+ {float64(1.0), " 1 ", false},
+ {float64(-1.0), " -1 ", false},
+ {float64(0.5), " 0.5 ", false},
+ {float64(-0.5), " -0.5 ", false},
+ {float64(0), " 0 ", false},
+ {math.Copysign(0, -1), " -0 ", false},
+ {"", `""`, false},
+ {"foo", `"foo"`, false},
// Newlines.
- {"\r\n\u2028\u2029", `"\r\n\u2028\u2029"`},
+ {"\r\n\u2028\u2029", `"\r\n\u2028\u2029"`, false},
// "\v" == "v" on IE 6 so use "\u000b" instead.
- {"\t\x0b", `"\t\u000b"`},
- {struct{ X, Y int }{1, 2}, `{"X":1,"Y":2}`},
- {[]any{}, "[]"},
- {[]any{42, "foo", nil}, `[42,"foo",null]`},
- {[]string{"<!--", "</script>", "-->"}, `["\u003c!--","\u003c/script\u003e","--\u003e"]`},
- {"<!--", `"\u003c!--"`},
- {"-->", `"--\u003e"`},
- {"<![CDATA[", `"\u003c![CDATA["`},
- {"]]>", `"]]\u003e"`},
- {"</script", `"\u003c/script"`},
- {"\U0001D11E", "\"\U0001D11E\""}, // or "\uD834\uDD1E"
- {nil, " null "},
+ {"\t\x0b", `"\t\u000b"`, false},
+ {struct{ X, Y int }{1, 2}, `{"X":1,"Y":2}`, false},
+ {[]any{}, "[]", false},
+ {[]any{42, "foo", nil}, `[42,"foo",null]`, false},
+ {[]string{"<!--", "</script>", "-->"}, `["\u003c!--","\u003c/script\u003e","--\u003e"]`, false},
+ {"<!--", `"\u003c!--"`, false},
+ {"-->", `"--\u003e"`, false},
+ {"<![CDATA[", `"\u003c![CDATA["`, false},
+ {"]]>", `"]]\u003e"`, false},
+ {"</script", `"\u003c/script"`, false},
+ {"\U0001D11E", "\"\U0001D11E\"", false}, // or "\uD834\uDD1E"
+ {nil, " null ", false},
+ {&jsonErrType{}, " /* json: error calling MarshalJSON for type *template.jsonErrType: beep * / boop \\x3C/script blip \\x3C!-- */null ", true},
}
for _, test := range tests {
if js := jsValEscaper(test.x); js != test.js {
t.Errorf("%+v: want\n\t%q\ngot\n\t%q", test.x, test.js, js)
}
+ if test.skipNest {
+ continue
+ }
// Make sure that escaping corner cases are not broken
// by nesting.
a := []any{test.x}
--
2.33.0
|
