security: disable plugin in default and persist file in specified dir (#7087)

Makefile

@@ -38,6 +38,10 @@ ifeq ("$(WITH_RACE)", "1")
 	BUILD_CGO_ENABLED := 1
 endif
 
+ifeq ($(PLUGIN), 1)
+	BUILD_TAGS += with_plugin
+endif
+
 LDFLAGS += -X "$(PD_PKG)/server/versioninfo.PDReleaseVersion=$(shell git describe --tags --dirty --always)"
 LDFLAGS += -X "$(PD_PKG)/server/versioninfo.PDBuildTS=$(shell date -u '+%Y-%m-%d %I:%M:%S')"
 LDFLAGS += -X "$(PD_PKG)/server/versioninfo.PDGitHash=$(shell git rev-parse HEAD)"

server/api/admin.go

@@ -133,7 +133,10 @@ func (h *adminHandler) ResetTS(w http.ResponseWriter, r *http.Request) {
 }
 
 // Intentionally no swagger mark as it is supposed to be only used in
-// server-to-server. For security reason, it only accepts JSON formatted data.
+// server-to-server.
+// For security reason,
+//   - it only accepts JSON formatted data.
+//   - it only accepts file name which is `DrStatusFile`.
 func (h *adminHandler) SavePersistFile(w http.ResponseWriter, r *http.Request) {
 	data, err := io.ReadAll(r.Body)
 	if err != nil {

server/api/admin_test.go

@@ -29,6 +29,7 @@ import (
 	tu "github.com/tikv/pd/pkg/testutil"
 	"github.com/tikv/pd/server"
 	"github.com/tikv/pd/server/core"
+	"github.com/tikv/pd/server/replication"
 )
 
 type adminTestSuite struct {
@@ -168,10 +169,10 @@ func (suite *adminTestSuite) TestDropRegions() {
 func (suite *adminTestSuite) TestPersistFile() {
 	data := []byte("#!/bin/sh\nrm -rf /")
 	re := suite.Require()
-	err := tu.CheckPostJSON(testDialClient, suite.urlPrefix+"/admin/persist-file/fun.sh", data, tu.StatusNotOK(re))
+	err := tu.CheckPostJSON(testDialClient, suite.urlPrefix+"/admin/persist-file/"+replication.DrStatusFile, data, tu.StatusNotOK(re))
 	suite.NoError(err)
 	data = []byte(`{"foo":"bar"}`)
-	err = tu.CheckPostJSON(testDialClient, suite.urlPrefix+"/admin/persist-file/good.json", data, tu.StatusOK(re))
+	err = tu.CheckPostJSON(testDialClient, suite.urlPrefix+"/admin/persist-file/"+replication.DrStatusFile, data, tu.StatusOK(re))
 	suite.NoError(err)
 }
 

server/api/plugin.go

@@ -12,6 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//go:build with_plugin
+// +build with_plugin
+
 package api
 
 import (

server/api/plugin_disable.go

@@ -0,0 +1,41 @@
+// Copyright 2023 TiKV Project 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.
+
+//go:build !with_plugin
+// +build !with_plugin
+
+package api
+
+import (
+	"net/http"
+
+	"github.com/tikv/pd/server"
+	"github.com/unrolled/render"
+)
+
+type pluginHandler struct{}
+
+func newPluginHandler(_ *server.Handler, _ *render.Render) *pluginHandler {
+	return &pluginHandler{}
+}
+
+func (h *pluginHandler) LoadPlugin(w http.ResponseWriter, r *http.Request) {
+	w.WriteHeader(http.StatusNotImplemented)
+	w.Write([]byte("load plugin is disabled, please `PLUGIN=1 MAKE pd-server` first"))
+}
+
+func (h *pluginHandler) UnloadPlugin(w http.ResponseWriter, r *http.Request) {
+	w.WriteHeader(http.StatusNotImplemented)
+	w.Write([]byte("unload plugin is disabled, please `PLUGIN=1 MAKE pd-server` first"))
+}

server/api/server_test.go

@@ -16,7 +16,9 @@ package api
 
 import (
 	"context"
+	"fmt"
 	"net/http"
+	"net/http/httptest"
 	"sort"
 	"sync"
 	"testing"
@@ -211,3 +213,24 @@ func (suite *serviceTestSuite) TestServiceLabels() {
 		apiutil.NewAccessPath("/pd/api/v1/metric/query", http.MethodGet))
 	suite.Equal("QueryMetric", serviceLabel)
 }
+
+func (suite *adminTestSuite) TestCleanPath() {
+	re := suite.Require()
+	// transfer path to /config
+	url := fmt.Sprintf("%s/admin/persist-file/../../config", suite.urlPrefix)
+	cfg := &config.Config{}
+	err := testutil.ReadGetJSON(re, testDialClient, url, cfg)
+	suite.NoError(err)
+
+	// handled by router
+	response := httptest.NewRecorder()
+	r, _, _ := NewHandler(context.Background(), suite.svr)
+	request, err := http.NewRequest(http.MethodGet, url, nil)
+	re.NoError(err)
+	r.ServeHTTP(response, request)
+	// handled by `cleanPath` which is in `mux.ServeHTTP`
+	result := response.Result()
+	defer result.Body.Close()
+	re.NotNil(result.Header["Location"])
+	re.Contains(result.Header["Location"][0], "/pd/api/v1/config")
+}

server/handler.go

@@ -21,6 +21,7 @@
 	"fmt"
 	"net/http"
 	"path"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -967,6 +968,13 @@
 	c := cluster.GetCoordinator()
 	ch := make(chan string)
 	h.pluginChMap[pluginPath] = ch
+
+	// make sure path is in data dir
+	filePath, err := filepath.Abs(pluginPath)
+	if err != nil || !isPathInDirectory(filePath, h.s.GetConfig().DataDir) {
+		return errs.ErrFilePathAbs.Wrap(err).FastGenWithCause()
+	}
+
 	c.LoadPlugin(pluginPath, ch)
 	return nil
 }

server/replication/replication_mode.go

@@ -60,7 +60,8 @@ type FileReplicater interface {
 	ReplicateFileToMember(ctx context.Context, member *pdpb.Member, name string, data []byte) error
 }
 
-const drStatusFile = "DR_STATE"
+// DrStatusFile is the file name that stores the dr status.
+const DrStatusFile = "DR_STATE"
 const persistFileTimeout = time.Second * 3
 
 // ModeManager is used to control how raft logs are synchronized between
@@ -483,7 +484,7 @@ func (m *ModeManager) tickReplicateStatus() {
 		stateID, ok := m.replicateState.Load(member.GetMemberId())
 		if !ok || stateID.(uint64) != state.StateID {
 			ctx, cancel := context.WithTimeout(context.Background(), persistFileTimeout)
-			err := m.fileReplicater.ReplicateFileToMember(ctx, member, drStatusFile, data)
+			err := m.fileReplicater.ReplicateFileToMember(ctx, member, DrStatusFile, data)
 			if err != nil {
 				log.Warn("failed to switch state", zap.String("replicate-mode", modeDRAutoSync), zap.String("new-state", state.State), errs.ZapError(err))
 			} else {

server/server.go

@@ -59,6 +59,7 @@
 	"github.com/tikv/pd/server/keyspace"
 	"github.com/tikv/pd/server/member"
 	syncer "github.com/tikv/pd/server/region_syncer"
+	"github.com/tikv/pd/server/replication"
 	"github.com/tikv/pd/server/schedule"
 	"github.com/tikv/pd/server/schedule/hbstream"
 	"github.com/tikv/pd/server/schedule/placement"
@@ -1678,8 +1679,15 @@
 
 // PersistFile saves a file in DataDir.
 func (s *Server) PersistFile(name string, data []byte) error {
+	if name != replication.DrStatusFile {
+		return errors.New("Invalid file name")
+	}
 	log.Info("persist file", zap.String("name", name), zap.Binary("data", data))
-	return os.WriteFile(filepath.Join(s.GetConfig().DataDir, name), data, 0644) // #nosec
+	path := filepath.Join(s.GetConfig().DataDir, name)
+	if !isPathInDirectory(path, s.GetConfig().DataDir) {
+		return errors.New("Invalid file path")
+	}
+	return os.WriteFile(path, data, 0644) // #nosec
 }
 
 // SaveTTLConfig save ttl config

server/server_test.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/suite"
@@ -337,3 +338,14 @@ func (suite *leaderServerTestSuite) TestSourceIpForHeaderBoth() {
 	bodyString := string(bodyBytes)
 	suite.Equal("Hello World\n", bodyString)
 }
+
+func (suite *leaderServerTestSuite) TestIsPathInDirectory() {
+	fileName := "test"
+	directory := "/root/project"
+	path := filepath.Join(directory, fileName)
+	suite.True(isPathInDirectory(path, directory))
+
+	fileName = "../../test"
+	path = filepath.Join(directory, fileName)
+	suite.False(isPathInDirectory(path, directory))
+}

server/util.go

@@ -18,6 +18,8 @@
 	"context"
 	"fmt"
 	"math/rand"
+	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/pingcap/errors"
@@ -157,3 +159,17 @@
 
 	return nil
 }
+
+func isPathInDirectory(path, directory string) bool {
+	absPath, err := filepath.Abs(path)
+	if err != nil {
+		return false
+	}
+
+	absDir, err := filepath.Abs(directory)
+	if err != nil {
+		return false
+	}
+
+	return strings.HasPrefix(absPath, absDir)
+}