Merge pull request #1325 from chaitin/ci-fix-security-rescan-findings

fix(management,mcp-server,yanshi): close the second security scan round

Author: jlongWang

compose.yaml

@@ -43,6 +43,11 @@ services:
       test: curl -k -f https://localhost:1443/api/open/health
     environment:
       - MGT_PG=postgres://safeline-ce:${POSTGRES_PASSWORD}@safeline-pg/safeline-ce?sslmode=disable
+      # Optional override for the control channel token that tcontrollerd has to
+      # present before it may subscribe. Leave it empty to use
+      # ${SAFELINE_DIR}/resources/sock/mgt_grpc_token, which this container
+      # creates and the tengine container reads.
+      - MGT_GRPC_TOKEN=${MGT_GRPC_TOKEN:-}
     depends_on:
       - postgres
       - fvm
@@ -83,6 +88,9 @@ services:
     environment:
       - TCD_MGT_API=https://${SUBNET_PREFIX}.4:1443/api/open/publish/server
       - TCD_SNSERVER=${SUBNET_PREFIX}.5:8000
+      # Optional override for the control channel token; empty means the token
+      # file in ${SAFELINE_DIR}/resources/sock is used.
+      - TCD_MGT_TOKEN=${MGT_GRPC_TOKEN:-}
       # deprecated
       - SNSERVER_ADDR=${SUBNET_PREFIX}.5:8000
       - CHAOS_ADDR=${SUBNET_PREFIX}.10
@@ -140,4 +148,4 @@ services:
       - ${SAFELINE_DIR}/resources/chaos:/app/chaos
     networks:
       safeline-ce:
-        ipv4_address: ${SUBNET_PREFIX}.10
+        ipv4_address: ${SUBNET_PREFIX}.10

management/tcontrollerd/config.yml

@@ -2,4 +2,9 @@
 log:
   output: stdout              # "stdout", "stderr" or file path
   level:  debug               # "debug", "info", "warn" or "error"
-mgt_addr: 169.254.0.4:9002        # gRPC addr of mgt-api webserver
+mgt_addr: 169.254.0.4:9002        # gRPC addr of mgt-api webserver
+# Shared secret of the control channel. mgt-api creates it in a directory that
+# both containers mount (see the safeLine compose file); set MGT_TOKEN_FILE to
+# point somewhere else, or TCD_MGT_TOKEN to pass the value through the
+# environment instead.
+# mgt_token_file: /app/sock/mgt_grpc_token

management/tcontrollerd/controller/controller.go

