summaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorAdin Scannell <ascannell@google.com>2021-01-07 15:28:41 -0800
committergVisor bot <gvisor-bot@google.com>2021-01-07 15:30:37 -0800
commit77b340ce82230e4e0bded01f43232c708328cd7e (patch)
treed098a443ba2a49718f2d07d8b62b99b79e572e2b
parent04b37c822022c27cb144e4af5ef21043a74127f3 (diff)
Require specific buckets for pprof handler.
This further restricts the surface exposed only to artifacts generated by the continuous integration system. This change also installs appropriate root certificates, so that objects can be fetched from https://storage.googleapis.com. PiperOrigin-RevId: 350650197
-rw-r--r--WORKSPACE11
-rw-r--r--website/BUILD3
-rw-r--r--website/cmd/server/main.go39
3 files changed, 44 insertions, 9 deletions
diff --git a/WORKSPACE b/WORKSPACE
index 72f9a89cc..c97db8603 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -1,6 +1,17 @@
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive", "http_file")
load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository")
+# Root certificates.
+#
+# Note that the sha256 hash is ommitted here intentionally. This should not be
+# used in any part of the build other than as certificates present in images.
+http_file(
+ name = "google_root_pem",
+ urls = [
+ "https://pki.goog/roots.pem"
+ ],
+)
+
# Bazel/starlark utilities.
http_archive(
name = "bazel_skylib",
diff --git a/website/BUILD b/website/BUILD
index 676c2b701..d5315abce 100644
--- a/website/BUILD
+++ b/website/BUILD
@@ -38,6 +38,7 @@ genrule(
":syscallmd",
"//website/blog:posts",
"//website/cmd/server",
+ "@google_root_pem//file",
],
outs = ["files.tgz"],
cmd = "set -x; " +
@@ -61,6 +62,8 @@ genrule(
"ruby /checks.rb " +
"/output && " +
"cp $(location //website/cmd/server) $$T/output/server && " +
+ "mkdir -p $$T/output/etc/ssl && " +
+ "cp $(location @google_root_pem//file) $$T/output/etc/ssl/cert.pem && " +
"tar -zcf $@ -C $$T/output . && " +
"rm -rf $$T",
tags = [
diff --git a/website/cmd/server/main.go b/website/cmd/server/main.go
index 601ccac5e..a1934f6f5 100644
--- a/website/cmd/server/main.go
+++ b/website/cmd/server/main.go
@@ -244,6 +244,14 @@ func (p *profileMeta) Parse(usage func()) []string {
// See registerProfile below.
const pprofFixedPrefix = "https://storage.googleapis.com/"
+// allowedBuckets enforces constraints on the pprof target.
+//
+// If the continuous integration system is changed in the future to use
+// additional buckets, they may be whitelisted here. See registerProfile.
+var allowedBuckets = map[string]bool{
+ "gvisor-buildkite": true,
+}
+
// Target returns the URL target.
func (p *profileMeta) Target() string {
return fmt.Sprintf("/profile/%s/", p.SourceURL[len(pprofFixedPrefix):])
@@ -259,7 +267,14 @@ func (p *profileMeta) HTTPServer(args *driver.HTTPServerArgs) error {
// directory path in order to keep relative links
// working in the profile. E.g. /profile/foo/ is the
// base URL for the profile at https://.../foo.
+ //
+ // The base target typically shows the dot-based graph,
+ // which will not work in the image (due to the lack of
+ // a dot binary to execute). Therefore, we redirect to
+ // the flamegraph handler. Everything should otherwise
+ // work the exact same way, except the "Graph" link.
handlerPath = target
+ handler = redirectHandler(path.Join(handlerPath, "flamegraph"))
}
p.Mux.Handle(handlerPath, handler)
}
@@ -273,10 +288,11 @@ func (p *profileMeta) HTTPServer(args *driver.HTTPServerArgs) error {
// We are passed effectively a URL, which we fetch and parse,
// then display the profile output. We limit the possibility of
// SSRF by interpreting the URL strictly as a part to an object
-// in Google Cloud Storage, but we allow the user to specify any
-// bucket (since this may change with the CI system).
+// in Google Cloud Storage, and further limit the buckets that
+// may be used. This contains the vast majority of concerns,
+// since objects must at least be uploaded by our CI system.
//
-// We additionally must consider the possibility that users may
+// However, we additionally consider the possibility that users
// craft malicious profile objects (somehow) and pass those URLs
// here as well. It seems feasible that we could parse a profile
// that causes a crash (DOS), but this would be automatically
@@ -294,17 +310,22 @@ func (p *profileMeta) HTTPServer(args *driver.HTTPServerArgs) error {
// causing an OOM and a crash. This has similar semantics to the
// DOS scenario above, and would just be handled by Cloud Run.
//
-// Finally, a malicious user could cause us to repeatedly fetch
-// extremely large objects. However, since we fetch objects via
-// the unauthenticated URL, such accesses would always be charged
-// to the object owner. Downloading large objects can lead to the
-// filling of the "disk" scenario above, but this is similarly a
-// minor issue and immediately mitigated.
+// Note that all of the above scenarios would require uploading
+// malicious profiles to controller buckets, and a clear audit
+// trail would exist in those cases.
func registerProfile(mux *http.ServeMux) {
const urlPrefix = "/profile/"
mux.Handle(urlPrefix, hostRedirectHandler(wrappedHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Extract the URL; this is everything except the final /.
parts := strings.Split(r.URL.Path[len(urlPrefix):], "/")
+ if len(parts) == 0 {
+ http.Error(w, "Invalid URL: no bucket provided.", http.StatusNotFound)
+ return
+ }
+ if !allowedBuckets[parts[0]] {
+ http.Error(w, fmt.Sprintf("Invalid URL: not an allowed bucket (%s).", parts[0]), http.StatusNotFound)
+ return
+ }
url := pprofFixedPrefix + strings.Join(parts[:len(parts)-1], "/")
if url == pprofFixedPrefix {
http.Error(w, "Invalid URL: no path provided.", http.StatusNotFound)