From 3f5031f5456bead4f67089f50f2386a192780b51 Mon Sep 17 00:00:00 2001 From: Yexiang Zhang Date: Fri, 15 Sep 2023 11:52:39 +0800 Subject: [PATCH 1/3] security fixes (#1592) --- pkg/apiserver/conprof/service.go | 1 + pkg/apiserver/resource_manager/service.go | 8 ++++++++ pkg/apiserver/statement/queries.go | 4 ++-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/apiserver/conprof/service.go b/pkg/apiserver/conprof/service.go index 5c72e37fc3..0d564cc7f8 100644 --- a/pkg/apiserver/conprof/service.go +++ b/pkg/apiserver/conprof/service.go @@ -228,6 +228,7 @@ func (s *Service) parseJWTToken(c *gin.Context) { queryStr, err := utils.ParseJWTString("conprof", token) if err != nil { rest.Error(c, rest.ErrBadRequest.WrapWithNoMessage(err)) + c.Abort() return } c.Request.URL.RawQuery = queryStr diff --git a/pkg/apiserver/resource_manager/service.go b/pkg/apiserver/resource_manager/service.go index 8d0e78a4a4..45479dc1fc 100644 --- a/pkg/apiserver/resource_manager/service.go +++ b/pkg/apiserver/resource_manager/service.go @@ -3,8 +3,10 @@ package resourcemanager import ( + "errors" "fmt" "net/http" + "regexp" "time" "github.com/gin-gonic/gin" @@ -17,6 +19,8 @@ import ( "github.com/pingcap/tidb-dashboard/util/rest" ) +var workloadInjectChecker = regexp.MustCompile(`^[a-zA-Z0-9_]+$`) + type ServiceParams struct { fx.In TiDBClient *tidb.Client @@ -109,6 +113,10 @@ func (s *Service) GetCalibrateByHardware(c *gin.Context) { rest.Error(c, rest.ErrBadRequest.New("workload cannot be empty")) return } + if !workloadInjectChecker.MatchString(w) { + rest.Error(c, errors.New("invalid workload")) + return + } db := utils.GetTiDBConnection(c) resp := &CalibrateResponse{} diff --git a/pkg/apiserver/statement/queries.go b/pkg/apiserver/statement/queries.go index efdbf40b83..80fb14e93a 100644 --- a/pkg/apiserver/statement/queries.go +++ b/pkg/apiserver/statement/queries.go @@ -16,7 +16,7 @@ const ( statementsTable = "INFORMATION_SCHEMA.CLUSTER_STATEMENTS_SUMMARY_HISTORY" ) -var injectChecker = regexp.MustCompile(`\s`) +var digestInjectChecker = regexp.MustCompile(`^[a-zA-Z0-9]+$`) func queryStmtTypes(db *gorm.DB) (result []string, err error) { // why should put DISTINCT inside the `Pluck()` method, see here: @@ -210,7 +210,7 @@ func (s *Service) createPlanBinding(db *gorm.DB, planDigest string) (err error) // Caution! SQL injection vulnerability! // We have to interpolate sql string here, since plan binding stmt does not support session level prepare. // go-sql-driver can enable interpolation globally. Refer to https://github.com/go-sql-driver/mysql#interpolateparams. - if injectChecker.MatchString(planDigest) { + if !digestInjectChecker.MatchString(planDigest) { return errors.New("invalid planDigest") } From 58f85cd0ea3e7c6119ec66a43618ed34a5c804b2 Mon Sep 17 00:00:00 2001 From: Yexiang Zhang Date: Mon, 18 Sep 2023 15:34:41 +0800 Subject: [PATCH 2/3] hide full stacktrace info in error message (#1593) --- util/rest/context_helpers.go | 4 ++-- util/rest/error_resp.go | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/util/rest/context_helpers.go b/util/rest/context_helpers.go index f0a52ca756..146a0dce80 100644 --- a/util/rest/context_helpers.go +++ b/util/rest/context_helpers.go @@ -6,7 +6,6 @@ import ( "net/http" "github.com/gin-gonic/gin" - "github.com/joomcode/errorx" "github.com/pingcap/tidb-dashboard/util/jsonserde/ginadapter" ) @@ -16,7 +15,8 @@ import ( // Otherwise there will be no error message written to the client. // See `ErrorHandlerFn` for more details. func Error(c *gin.Context, err error) { - _ = c.Error(errorx.EnsureStackTrace(err)) + // For security reasons, we need to hide detailed stacktrace info. + _ = c.Error(err) // before: c.Error(errorx.EnsureStackTrace(err)) } // JSON writes a JSON string to the client with the given status code. diff --git a/util/rest/error_resp.go b/util/rest/error_resp.go index 63513ef380..e5d62c5920 100644 --- a/util/rest/error_resp.go +++ b/util/rest/error_resp.go @@ -95,9 +95,10 @@ func buildDetailMessage(err error) string { func NewErrorResponse(err error) ErrorResponse { return ErrorResponse{ - Error: true, - Message: buildSimpleMessage(err), - Code: removeErrorPrefix(buildCode(err)), - FullText: buildDetailMessage(err), + Error: true, + Message: buildSimpleMessage(err), + Code: removeErrorPrefix(buildCode(err)), + // For security reasons, we need to hide detailed stacktrace info. + // FullText: buildDetailMessage(err), } } From e78b42b325515237d2e38647a8fd489ef447d653 Mon Sep 17 00:00:00 2001 From: mornyx Date: Thu, 21 Sep 2023 00:24:10 +0800 Subject: [PATCH 3/3] update release-version to 2023.09.21.1 Signed-off-by: mornyx --- release-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/release-version b/release-version index 72f49c8c75..5b9134d514 100644 --- a/release-version +++ b/release-version @@ -1,3 +1,3 @@ # This file specifies the TiDB Dashboard internal version, which will be printed in `--version` # and UI. In release branch, changing this file will result in publishing a new version and tag. -2023.09.11.1 +2023.09.21.1