Skip to content

Commit 159a5c5

Browse files
MasonMMasonM
authored
fix(security): stored XSS in artifact directory listings (#15255)
Signed-off-by: Mason Malone <651224+MasonM@users.noreply.github.com>
1 parent 2824d21 commit 159a5c5

File tree

3 files changed

+159
-45
lines changed

3 files changed

+159
-45
lines changed

‎server/artifacts/artifact_server.go‎

Lines changed: 53 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
package artifacts
22

33
import (
4+
"bytes"
45
"context"
56
"errors"
67
"fmt"
8+
"html/template"
79
"io"
810
"mime"
911
"net/http"
@@ -218,31 +220,14 @@ func (a *ArtifactServer) GetArtifactFile(w http.ResponseWriter, r *http.Request)
218220
return
219221
}
220222
}
221-
222-
w.WriteHeader(http.StatusOK)
223-
_, _ = w.Write([]byte("<html><body><ul>\n"))
224-
225-
dirs := map[string]bool{} // to de-dupe sub-dirs
226-
227-
_, _ = fmt.Fprintf(w, "<li><a href=\"%s\">%s</a></li>\n", "..", "..")
228-
229-
for _, object := range objects {
230-
231-
// object is prefixed the key, we must trim it
232-
dir, file := path.Split(strings.TrimPrefix(object, key+"/"))
233-
234-
// if dir is empty string, we are in the root dir
235-
if dir == "" {
236-
_, _ = fmt.Fprintf(w, "<li><a href=\"%s\">%s</a></li>\n", file, file)
237-
} else if dirs[dir] {
238-
continue
239-
} else {
240-
_, _ = fmt.Fprintf(w, "<li><a href=\"%s\">%s</a></li>\n", dir, dir)
241-
dirs[dir] = true
242-
}
223+
a.setSecurityHeaders(w)
224+
output, err := a.renderDirectoryListing(objects, key)
225+
if err != nil {
226+
a.serverInternalError(ctx, err, w)
227+
return
243228
}
244-
_, _ = w.Write([]byte("</ul></body></html>"))
245-
229+
w.WriteHeader(http.StatusOK)
230+
_, _ = w.Write(output)
246231
} else { // stream the file itself
247232
a.logger.WithFields(logging.Fields{
248233
"artifact": artifact,
@@ -257,6 +242,42 @@ func (a *ArtifactServer) GetArtifactFile(w http.ResponseWriter, r *http.Request)
257242

258243
}
259244

245+
func (a *ArtifactServer) renderDirectoryListing(objects []string, key string) ([]byte, error) {
246+
output := bytes.NewBufferString("<html><body><ul>\n<li><a href=\"..\">..</a></li>\n")
247+
248+
dirs := map[string]bool{} // to de-dupe sub-dirs
249+
250+
// Use html/template to prevent XSS attacks.
251+
// The "./" prefix is necessary so the template engine recognizes it as a relative URL.
252+
// Without that, a file called "javascript:alert(1)" would be escaped to "#ZgotmplZ" by the urlFilter.
253+
tmpl, err := template.New("list").Parse("<li><a href=\"./{{.}}\">{{.}}</a></li>\n")
254+
if err != nil {
255+
return nil, err
256+
}
257+
258+
for _, object := range objects {
259+
260+
// object is prefixed the key, we must trim it
261+
dir, file := path.Split(strings.TrimPrefix(object, key+"/"))
262+
263+
// if dir is empty string, we are in the root dir
264+
if dir == "" {
265+
if err = tmpl.Execute(output, file); err != nil {
266+
return nil, err
267+
}
268+
} else if dirs[dir] {
269+
continue
270+
} else {
271+
if err = tmpl.Execute(output, dir); err != nil {
272+
return nil, err
273+
}
274+
dirs[dir] = true
275+
}
276+
}
277+
_, _ = output.WriteString("</ul></body></html>")
278+
return output.Bytes(), nil
279+
}
280+
260281
func (a *ArtifactServer) getArtifact(w http.ResponseWriter, r *http.Request, isInput bool) {
261282
requestPath := strings.SplitN(r.URL.Path, "/", 6)
262283
if len(requestPath) != 6 {
@@ -502,6 +523,13 @@ func (a *ArtifactServer) getArtifactAndDriver(ctx context.Context, nodeID, artif
502523
return art, driver, nil
503524
}
504525

526+
func (a *ArtifactServer) setSecurityHeaders(w http.ResponseWriter) {
527+
// Set strict CSP headers for defense-in-depth against XSS: https://web.dev/articles/strict-csp
528+
w.Header().Add("Content-Security-Policy", env.GetString("ARGO_ARTIFACT_CONTENT_SECURITY_POLICY", "sandbox; base-uri 'none'; default-src 'none'; img-src 'self'; style-src 'self' 'unsafe-inline'"))
529+
// Mitigate clickjacking attacks
530+
w.Header().Add("X-Frame-Options", env.GetString("ARGO_ARTIFACT_X_FRAME_OPTIONS", "SAMEORIGIN"))
531+
}
532+
505533
func (a *ArtifactServer) returnArtifact(ctx context.Context, w http.ResponseWriter, art *wfv1.Artifact, driver common.ArtifactDriver) error {
506534
logger := logging.RequireLoggerFromContext(ctx)
507535
stream, err := driver.OpenStream(ctx, art)
@@ -518,8 +546,7 @@ func (a *ArtifactServer) returnArtifact(ctx context.Context, w http.ResponseWrit
518546
key, _ := art.GetKey()
519547
w.Header().Add("Content-Disposition", fmt.Sprintf(`filename="%s"`, path.Base(key)))
520548
w.Header().Add("Content-Type", mime.TypeByExtension(path.Ext(key)))
521-
w.Header().Add("Content-Security-Policy", env.GetString("ARGO_ARTIFACT_CONTENT_SECURITY_POLICY", "sandbox; base-uri 'none'; default-src 'none'; img-src 'self'; style-src 'self' 'unsafe-inline'"))
522-
w.Header().Add("X-Frame-Options", env.GetString("ARGO_ARTIFACT_X_FRAME_OPTIONS", "SAMEORIGIN"))
549+
a.setSecurityHeaders(w)
523550

524551
_, err = io.Copy(w, stream)
525552
if err != nil {

‎server/artifacts/artifact_server_test.go‎

Lines changed: 97 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -134,18 +134,27 @@ func (a *fakeArtifactDriver) ListObjects(_ context.Context, artifact *wfv1.Artif
134134
return nil, err
135135
}
136136
if artifact.Name == "my-s3-artifact-directory" {
137+
prefix := "my-wf/my-node-1/my-s3-artifact-directory"
138+
subdir := []string{
139+
prefix + "/subdirectory/b.txt",
140+
prefix + "/subdirectory/c.txt",
141+
}
142+
// XSS test strings. Loosely adapted from https://cheatsheetseries.owasp.org/cheatsheets/XSS_Filter_Evasion_Cheat_Sheet.html#waf-bypass-strings-for-xss
143+
xss := []string{
144+
prefix + `/xss/xss\"><img src=x onerror="alert(document.domain)">.html`,
145+
prefix + `/xss/javascript:alert(document.domain)`,
146+
prefix + `/xss/javascript:\u0061lert(1)`,
147+
prefix + `/xss/<Input value = "XSS" type = text>`,
148+
}
137149
if strings.HasSuffix(key, "subdirectory") {
138-
return []string{
139-
"my-wf/my-node-1/my-s3-artifact-directory/subdirectory/b.txt",
140-
"my-wf/my-node-1/my-s3-artifact-directory/subdirectory/c.txt",
141-
}, nil
150+
return subdir, nil
151+
} else if strings.HasSuffix(key, "xss") {
152+
return xss, nil
142153
} else {
143-
return []string{
144-
"my-wf/my-node-1/my-s3-artifact-directory/a.txt",
145-
"my-wf/my-node-1/my-s3-artifact-directory/index.html",
146-
"my-wf/my-node-1/my-s3-artifact-directory/subdirectory/b.txt",
147-
"my-wf/my-node-1/my-s3-artifact-directory/subdirectory/c.txt",
148-
}, nil
154+
return append(append([]string{
155+
prefix + "/a.txt",
156+
prefix + "/index.html",
157+
}, subdir...), xss...), nil
149158
}
150159
}
151160
return []string{}, nil
@@ -402,9 +411,21 @@ func TestArtifactServer_GetArtifactFile(t *testing.T) {
402411
statusCode: 200,
403412
isDirectory: true,
404413
directoryFiles: []string{
405-
"..",
406-
"b.txt",
407-
"c.txt",
414+
`<a href="..">..</a>`,
415+
`<a href="./b.txt">b.txt</a>`,
416+
`<a href="./c.txt">c.txt</a>`,
417+
},
418+
},
419+
{
420+
path: "/artifact-files/my-ns/workflows/my-wf/my-node-1/outputs/my-s3-artifact-directory/xss/",
421+
statusCode: 200,
422+
isDirectory: true,
423+
directoryFiles: []string{
424+
`<a href="..">..</a>`,
425+
`<a href="./xss%5c%22%3e%3cimg%20src=x%20onerror=%22alert%28document.domain%29%22%3e.html">xss\&#34;&gt;&lt;img src=x onerror=&#34;alert(document.domain)&#34;&gt;.html</a>`,
426+
`<a href="./javascript:alert%28document.domain%29">javascript:alert(document.domain)</a></li>`,
427+
`<a href="./javascript:%5cu0061lert%281%29">javascript:\u0061lert(1)</a>`,
428+
`<a href="./%3cInput%20value%20=%20%22XSS%22%20type%20=%20text%3e">&lt;Input value = &#34;XSS&#34; type = text&gt;</a>`,
408429
},
409430
},
410431
{
@@ -417,9 +438,9 @@ func TestArtifactServer_GetArtifactFile(t *testing.T) {
417438
statusCode: 200,
418439
isDirectory: true,
419440
directoryFiles: []string{
420-
"..",
421-
"b.txt",
422-
"c.txt",
441+
`<a href="..">..</a>`,
442+
`<a href="./b.txt">b.txt</a>`,
443+
`<a href="./c.txt">c.txt</a>`,
423444
},
424445
},
425446
{
@@ -476,6 +497,8 @@ func TestArtifactServer_GetArtifactFile(t *testing.T) {
476497
}
477498
if tt.isDirectory {
478499
fmt.Printf("got directory listing:\n%s\n", all)
500+
assert.Contains(t, recorder.Header().Get("Content-Security-Policy"), "sandbox")
501+
assert.Equal(t, "SAMEORIGIN", recorder.Header().Get("X-Frame-Options"))
479502
// verify that the files are contained in the listing we got back
480503
assert.Len(t, tt.directoryFiles, strings.Count(string(all), "<li>"))
481504
for _, file := range tt.directoryFiles {
@@ -490,6 +513,64 @@ func TestArtifactServer_GetArtifactFile(t *testing.T) {
490513
}
491514
}
492515

516+
func TestArtifactServer_RenderDirectoryListings(t *testing.T) {
517+
s := newServer(t)
518+
519+
t.Run("Empty Directory", func(t *testing.T) {
520+
expected := `<html><body><ul>
521+
<li><a href="..">..</a></li>
522+
</ul></body></html>`
523+
actual, err := s.renderDirectoryListing([]string{}, "")
524+
require.NoError(t, err)
525+
assert.Equal(t, expected, string(actual))
526+
})
527+
528+
t.Run("Single File", func(t *testing.T) {
529+
expected := `<html><body><ul>
530+
<li><a href="..">..</a></li>
531+
<li><a href="./foo.html">foo.html</a></li>
532+
</ul></body></html>`
533+
actual, err := s.renderDirectoryListing([]string{"foo.html"}, "")
534+
require.NoError(t, err)
535+
assert.Equal(t, expected, string(actual))
536+
})
537+
538+
t.Run("Nested Files", func(t *testing.T) {
539+
expected := `<html><body><ul>
540+
<li><a href="..">..</a></li>
541+
<li><a href="./foo.html">foo.html</a></li>
542+
<li><a href="./dir/">dir/</a></li>
543+
<li><a href="./dir2/">dir2/</a></li>
544+
</ul></body></html>`
545+
actual, err := s.renderDirectoryListing([]string{
546+
"dir/foo.html",
547+
"dir/dir/bar.html",
548+
"dir/dir2/baz.html",
549+
"dir/dir/bar2.html",
550+
}, "dir")
551+
require.NoError(t, err)
552+
assert.Equal(t, expected, string(actual))
553+
})
554+
555+
t.Run("XSS Filtering", func(t *testing.T) {
556+
expected := `<html><body><ul>
557+
<li><a href="..">..</a></li>
558+
<li><a href="./xss%5c%22%3e%3cimg%20src=x%20onerror=%22alert%28document.domain%29%22%3e.html">xss\&#34;&gt;&lt;img src=x onerror=&#34;alert(document.domain)&#34;&gt;.html</a></li>
559+
<li><a href="./javascript:alert%28document.domain%29">javascript:alert(document.domain)</a></li>
560+
<li><a href="./javascript:%5cu0061lert%281%29">javascript:\u0061lert(1)</a></li>
561+
<li><a href="./%3cInput%20value%20=%20%22XSS%22%20type%20=%20text%3e">&lt;Input value = &#34;XSS&#34; type = text&gt;</a></li>
562+
</ul></body></html>`
563+
actual, err := s.renderDirectoryListing([]string{
564+
`xss\"><img src=x onerror="alert(document.domain)">.html`,
565+
`javascript:alert(document.domain)`,
566+
`javascript:\u0061lert(1)`,
567+
`<Input value = "XSS" type = text>`,
568+
}, "")
569+
require.NoError(t, err)
570+
assert.Equal(t, expected, string(actual))
571+
})
572+
}
573+
493574
func TestArtifactServer_GetOutputArtifact(t *testing.T) {
494575
s := newServer(t)
495576

‎test/e2e/argo_server_test.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1728,7 +1728,13 @@ func (s *ArgoServerSuite) artifactServerRetrievalTests(name string, uid types.UI
17281728
Status(200)
17291729

17301730
resp.Body().
1731-
Contains("<a href=\"subdirectory/\">subdirectory/</a>")
1731+
Contains("<a href=\"./subdirectory/\">subdirectory/</a>")
1732+
1733+
resp.Header("Content-Security-Policy").
1734+
IsEqual("sandbox; base-uri 'none'; default-src 'none'; img-src 'self'; style-src 'self' 'unsafe-inline'")
1735+
1736+
resp.Header("X-Frame-Options").
1737+
IsEqual("SAMEORIGIN")
17321738

17331739
})
17341740

@@ -1739,8 +1745,8 @@ func (s *ArgoServerSuite) artifactServerRetrievalTests(name string, uid types.UI
17391745
Status(200)
17401746

17411747
resp.Body().
1742-
Contains("<a href=\"sub-file-1\">sub-file-1</a>").
1743-
Contains("<a href=\"sub-file-2\">sub-file-2</a>")
1748+
Contains("<a href=\"./sub-file-1\">sub-file-1</a>").
1749+
Contains("<a href=\"./sub-file-2\">sub-file-2</a>")
17441750

17451751
})
17461752

0 commit comments

Comments
 (0)