@@ -1,9 +1,15 @@
 package controller
 
 import (
+	"context"
+	"os"
+	"strings"
+
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/grpc/metadata"
 
+	"chaitin.cn/dev/go/errors"
 	"chaitin.cn/patronus/safeline-2/management/tcontrollerd/pkg/config"
 	"chaitin.cn/patronus/safeline-2/management/tcontrollerd/pkg/log"
 	pb "chaitin.cn/patronus/safeline-2/management/tcontrollerd/proto/website"
@@ -13,8 +19,26 @@ var (
 	logger = log.GetLogger("controller")
 )
 
+const (
+	// TokenEnv holds the control channel token itself.
+	TokenEnv = "TCD_MGT_TOKEN"
+
+	// tokenMetadataKey is the metadata key that carries the token to the
+	// management server. gRPC metadata keys have to be lower case.
+	tokenMetadataKey = "mgt-token"
+)
+
 func Handle() error {
 	logger.Infof("Connect mgt-webserver at %s", config.GlobalConfig.MgtWebserver)
+
+	// The management server only accepts a subscription that proves it belongs
+	// to this installation.
+	token, err := controlToken()
+	if err != nil {
+		logger.Errorf("Fail to read the control channel token: %v", err)
+		return err
+	}
+
 	gRPCConn, err := grpc.Dial(config.GlobalConfig.MgtWebserver, []grpc.DialOption{
 		grpc.WithTransportCredentials(insecure.NewCredentials()),
 	}...)
@@ -33,9 +57,34 @@ func Handle() error {
 		}
 	}(gRPCConn)
 
-	if err = websiteHandler(wsClient); err != nil {
+	ctx := metadata.AppendToOutgoingContext(context.Background(), tokenMetadataKey, token)
+	if err = websiteHandler(ctx, wsClient); err != nil {
 		return err
 	}
 
 	return nil
 }
+
+// controlToken returns the shared secret of the control channel.
+//
+// The value is taken from the environment when it is set there, and from the
+// token file otherwise. The management server creates that file in the
+// directory both containers mount, so the standard deployment needs no
+// configuration.
+func controlToken() (string, error) {
+	if token := strings.TrimSpace(os.Getenv(TokenEnv)); token != "" {
+		return token, nil
+	}
+
+	content, err := os.ReadFile(config.GlobalConfig.MgtTokenFile)
+	if err != nil {
+		return "", errors.Wrapf(err, "read the control channel token from %s", config.GlobalConfig.MgtTokenFile)
+	}
+
+	token := strings.TrimSpace(string(content))
+	if token == "" {
+		return "", errors.New("the control channel token file " + config.GlobalConfig.MgtTokenFile + " is empty")
+	}
+
+	return token, nil
+}

management/tcontrollerd/controller/validation.go

@@ -2,6 +2,7 @@ package controller
 
 import (
 	"fmt"
+	"path/filepath"
 	"regexp"
 	"strconv"
 	"strings"
@@ -25,8 +26,44 @@ var (
 	// nginx "server_name" directive, so anything that could terminate the
 	// directive (or start another one) must not pass.
 	serverNameLabelPattern = regexp.MustCompile(`^[A-Za-z0-9_]([A-Za-z0-9_-]*[A-Za-z0-9_])?$`)
+
+	// certFilenamePattern matches the file name of a certificate or a key. The
+	// name is rendered into "ssl_certificate" and "ssl_certificate_key", and
+	// it is joined to the certificate directory, so only a plain file name
+	// without a path separator, a control character or any nginx syntax may
+	// pass.
+	certFilenamePattern = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`)
 )
 
+// maxCertFilenameLength bounds a certificate file name.
+const maxCertFilenameLength = 255
+
+// validateCertFilename accepts the name of a file in the certificate directory.
+//
+// The management server validates the same value before it stores it, but this
+// side cannot rely on that: the configuration is pushed over the network, and a
+// name carrying nginx syntax would turn into directives that run commands.
+func validateCertFilename(name string) error {
+	// An empty name means "this site is served over plain HTTP".
+	if name == "" {
+		return nil
+	}
+
+	if len(name) > maxCertFilenameLength {
+		return fmt.Errorf("invalid certificate file name %q: longer than %d characters", name, maxCertFilenameLength)
+	}
+
+	if name != filepath.Base(name) || name == "." || name == ".." {
+		return fmt.Errorf("invalid certificate file name %q: only a file name without a directory is allowed", name)
+	}
+
+	if !certFilenamePattern.MatchString(name) {
+		return fmt.Errorf("invalid certificate file name %q: only letters, digits, dot, dash and underscore are allowed", name)
+	}
+
+	return nil
+}
+
 // validatePort accepts the decimal notation of a TCP port. It rejects any
 // other value because the port is interpolated into a "listen" directive.
 func validatePort(port string) error {

management/tcontrollerd/controller/website.go

@@ -30,6 +30,10 @@ const (
 	nginxBackupFilePrefix = "BAK_IF_backend_"
 	nginxFileMode         = 0644
 
+	// nginxCertsPath is the directory the ssl_certificate templates below point
+	// at; certificate file names have to stay inside it.
+	nginxCertsPath = "/etc/nginx/certs"
+
 	HttpsScheme      = "https"
 	DefaultHttpsPort = "443"
 )
@@ -58,6 +62,19 @@ func generateNginxConfig(website *model.WebsiteConfig) (string, error) {
 	sslCertFilename := ""
 	sslCertKeyFilename := ""
 	if website.KeyFilename != "" && website.CertFilename != "" {
+		for _, filename := range []string{website.CertFilename, website.KeyFilename} {
+			if err := validateCertFilename(filename); err != nil {
+				return "", err
+			}
+
+			// The template renders the name into an absolute path, so an
+			// explicit check backs up the name validation above: the resolved
+			// path has to stay inside the certificate directory.
+			if filepath.Dir(filepath.Join(nginxCertsPath, filename)) != nginxCertsPath {
+				return "", fmt.Errorf("invalid certificate file name %q: it leaves %s", filename, nginxCertsPath)
+			}
+		}
+
 		sslFlag = " ssl" // with a blank ahead
 		sslCertFilename = fmt.Sprintf(certTpl, website.CertFilename)
 		sslCertKeyFilename = fmt.Sprintf(certKeyTpl, website.KeyFilename)
@@ -110,11 +127,28 @@ func generateFullConfigAndReload(msg []byte) error {
 		return err
 	}
 
+	if err := utils.EnsureDir(nginxConfigPath); err != nil {
+		return err
+	}
+
 	configFilename := make(map[string]struct{})
 	for _, website := range websites {
 		configFilename[fmt.Sprintf("%s%d", nginxFilePrefix, website.Id)] = struct{}{}
 	}
-	filepath.Walk(nginxConfigPath, func(path string, info fs.FileInfo, err error) error {
+
+	if err := filepath.Walk(nginxConfigPath, func(path string, info fs.FileInfo, err error) error {
+		// filepath.Walk reports the entries it could not read through err and
+		// leaves info nil in that case: touching info before this check is a
+		// nil pointer dereference that takes the whole daemon down.
+		if err != nil {
+			logger.Warnf("Skip %s: %v", path, err)
+			return nil
+		}
+
+		if info == nil || info.IsDir() {
+			return nil
+		}
+
 		_, ok := configFilename[info.Name()]
 		if !ok && strings.HasPrefix(info.Name(), nginxFilePrefix) {
 			if err := os.Remove(path); err != nil {
@@ -124,7 +158,9 @@ func generateFullConfigAndReload(msg []byte) error {
 		}
 
 		return nil
-	})
+	}); err != nil {
+		return err
+	}
 
 	for _, website := range websites {
 		if err := generateConfigAndReload(&website); err != nil {
@@ -231,8 +267,8 @@ func sendResponse(stream pb.Website_SubscribeClient, eventType string, errMsg []
 	})
 }
 
-func websiteHandler(wc pb.WebsiteClient) error {
-	stream, err := wc.Subscribe(context.Background())
+func websiteHandler(ctx context.Context, wc pb.WebsiteClient) error {
+	stream, err := wc.Subscribe(ctx)
 	if err != nil {
 		logger.Errorf("Subscribe failed: %v", err)
 		return err

management/tcontrollerd/pkg/config/config.go

@@ -10,6 +10,12 @@ var (
 	GlobalConfig = DefaultGlobalConfig()
 )
 
+// DefaultMgtTokenFile is where the control channel token is read from by
+// default. The standard deployment mounts the same host directory at
+// /app/sock inside the management container, which creates the file, and
+// inside the tengine container, which is where tcontrollerd runs.
+const DefaultMgtTokenFile = "/app/sock/mgt_grpc_token"
+
 func InitConfigs(configFilePath string) error {
 	s, err := settings.New(configFilePath)
 	if err != nil {
@@ -28,5 +34,15 @@ func InitConfigs(configFilePath string) error {
 		GlobalConfig.MgtWebserver = v
 	}
 
+	// The token file is optional in the configuration file: the default is
+	// where the management server puts it.
+	if err := s.Unmarshal("mgt_token_file", &GlobalConfig.MgtTokenFile); err != nil {
+		return err
+	}
+
+	if v, ok := os.LookupEnv("MGT_TOKEN_FILE"); ok {
+		GlobalConfig.MgtTokenFile = v
+	}
+
 	return nil
 }

management/tcontrollerd/pkg/config/global.go

@@ -3,11 +3,13 @@ package config
 type Config struct {
 	Log          LogConfig
 	MgtWebserver string
+	MgtTokenFile string
 }
 
 func DefaultGlobalConfig() Config {
 	return Config{
 		Log:          DefaultLogConfig(),
 		MgtWebserver: "169.254.0.4:9002",
+		MgtTokenFile: DefaultMgtTokenFile,
 	}
 }

management/tcontrollerd/pkg/log/log.go

@@ -51,11 +51,17 @@ func InitLogger() error {
 			fileFlag = fileFlag | os.O_CREATE
 		}
 
-		if fp, err := os.OpenFile(config.GlobalConfig.Log.Output, fileFlag, os.ModePerm); err != nil {
+		// os.ModePerm (0777) let every local user rewrite the audit log. The
+		// file is created with an owner/group readable mode instead.
+		if fp, err := os.OpenFile(config.GlobalConfig.Log.Output, fileFlag, logFileMode); err != nil {
 			return fmt.Errorf("failed to open log file: %s", err.Error())
 		} else {
 			log.SetOutput(log.AllLoggers, log.NewLockOutput(fp))
 		}
+
+		// The mode above only applies when the file is created, so a log file
+		// that an older release left world writable is tightened here.
+		tightenLogFileMode(config.GlobalConfig.Log.Output)
 	}
 
 	// hook
@@ -67,6 +73,28 @@ func InitLogger() error {
 	return nil
 }
 
+// logFileMode is the permission the log file is created with. Logs are audit
+// material: they must not be writable by other local users.
+const logFileMode os.FileMode = 0640
+
+// looseLogFileMask matches the group and world write bits.
+const looseLogFileMask os.FileMode = 0o022
+
+// tightenLogFileMode removes the group and world write bits from an existing
+// log file, so that a log file created by an older release cannot stay
+// tamperable by other local users.
+func tightenLogFileMode(path string) {
+	info, err := os.Stat(path)
+	if err != nil || info.Mode().Perm()&looseLogFileMask == 0 {
+		return
+	}
+
+	log.Warnf("Log file %s is writable by other users, setting mode to %#o", path, logFileMode)
+	if err := os.Chmod(path, logFileMode); err != nil {
+		log.Warnf("Cannot tighten the mode of log file %s: %s", path, err)
+	}
+}
+
 type RuntimeHook struct{}
 
 func (h *RuntimeHook) Levels() []logrus.Level {

management/tcontrollerd/utils/file.go

@@ -50,11 +50,39 @@ func FilesExist(paths ...string) (bool, error) {
 }
 
 func RenameWriteFile(filename string, data []byte, perm os.FileMode) error {
-	randFileName := filename + ".tmp." + RandStr(8)
-	if err := ioutil.WriteFile(randFileName, data, perm); err != nil {
+	// os.CreateTemp creates the file with O_CREATE|O_EXCL and mode 0600, so a
+	// file or symlink planted at the temporary path beforehand cannot be
+	// followed. The previous implementation guessed the name with math/rand and
+	// wrote through ioutil.WriteFile, which follows symlinks.
+	tmpFile, err := os.CreateTemp(filepath.Dir(filename), filepath.Base(filename)+".tmp.")
+	if err != nil {
 		return err
 	}
-	return os.Rename(randFileName, filename)
+	tmpName := tmpFile.Name()
+
+	renamed := false
+	defer func() {
+		if !renamed {
+			os.Remove(tmpName)
+		}
+	}()
+
+	if err = tmpFile.Chmod(perm); err != nil {
+		tmpFile.Close()
+		return err
+	}
+	if _, err = tmpFile.Write(data); err != nil {
+		tmpFile.Close()
+		return err
+	}
+	if err = tmpFile.Close(); err != nil {
+		return err
+	}
+	if err = os.Rename(tmpName, filename); err != nil {
+		return err
+	}
+	renamed = true
+	return nil
 }
 
 func EnsureRenameWriteFile(path string, data []byte, mode os.FileMode) error {