Merge pull request #45559 from rmmh/no-xss

Automatic merge from submit-queue

HTML escape apiserver errors to avoid triggering vulnerability scanners.

Simple XSS scans might fetch /<script>alert('vulnerable')</script>, and
fail when the response body includes the script tag verbatim, despite
the headers directing the browser to interpret the response as text.

This isn't a real vulnerability, but it's easier to fix this here than
it is to fix the scanners.


**Release note**:
```release-note
NONE
```

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/BUILD

@@ -10,13 +10,18 @@ load(
 
 go_test(
     name = "go_default_test",
-    srcs = ["status_test.go"],
+    srcs = [
+        "errors_test.go",
+        "status_test.go",
+    ],
     library = ":go_default_library",
     tags = ["automanaged"],
     deps = [
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",
     ],
 )
 

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors.go

@@ -19,22 +19,26 @@ package responsewriters
 import (
 	"fmt"
 	"net/http"
+	"strings"
 
 	"k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 )
 
+// Avoid emitting errors that look like valid HTML. Quotes are okay.
+var sanitizer = strings.NewReplacer(`&`, "&amp;", `<`, "&lt;", `>`, "&gt;")
+
 // BadGatewayError renders a simple bad gateway error.
 func BadGatewayError(w http.ResponseWriter, req *http.Request) {
 	w.Header().Set("Content-Type", "text/plain")
 	w.Header().Set("X-Content-Type-Options", "nosniff")
 	w.WriteHeader(http.StatusBadGateway)
-	fmt.Fprintf(w, "Bad Gateway: %#v", req.RequestURI)
+	fmt.Fprintf(w, "Bad Gateway: %q", sanitizer.Replace(req.RequestURI))
 }
 
 // Forbidden renders a simple forbidden error
 func Forbidden(attributes authorizer.Attributes, w http.ResponseWriter, req *http.Request, reason string) {
-	msg := forbiddenMessage(attributes)
+	msg := sanitizer.Replace(forbiddenMessage(attributes))
 	w.Header().Set("Content-Type", "text/plain")
 	w.Header().Set("X-Content-Type-Options", "nosniff")
 	w.WriteHeader(http.StatusForbidden)
@@ -76,12 +80,12 @@ func InternalError(w http.ResponseWriter, req *http.Request, err error) {
 	w.Header().Set("Content-Type", "text/plain")
 	w.Header().Set("X-Content-Type-Options", "nosniff")
 	w.WriteHeader(http.StatusInternalServerError)
-	fmt.Fprintf(w, "Internal Server Error: %#v: %v", req.RequestURI, err)
+	fmt.Fprintf(w, "Internal Server Error: %q: %v", sanitizer.Replace(req.RequestURI), err)
 	runtime.HandleError(err)
 }
 
 // NotFound renders a simple not found error.
 func NotFound(w http.ResponseWriter, req *http.Request) {
 	w.WriteHeader(http.StatusNotFound)
-	fmt.Fprintf(w, "Not Found: %#v", req.RequestURI)
+	fmt.Fprintf(w, "Not Found: %q", sanitizer.Replace(req.RequestURI))
 }

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go

@@ -0,0 +1,84 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package responsewriters
+
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"k8s.io/apiserver/pkg/authentication/user"
+	"k8s.io/apiserver/pkg/authorization/authorizer"
+)
+
+func TestErrors(t *testing.T) {
+	internalError := errors.New("ARGH")
+	fns := map[string]func(http.ResponseWriter, *http.Request){
+		"BadGatewayError": BadGatewayError,
+		"NotFound":        NotFound,
+		"InternalError": func(w http.ResponseWriter, req *http.Request) {
+			InternalError(w, req, internalError)
+		},
+	}
+	cases := []struct {
+		fn       string
+		uri      string
+		expected string
+	}{
+		{"BadGatewayError", "/get", `Bad Gateway: "/get"`},
+		{"BadGatewayError", "/<script>", `Bad Gateway: "/&lt;script&gt;"`},
+		{"NotFound", "/get", `Not Found: "/get"`},
+		{"NotFound", "/<script&>", `Not Found: "/&lt;script&amp;&gt;"`},
+		{"InternalError", "/get", `Internal Server Error: "/get": ARGH`},
+		{"InternalError", "/<script>", `Internal Server Error: "/&lt;script&gt;": ARGH`},
+	}
+	for _, test := range cases {
+		observer := httptest.NewRecorder()
+		fns[test.fn](observer, &http.Request{RequestURI: test.uri})
+		result := string(observer.Body.Bytes())
+		if result != test.expected {
+			t.Errorf("%s(..., %q) != %q, got %q", test.fn, test.uri, test.expected, result)
+		}
+	}
+}
+
+func TestForbidden(t *testing.T) {
+	u := &user.DefaultInfo{Name: "NAME"}
+	cases := []struct {
+		expected   string
+		attributes authorizer.Attributes
+		reason     string
+	}{
+		{`User "NAME" cannot GET path "/whatever".`,
+			authorizer.AttributesRecord{User: u, Verb: "GET", Path: "/whatever"}, ""},
+		{`User "NAME" cannot GET path "/&lt;script&gt;".`,
+			authorizer.AttributesRecord{User: u, Verb: "GET", Path: "/<script>"}, ""},
+		{`User "NAME" cannot GET pod at the cluster scope.`,
+			authorizer.AttributesRecord{User: u, Verb: "GET", Resource: "pod", ResourceRequest: true}, ""},
+		{`User "NAME" cannot GET pod.v2/quota in the namespace "test".`,
+			authorizer.AttributesRecord{User: u, Verb: "GET", Namespace: "test", APIGroup: "v2", Resource: "pod", Subresource: "quota", ResourceRequest: true}, ""},
+	}
+	for _, test := range cases {
+		observer := httptest.NewRecorder()
+		Forbidden(test.attributes, observer, &http.Request{}, test.reason)
+		result := string(observer.Body.Bytes())
+		if result != test.expected {
+			t.Errorf("Forbidden(%#v...) != %#v, got %#v", test.attributes, test.expected, result)
+		}
+	}
+}