From 4a82b317b793d34d7d3323287ea665278a93a413 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Thu, 15 Sep 2022 22:08:45 -0700
Subject: [PATCH] ipn/{ipnlocal,localapi}: use strs.CutPrefix, add more domain
 validation

The GitHub CodeQL scanner flagged the localapi's cert domain usage as a problem
because user input in the URL made it to disk stat checks.

The domain is validated against the ipnstate.Status later, and only
authenticated root/configured users can hit this, but add some
paranoia anyway.

Change-Id: I373ef23832f1d8b3a27208bc811b6588ae5a1ddd
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 ipn/ipnlocal/peerapi.go   |  5 +++--
 ipn/localapi/cert.go      | 30 +++++++++++++++++++++++++++---
 ipn/localapi/cert_test.go | 30 ++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 5 deletions(-)
 create mode 100644 ipn/localapi/cert_test.go

diff --git a/ipn/ipnlocal/peerapi.go b/ipn/ipnlocal/peerapi.go
index 3932e3b73..1be0b51ab 100644
--- a/ipn/ipnlocal/peerapi.go
+++ b/ipn/ipnlocal/peerapi.go
@@ -44,6 +44,7 @@ import (
 	"tailscale.com/net/netutil"
 	"tailscale.com/tailcfg"
 	"tailscale.com/util/clientmetric"
+	"tailscale.com/util/strs"
 	"tailscale.com/wgengine"
 	"tailscale.com/wgengine/filter"
 )
@@ -720,8 +721,8 @@ func (h *peerAPIHandler) handlePeerPut(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	rawPath := r.URL.EscapedPath()
-	suffix := strings.TrimPrefix(rawPath, "/v0/put/")
-	if suffix == rawPath {
+	suffix, ok := strs.CutPrefix(rawPath, "/v0/put/")
+	if !ok {
 		http.Error(w, "misconfigured internals", 500)
 		return
 	}
diff --git a/ipn/localapi/cert.go b/ipn/localapi/cert.go
index 29c500469..6e136c95a 100644
--- a/ipn/localapi/cert.go
+++ b/ipn/localapi/cert.go
@@ -37,6 +37,7 @@ import (
 	"tailscale.com/envknob"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/types/logger"
+	"tailscale.com/util/strs"
 	"tailscale.com/version"
 	"tailscale.com/version/distro"
 )
@@ -86,12 +87,15 @@ func (h *Handler) serveCert(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	domain := strings.TrimPrefix(r.URL.Path, "/localapi/v0/cert/")
-	if domain == r.URL.Path {
+	domain, ok := strs.CutPrefix(r.URL.Path, "/localapi/v0/cert/")
+	if !ok {
 		http.Error(w, "internal handler config wired wrong", 500)
 		return
 	}
-
+	if !validLookingCertDomain(domain) {
+		http.Error(w, "invalid domain", 400)
+		return
+	}
 	now := time.Now()
 	logf := logger.WithPrefix(h.logf, fmt.Sprintf("cert(%q): ", domain))
 	traceACME := func(v any) {
@@ -164,6 +168,11 @@ func certFile(dir, domain string) string { return filepath.Join(dir, domain+".cr
 // keypair for domain exists on disk in dir that is valid at the
 // provided now time.
 func (h *Handler) getCertPEMCached(dir, domain string, now time.Time) (p *keyPair, ok bool) {
+	if !validLookingCertDomain(domain) {
+		// Before we read files from disk using it, validate it's halfway
+		// reasonable looking.
+		return nil, false
+	}
 	if keyPEM, err := os.ReadFile(keyFile(dir, domain)); err == nil {
 		certPEM, _ := os.ReadFile(certFile(dir, domain))
 		if validCertPEM(domain, keyPEM, certPEM, now) {
@@ -425,6 +434,21 @@ func validCertPEM(domain string, keyPEM, certPEM []byte, now time.Time) bool {
 	return err == nil
 }
 
+// validLookingCertDomain reports whether name looks like a valid domain name that
+// we might be able to get a cert for.
+//
+// It's a light check primarily for double checking before it's used
+// as part of a filesystem path. The actual validation happens in checkCertDomain.
+func validLookingCertDomain(name string) bool {
+	if name == "" ||
+		strings.Contains(name, "..") ||
+		strings.ContainsAny(name, ":/\\\x00") ||
+		!strings.Contains(name, ".") {
+		return false
+	}
+	return true
+}
+
 func checkCertDomain(st *ipnstate.Status, domain string) error {
 	if domain == "" {
 		return errors.New("missing domain name")
diff --git a/ipn/localapi/cert_test.go b/ipn/localapi/cert_test.go
new file mode 100644
index 000000000..ee896f440
--- /dev/null
+++ b/ipn/localapi/cert_test.go
@@ -0,0 +1,30 @@
+// Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build !ios && !android && !js
+// +build !ios,!android,!js
+
+package localapi
+
+import "testing"
+
+func TestValidLookingCertDomain(t *testing.T) {
+	tests := []struct {
+		in   string
+		want bool
+	}{
+		{"foo.com", true},
+		{"foo..com", false},
+		{"foo/com.com", false},
+		{"NUL", false},
+		{"", false},
+		{"foo\\bar.com", false},
+		{"foo\x00bar.com", false},
+	}
+	for _, tt := range tests {
+		if got := validLookingCertDomain(tt.in); got != tt.want {
+			t.Errorf("validLookingCertDomain(%q) = %v, want %v", tt.in, got, tt.want)
+		}
+	}
+}