From bef28522c0f873b46f15bd3a82a96abc6d9d2386 Mon Sep 17 00:00:00 2001 From: Jaeyeon Park Date: Sat, 5 Oct 2024 17:13:50 +0900 Subject: [PATCH 1/4] fix: graphql server path injection Signed-off-by: Jaeyeon Park --- .../server/pkg/chaoshub/handler/handler.go | 23 +++++++++++++------ .../graphql/server/pkg/chaoshub/ops/gitops.go | 5 ++-- .../graphql/server/pkg/chaoshub/service.go | 15 ++++++------ .../graphql/server/pkg/gitops/gitops.go | 5 ++-- .../graphql/server/pkg/gitops/service.go | 9 ++++---- 5 files changed, 35 insertions(+), 22 deletions(-) diff --git a/chaoscenter/graphql/server/pkg/chaoshub/handler/handler.go b/chaoscenter/graphql/server/pkg/chaoshub/handler/handler.go index a62fcffd18b..39862253ff5 100644 --- a/chaoscenter/graphql/server/pkg/chaoshub/handler/handler.go +++ b/chaoscenter/graphql/server/pkg/chaoshub/handler/handler.go @@ -155,13 +155,13 @@ func IsFileExisting(path string) (bool, error) { // DownloadRemoteHub is used to download a remote hub from the url provided by the user func DownloadRemoteHub(hubDetails model.CreateRemoteChaosHub, projectID string) error { - dirPath := DefaultPath + projectID + dirPath := DefaultPath + sanitize.PathName(projectID) err := os.MkdirAll(dirPath, 0755) if err != nil { return err } //create the destination directory where the hub will be downloaded - hubPath := dirPath + "/" + hubDetails.Name + ".zip" + hubPath := dirPath + "/" + sanitize.PathName(hubDetails.Name) + ".zip" destDir, err := os.Create(hubPath) if err != nil { log.Error(err) @@ -278,7 +278,7 @@ func CopyZipItems(file *zip.File, extractPath string, chartsPath string) error { // SyncRemoteRepo is used to sync the remote ChaosHub func SyncRemoteRepo(hubData model.CloningInput, projectID string) error { - hubPath := DefaultPath + projectID + "/" + hubData.Name + hubPath := DefaultPath + sanitize.PathName(projectID) + "/" + sanitize.PathName(hubData.Name) err := os.RemoveAll(hubPath) if err != nil { return err @@ -320,8 +320,13 @@ func ChaosHubIconHandler() gin.HandlerFunc { responseStatusCode int ) + projectID := sanitize.PathName(c.Param("projectId")) + hubName := sanitize.PathName(c.Param("hubName")) + chartName := sanitize.PathName(c.Param("chartName")) + iconName := sanitize.PathName(c.Param("iconName")) + if strings.ToLower(c.Param("chartName")) == "predefined" { - img, err = os.Open(utils.Config.CustomChaosHubPath + c.Param("projectId") + "/" + c.Param("hubName") + "/experiments/icons/" + c.Param("iconName")) + img, err = os.Open(utils.Config.CustomChaosHubPath + projectID + "/" + hubName + "/experiments/icons/" + iconName) responseStatusCode = http.StatusOK if err != nil { responseStatusCode = http.StatusInternalServerError @@ -329,7 +334,7 @@ func ChaosHubIconHandler() gin.HandlerFunc { fmt.Fprint(c.Writer, "icon cannot be fetched, err : "+err.Error()) } } else { - img, err = os.Open(utils.Config.CustomChaosHubPath + c.Param("projectId") + "/" + c.Param("hubName") + "/faults/" + c.Param("chartName") + "/icons/" + c.Param("iconName")) + img, err = os.Open(utils.Config.CustomChaosHubPath + projectID + "/" + hubName + "/faults/" + chartName + "/icons/" + iconName) responseStatusCode = http.StatusOK if err != nil { responseStatusCode = http.StatusInternalServerError @@ -354,8 +359,12 @@ func DefaultChaosHubIconHandler() gin.HandlerFunc { responseStatusCode int ) + hubName := sanitize.PathName(c.Param("hubName")) + chartName := sanitize.PathName(c.Param("chartName")) + iconName := sanitize.PathName(c.Param("iconName")) + if strings.ToLower(c.Param("chartName")) == "predefined" { - img, err = os.Open(utils.Config.DefaultChaosHubPath + c.Param("hubName") + "/experiments/icons/" + c.Param("iconName")) + img, err = os.Open(utils.Config.DefaultChaosHubPath + hubName + "/experiments/icons/" + iconName) responseStatusCode = http.StatusOK if err != nil { responseStatusCode = http.StatusInternalServerError @@ -363,7 +372,7 @@ func DefaultChaosHubIconHandler() gin.HandlerFunc { fmt.Fprint(c.Writer, "icon cannot be fetched, err : "+err.Error()) } } else { - img, err = os.Open(utils.Config.DefaultChaosHubPath + c.Param("hubName") + "/faults/" + c.Param("chartName") + "/icons/" + c.Param("iconName")) + img, err = os.Open(utils.Config.DefaultChaosHubPath + hubName + "/faults/" + chartName + "/icons/" + iconName) responseStatusCode = http.StatusOK if err != nil { responseStatusCode = http.StatusInternalServerError diff --git a/chaoscenter/graphql/server/pkg/chaoshub/ops/gitops.go b/chaoscenter/graphql/server/pkg/chaoshub/ops/gitops.go index 9480b144e6d..2f7313eaa6d 100644 --- a/chaoscenter/graphql/server/pkg/chaoshub/ops/gitops.go +++ b/chaoscenter/graphql/server/pkg/chaoshub/ops/gitops.go @@ -7,6 +7,7 @@ import ( "github.com/litmuschaos/litmus/chaoscenter/graphql/server/graph/model" "github.com/litmuschaos/litmus/chaoscenter/graphql/server/utils" + "github.com/mrz1836/go-sanitize" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" @@ -45,9 +46,9 @@ func GetClonePath(c ChaosHubConfig) string { var repoPath string if c.IsDefault { - repoPath = "/tmp/default/" + c.HubName + repoPath = "/tmp/default/" + sanitize.PathName(c.HubName) } else { - repoPath = DefaultPath + c.ProjectID + "/" + c.HubName + repoPath = DefaultPath + sanitize.PathName(c.ProjectID) + "/" + sanitize.PathName(c.HubName) } return repoPath } diff --git a/chaoscenter/graphql/server/pkg/chaoshub/service.go b/chaoscenter/graphql/server/pkg/chaoshub/service.go index 53f46cc975f..baa66fd7e56 100644 --- a/chaoscenter/graphql/server/pkg/chaoshub/service.go +++ b/chaoscenter/graphql/server/pkg/chaoshub/service.go @@ -17,6 +17,7 @@ import ( "github.com/litmuschaos/litmus/chaoscenter/graphql/server/pkg/database/mongodb" dbSchemaChaosHub "github.com/litmuschaos/litmus/chaoscenter/graphql/server/pkg/database/mongodb/chaos_hub" "github.com/litmuschaos/litmus/chaoscenter/graphql/server/utils" + "github.com/mrz1836/go-sanitize" log "github.com/sirupsen/logrus" "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/mongo" @@ -329,7 +330,7 @@ func (c *chaosHubService) UpdateChaosHub(ctx context.Context, chaosHub model.Upd if err != nil { return nil, err } - clonePath := DefaultPath + prevChaosHub.ProjectID + "/" + prevChaosHub.Name + clonePath := DefaultPath + sanitize.PathName(prevChaosHub.ProjectID) + "/" + sanitize.PathName(prevChaosHub.Name) if prevChaosHub.HubType == string(model.HubTypeRemote) { if prevChaosHub.Name != chaosHub.Name || prevChaosHub.RepoURL != chaosHub.RepoURL || prevChaosHub.RemoteHub != chaosHub.RemoteHub { remoteHub := model.CreateRemoteChaosHub{ @@ -439,7 +440,7 @@ func (c *chaosHubService) DeleteChaosHub(ctx context.Context, hubID string, proj log.Error(err) return false, err } - clonePath := DefaultPath + projectID + "/" + chaosHub.Name + clonePath := DefaultPath + sanitize.PathName(projectID) + "/" + sanitize.PathName(chaosHub.Name) err = os.RemoveAll(clonePath) if err != nil { log.Error(err) @@ -735,9 +736,9 @@ func (c *chaosHubService) ListPredefinedExperiments(ctx context.Context, hubID s var hubPath string if hub.IsDefault { - hubPath = "/tmp/default/" + hub.Name + "/experiments/" + hubPath = "/tmp/default/" + sanitize.PathName(hub.Name) + "/experiments/" } else { - hubPath = DefaultPath + projectID + "/" + hub.Name + "/experiments/" + hubPath = DefaultPath + sanitize.PathName(projectID) + "/" + sanitize.PathName(hub.Name) + "/experiments/" } var predefinedWorkflows []*model.PredefinedExperimentList files, err := os.ReadDir(hubPath) @@ -794,9 +795,9 @@ func (c *chaosHubService) GetPredefinedExperiment(ctx context.Context, hubID str } var hubPath string if hub.IsDefault { - hubPath = "/tmp/default/" + hub.Name + "/experiments/" + hubPath = "/tmp/default/" + sanitize.PathName(hub.Name) + "/experiments/" } else { - hubPath = DefaultPath + projectID + "/" + hub.Name + "/experiments/" + hubPath = DefaultPath + sanitize.PathName(projectID) + "/" + sanitize.PathName(hub.Name) + "/experiments/" } var predefinedWorkflows []*model.PredefinedExperimentList @@ -814,7 +815,7 @@ func (c *chaosHubService) getPredefinedExperimentDetails(experimentsPath string, var ( csvManifest = "" workflowManifest = "" - path = experimentsPath + experiment + "/" + experiment + ".chartserviceversion.yaml" + path = experimentsPath + sanitize.PathName(experiment) + "/" + sanitize.PathName(experiment) + ".chartserviceversion.yaml" isExist = true preDefinedWorkflow = &model.PredefinedExperimentList{} ) diff --git a/chaoscenter/graphql/server/pkg/gitops/gitops.go b/chaoscenter/graphql/server/pkg/gitops/gitops.go index 8065ace8c30..340e1dda9a2 100644 --- a/chaoscenter/graphql/server/pkg/gitops/gitops.go +++ b/chaoscenter/graphql/server/pkg/gitops/gitops.go @@ -24,6 +24,7 @@ import ( "github.com/litmuschaos/litmus/chaoscenter/graphql/server/graph/model" "github.com/litmuschaos/litmus/chaoscenter/graphql/server/pkg/authorization" "github.com/litmuschaos/litmus/chaoscenter/graphql/server/pkg/database/mongodb/gitops" + "github.com/mrz1836/go-sanitize" log "github.com/sirupsen/logrus" ssh2 "golang.org/x/crypto/ssh" ) @@ -84,7 +85,7 @@ func GetGitOpsConfig(repoData gitops.GitConfigDB) GitConfig { RepositoryURL: repoData.RepositoryURL, RemoteName: "origin", Branch: repoData.Branch, - LocalPath: DefaultPath + repoData.ProjectID, + LocalPath: DefaultPath + sanitize.PathName(repoData.ProjectID), LatestCommit: repoData.LatestCommit, UserName: repoData.UserName, Password: repoData.Password, @@ -98,7 +99,7 @@ func GetGitOpsConfig(repoData gitops.GitConfigDB) GitConfig { // setupGitRepo helps clones and sets up the repo for GitOps func (c GitConfig) setupGitRepo(user GitUser) error { - projectPath := c.LocalPath + "/" + ProjectDataPath + "/" + c.ProjectID + projectPath := c.LocalPath + "/" + ProjectDataPath + "/" + sanitize.PathName(c.ProjectID) // clone repo _, err := c.GitClone() diff --git a/chaoscenter/graphql/server/pkg/gitops/service.go b/chaoscenter/graphql/server/pkg/gitops/service.go index 2c19987af22..3250eb7b5e3 100644 --- a/chaoscenter/graphql/server/pkg/gitops/service.go +++ b/chaoscenter/graphql/server/pkg/gitops/service.go @@ -21,6 +21,7 @@ import ( "github.com/litmuschaos/litmus/chaoscenter/graphql/server/pkg/database/mongodb/chaos_infrastructure" "github.com/litmuschaos/litmus/chaoscenter/graphql/server/pkg/database/mongodb/gitops" "github.com/litmuschaos/litmus/chaoscenter/graphql/server/pkg/grpc" + "github.com/mrz1836/go-sanitize" log "github.com/sirupsen/logrus" "github.com/tidwall/gjson" "github.com/tidwall/sjson" @@ -154,7 +155,7 @@ func (g *gitOpsService) DisableGitOpsHandler(ctx context.Context, projectID stri return false, errors.New("Failed to delete git config from DB : " + err.Error()) } - err = os.RemoveAll(DefaultPath + projectID) + err = os.RemoveAll(DefaultPath + sanitize.PathName(projectID)) if err != nil { return false, errors.New("Failed to delete git repo from disk : " + err.Error()) } @@ -183,7 +184,7 @@ func (g *gitOpsService) UpdateGitOpsDetailsHandler(ctx context.Context, projectI gitConfig := GetGitOpsConfig(gitDB) originalPath := gitConfig.LocalPath - gitConfig.LocalPath = tempPath + gitConfig.ProjectID + gitConfig.LocalPath = tempPath + sanitize.PathName(gitConfig.ProjectID) commit, err := SetupGitOps(GitUserFromContext(ctx), gitConfig) if err != nil { return false, errors.New("Failed to setup GitOps : " + err.Error()) @@ -319,7 +320,7 @@ func (g *gitOpsService) DeleteExperimentFromGit(ctx context.Context, projectID s return errors.New("Sync Error | " + err.Error()) } - experimentPath := ProjectDataPath + "/" + gitConfig.ProjectID + "/" + experiment.ExperimentName + ".yaml" + experimentPath := ProjectDataPath + "/" + sanitize.PathName(gitConfig.ProjectID) + "/" + sanitize.PathName(experiment.ExperimentName) + ".yaml" exists, err := PathExists(gitConfig.LocalPath + "/" + experimentPath) if err != nil { return errors.New("Cannot delete experiment from git : " + err.Error()) @@ -457,7 +458,7 @@ func (g *gitOpsService) SyncDBToGit(ctx context.Context, config GitConfig) error continue } // check if file was deleted or not - exists, err := PathExists(config.LocalPath + "/" + file) + exists, err := PathExists(config.LocalPath + "/" + sanitize.PathName(file)) if err != nil { return errors.New("Error checking file in local repo : " + file + " | " + err.Error()) } From e4ae78e3ed44fff9f45f12ac8c3695451126dab6 Mon Sep 17 00:00:00 2001 From: Jaeyeon Park Date: Sun, 6 Oct 2024 15:07:58 +0900 Subject: [PATCH 2/4] test: graphql server path injection Signed-off-by: Jaeyeon Park --- .../pkg/chaoshub/handler/handler_test.go | 16 ++++++ .../server/pkg/chaoshub/ops/gitops_test.go | 52 ++++++++++++++++--- 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go b/chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go index 1a30b1e012a..af686001bdf 100644 --- a/chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go +++ b/chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go @@ -157,6 +157,14 @@ func TestDownloadRemoteHub(t *testing.T) { }, isError: true, }, + { + name: "path injection", + projectID: uuid.New().String(), + chaosHub: model.CreateRemoteChaosHub{ + Name: uuid.New().String() + "../path/injection", + RepoURL: "https://github.com/litmuschaos/chaos-charts/archive/refs/heads/master.zip", + }, + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { @@ -209,6 +217,14 @@ func TestSyncRemoteRepo(t *testing.T) { }, isError: true, }, + { + name: "path injection", + projectID: uuid.New().String(), + chaosHub: model.CloningInput{ + Name: uuid.New().String() + "../path/injection", + RepoURL: "https://github.com/litmuschaos/chaos-charts/archive/refs/heads/master.zip", + }, + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { diff --git a/chaoscenter/graphql/server/pkg/chaoshub/ops/gitops_test.go b/chaoscenter/graphql/server/pkg/chaoshub/ops/gitops_test.go index 531c5510ea7..74999d756fd 100644 --- a/chaoscenter/graphql/server/pkg/chaoshub/ops/gitops_test.go +++ b/chaoscenter/graphql/server/pkg/chaoshub/ops/gitops_test.go @@ -39,16 +39,52 @@ func clearCloneRepository(projectID string) { // TestGetClonePath is used to test the GetClonePath function func TestGetClonePath(t *testing.T) { - // given projectID := uuid.New().String() - chaosHubConfig := chaosHubOps.ChaosHubConfig{ - ProjectID: projectID, - HubName: "test", + + testCases := []struct { + name string + projectID string + hubName string + isDefault bool + expected string + }{ + { + name: "default hub", + projectID: projectID, + hubName: "defaultHub", + isDefault: true, + expected: "/tmp/default/defaultHub", + }, + { + name: "non-default hub", + projectID: projectID, + hubName: "testHub", + isDefault: false, + expected: "/tmp/" + projectID + "/testHub", + }, + { + name: "path injection", + projectID: projectID, + hubName: "testHub/../Path/Injection", + isDefault: false, + expected: "/tmp/" + projectID + "/testHubPathInjection", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // given + chaosHubConfig := chaosHubOps.ChaosHubConfig{ + ProjectID: tc.projectID, + HubName: tc.hubName, + IsDefault: tc.isDefault, + } + // when + path := chaosHubOps.GetClonePath(chaosHubConfig) + // then + assert.Equal(t, tc.expected, path) + }) } - // when - path := chaosHubOps.GetClonePath(chaosHubConfig) - // then - assert.Equal(t, "/tmp/"+projectID+"/test", path) } // TestGitConfigConstruct is used to test the GitConfigConstruct function From 4de4c45983a30c2a3cae39c6891aaba481e7e360 Mon Sep 17 00:00:00 2001 From: Jaeyeon Park Date: Wed, 9 Oct 2024 16:54:13 +0900 Subject: [PATCH 3/4] fix: graphql server path injection with error Signed-off-by: Jaeyeon Park --- .../server/pkg/chaoshub/handler/handler.go | 85 ++++++++++++++++--- .../graphql/server/pkg/chaoshub/service.go | 47 ++++++++-- .../graphql/server/pkg/gitops/gitops.go | 6 +- .../graphql/server/pkg/gitops/service.go | 31 ++++++- 4 files changed, 144 insertions(+), 25 deletions(-) diff --git a/chaoscenter/graphql/server/pkg/chaoshub/handler/handler.go b/chaoscenter/graphql/server/pkg/chaoshub/handler/handler.go index 39862253ff5..b1ef2997e78 100644 --- a/chaoscenter/graphql/server/pkg/chaoshub/handler/handler.go +++ b/chaoscenter/graphql/server/pkg/chaoshub/handler/handler.go @@ -155,13 +155,22 @@ func IsFileExisting(path string) (bool, error) { // DownloadRemoteHub is used to download a remote hub from the url provided by the user func DownloadRemoteHub(hubDetails model.CreateRemoteChaosHub, projectID string) error { - dirPath := DefaultPath + sanitize.PathName(projectID) + if projectID != sanitize.PathName(projectID) { + return fmt.Errorf("err: invalid projectID '%s', potential path injection detected", projectID) + } + + dirPath := DefaultPath + projectID err := os.MkdirAll(dirPath, 0755) if err != nil { return err } + + if hubDetails.Name != sanitize.PathName(hubDetails.Name) { + return fmt.Errorf("err: invalid hub name '%s', potential path injection detected", hubDetails.Name) + } + //create the destination directory where the hub will be downloaded - hubPath := dirPath + "/" + sanitize.PathName(hubDetails.Name) + ".zip" + hubPath := dirPath + "/" + hubDetails.Name + ".zip" destDir, err := os.Create(hubPath) if err != nil { log.Error(err) @@ -278,7 +287,15 @@ func CopyZipItems(file *zip.File, extractPath string, chartsPath string) error { // SyncRemoteRepo is used to sync the remote ChaosHub func SyncRemoteRepo(hubData model.CloningInput, projectID string) error { - hubPath := DefaultPath + sanitize.PathName(projectID) + "/" + sanitize.PathName(hubData.Name) + if projectID != sanitize.PathName(projectID) { + return fmt.Errorf("err: invalid projectID '%s', potential path injection detected", projectID) + } + + if hubData.Name != sanitize.PathName(hubData.Name) { + return fmt.Errorf("err: invalid hub name '%s', potential path injection detected", hubData.Name) + } + + hubPath := DefaultPath + projectID + "/" + hubData.Name err := os.RemoveAll(hubPath) if err != nil { return err @@ -320,13 +337,36 @@ func ChaosHubIconHandler() gin.HandlerFunc { responseStatusCode int ) - projectID := sanitize.PathName(c.Param("projectId")) - hubName := sanitize.PathName(c.Param("hubName")) - chartName := sanitize.PathName(c.Param("chartName")) - iconName := sanitize.PathName(c.Param("iconName")) + if c.Param("projectId") != sanitize.PathName(c.Param("projectId")) { + responseStatusCode = http.StatusBadRequest + log.Errorf("err: invalid projectID '%s', potential path injection detected", c.Param("projectId")) + fmt.Fprintf(c.Writer, "err: invalid projectID '%s', potential path injection detected", c.Param("projectId")) + return + } + + if c.Param("hubName") != sanitize.PathName(c.Param("hubName")) { + responseStatusCode = http.StatusBadRequest + log.Errorf("err: invalid hub name '%s', potential path injection detected", c.Param("hubName")) + fmt.Fprintf(c.Writer, "err: invalid hub name '%s', potential path injection detected", c.Param("hubName")) + return + } + + if c.Param("chartName") != sanitize.PathName(c.Param("chartName")) { + responseStatusCode = http.StatusBadRequest + log.Errorf("err: invalid chart name '%s', potential path injection detected", c.Param("chartName")) + fmt.Fprintf(c.Writer, "err: invalid chart name '%s', potential path injection detected", c.Param("chartName")) + return + } + + if c.Param("iconName") != sanitize.PathName(c.Param("iconName")) { + responseStatusCode = http.StatusBadRequest + log.Errorf("err: invalid icon name '%s', potential path injection detected", c.Param("iconName")) + fmt.Fprintf(c.Writer, "err: invalid icon name '%s', potential path injection detected", c.Param("iconName")) + return + } if strings.ToLower(c.Param("chartName")) == "predefined" { - img, err = os.Open(utils.Config.CustomChaosHubPath + projectID + "/" + hubName + "/experiments/icons/" + iconName) + img, err = os.Open(utils.Config.CustomChaosHubPath + c.Param("projectId") + "/" + c.Param("hubName") + "/experiments/icons/" + c.Param("iconName")) responseStatusCode = http.StatusOK if err != nil { responseStatusCode = http.StatusInternalServerError @@ -334,7 +374,7 @@ func ChaosHubIconHandler() gin.HandlerFunc { fmt.Fprint(c.Writer, "icon cannot be fetched, err : "+err.Error()) } } else { - img, err = os.Open(utils.Config.CustomChaosHubPath + projectID + "/" + hubName + "/faults/" + chartName + "/icons/" + iconName) + img, err = os.Open(utils.Config.CustomChaosHubPath + c.Param("projectId") + "/" + c.Param("hubName") + "/faults/" + c.Param("chartName") + "/icons/" + c.Param("iconName")) responseStatusCode = http.StatusOK if err != nil { responseStatusCode = http.StatusInternalServerError @@ -359,12 +399,29 @@ func DefaultChaosHubIconHandler() gin.HandlerFunc { responseStatusCode int ) - hubName := sanitize.PathName(c.Param("hubName")) - chartName := sanitize.PathName(c.Param("chartName")) - iconName := sanitize.PathName(c.Param("iconName")) + if c.Param("hubName") != sanitize.PathName(c.Param("hubName")) { + responseStatusCode = http.StatusBadRequest + log.Errorf("err: invalid hub name '%s', potential path injection detected", c.Param("hubName")) + fmt.Fprintf(c.Writer, "err: invalid hub name '%s', potential path injection detected", c.Param("hubName")) + return + } + + if c.Param("chartName") != sanitize.PathName(c.Param("chartName")) { + responseStatusCode = http.StatusBadRequest + log.Errorf("err: invalid chart name '%s', potential path injection detected", c.Param("chartName")) + fmt.Fprintf(c.Writer, "err: invalid chart name '%s', potential path injection detected", c.Param("chartName")) + return + } + + if c.Param("iconName") != sanitize.PathName(c.Param("iconName")) { + responseStatusCode = http.StatusBadRequest + log.Errorf("err: invalid icon name '%s', potential path injection detected", c.Param("iconName")) + fmt.Fprintf(c.Writer, "err: invalid icon name '%s', potential path injection detected", c.Param("iconName")) + return + } if strings.ToLower(c.Param("chartName")) == "predefined" { - img, err = os.Open(utils.Config.DefaultChaosHubPath + hubName + "/experiments/icons/" + iconName) + img, err = os.Open(utils.Config.DefaultChaosHubPath + c.Param("hubName") + "/experiments/icons/" + c.Param("iconName")) responseStatusCode = http.StatusOK if err != nil { responseStatusCode = http.StatusInternalServerError @@ -372,7 +429,7 @@ func DefaultChaosHubIconHandler() gin.HandlerFunc { fmt.Fprint(c.Writer, "icon cannot be fetched, err : "+err.Error()) } } else { - img, err = os.Open(utils.Config.DefaultChaosHubPath + hubName + "/faults/" + chartName + "/icons/" + iconName) + img, err = os.Open(utils.Config.DefaultChaosHubPath + c.Param("hubName") + "/faults/" + c.Param("chartName") + "/icons/" + c.Param("iconName")) responseStatusCode = http.StatusOK if err != nil { responseStatusCode = http.StatusInternalServerError diff --git a/chaoscenter/graphql/server/pkg/chaoshub/service.go b/chaoscenter/graphql/server/pkg/chaoshub/service.go index baa66fd7e56..19b89621f8b 100644 --- a/chaoscenter/graphql/server/pkg/chaoshub/service.go +++ b/chaoscenter/graphql/server/pkg/chaoshub/service.go @@ -330,7 +330,16 @@ func (c *chaosHubService) UpdateChaosHub(ctx context.Context, chaosHub model.Upd if err != nil { return nil, err } - clonePath := DefaultPath + sanitize.PathName(prevChaosHub.ProjectID) + "/" + sanitize.PathName(prevChaosHub.Name) + + if prevChaosHub.ProjectID != sanitize.PathName(prevChaosHub.ProjectID) { + return nil, fmt.Errorf("err: invalid projectID '%s', potential path injection detected", projectID) + } + + if prevChaosHub.Name != sanitize.PathName(prevChaosHub.Name) { + return nil, fmt.Errorf("err: invalid hub name '%s', potential path injection detected", chaosHub.Name) + } + + clonePath := DefaultPath + prevChaosHub.ProjectID + "/" + prevChaosHub.Name if prevChaosHub.HubType == string(model.HubTypeRemote) { if prevChaosHub.Name != chaosHub.Name || prevChaosHub.RepoURL != chaosHub.RepoURL || prevChaosHub.RemoteHub != chaosHub.RemoteHub { remoteHub := model.CreateRemoteChaosHub{ @@ -440,7 +449,16 @@ func (c *chaosHubService) DeleteChaosHub(ctx context.Context, hubID string, proj log.Error(err) return false, err } - clonePath := DefaultPath + sanitize.PathName(projectID) + "/" + sanitize.PathName(chaosHub.Name) + + if projectID != sanitize.PathName(projectID) { + return false, fmt.Errorf("err: invalid projectID '%s', potential path injection detected", projectID) + } + + if chaosHub.Name != sanitize.PathName(chaosHub.Name) { + return false, fmt.Errorf("err: invalid hub name '%s', potential path injection detected", chaosHub.Name) + } + + clonePath := DefaultPath + projectID + "/" + chaosHub.Name err = os.RemoveAll(clonePath) if err != nil { log.Error(err) @@ -734,11 +752,19 @@ func (c *chaosHubService) ListPredefinedExperiments(ctx context.Context, hubID s return nil, err } + if projectID != sanitize.PathName(projectID) { + return nil, fmt.Errorf("err: invalid projectID '%s', potential path injection detected", projectID) + } + + if hub.Name != sanitize.PathName(hub.Name) { + return nil, fmt.Errorf("err: invalid hub name '%s', potential path injection detected", hub.Name) + } + var hubPath string if hub.IsDefault { - hubPath = "/tmp/default/" + sanitize.PathName(hub.Name) + "/experiments/" + hubPath = "/tmp/default/" + hub.Name + "/experiments/" } else { - hubPath = DefaultPath + sanitize.PathName(projectID) + "/" + sanitize.PathName(hub.Name) + "/experiments/" + hubPath = DefaultPath + projectID + "/" + hub.Name + "/experiments/" } var predefinedWorkflows []*model.PredefinedExperimentList files, err := os.ReadDir(hubPath) @@ -793,11 +819,20 @@ func (c *chaosHubService) GetPredefinedExperiment(ctx context.Context, hubID str if err != nil { return nil, err } + + if projectID != sanitize.PathName(projectID) { + return nil, fmt.Errorf("err: invalid projectID '%s', potential path injection detected", projectID) + } + + if hub.Name != sanitize.PathName(hub.Name) { + return nil, fmt.Errorf("err: invalid hub name '%s', potential path injection detected", hub.Name) + } + var hubPath string if hub.IsDefault { - hubPath = "/tmp/default/" + sanitize.PathName(hub.Name) + "/experiments/" + hubPath = "/tmp/default/" + hub.Name + "/experiments/" } else { - hubPath = DefaultPath + sanitize.PathName(projectID) + "/" + sanitize.PathName(hub.Name) + "/experiments/" + hubPath = DefaultPath + projectID + "/" + hub.Name + "/experiments/" } var predefinedWorkflows []*model.PredefinedExperimentList diff --git a/chaoscenter/graphql/server/pkg/gitops/gitops.go b/chaoscenter/graphql/server/pkg/gitops/gitops.go index 340e1dda9a2..fbd7338f648 100644 --- a/chaoscenter/graphql/server/pkg/gitops/gitops.go +++ b/chaoscenter/graphql/server/pkg/gitops/gitops.go @@ -99,7 +99,11 @@ func GetGitOpsConfig(repoData gitops.GitConfigDB) GitConfig { // setupGitRepo helps clones and sets up the repo for GitOps func (c GitConfig) setupGitRepo(user GitUser) error { - projectPath := c.LocalPath + "/" + ProjectDataPath + "/" + sanitize.PathName(c.ProjectID) + if c.ProjectID != sanitize.PathName(c.ProjectID) { + return fmt.Errorf("err: invalid projectID '%s', potential path injection detected", c.ProjectID) + } + + projectPath := c.LocalPath + "/" + ProjectDataPath + "/" + c.ProjectID // clone repo _, err := c.GitClone() diff --git a/chaoscenter/graphql/server/pkg/gitops/service.go b/chaoscenter/graphql/server/pkg/gitops/service.go index 3250eb7b5e3..f5aecec3649 100644 --- a/chaoscenter/graphql/server/pkg/gitops/service.go +++ b/chaoscenter/graphql/server/pkg/gitops/service.go @@ -146,6 +146,10 @@ func (g *gitOpsService) EnableGitOpsHandler(ctx context.Context, projectID strin // DisableGitOpsHandler disables GitOps for a specific project func (g *gitOpsService) DisableGitOpsHandler(ctx context.Context, projectID string) (bool, error) { + if projectID != sanitize.PathName(projectID) { + return false, fmt.Errorf("err: invalid projectID '%s', potential path injection detected", projectID) + } + gitLock.Lock(projectID, nil) defer gitLock.Unlock(projectID, nil) @@ -155,7 +159,7 @@ func (g *gitOpsService) DisableGitOpsHandler(ctx context.Context, projectID stri return false, errors.New("Failed to delete git config from DB : " + err.Error()) } - err = os.RemoveAll(DefaultPath + sanitize.PathName(projectID)) + err = os.RemoveAll(DefaultPath + projectID) if err != nil { return false, errors.New("Failed to delete git repo from disk : " + err.Error()) } @@ -184,7 +188,12 @@ func (g *gitOpsService) UpdateGitOpsDetailsHandler(ctx context.Context, projectI gitConfig := GetGitOpsConfig(gitDB) originalPath := gitConfig.LocalPath - gitConfig.LocalPath = tempPath + sanitize.PathName(gitConfig.ProjectID) + + if gitConfig.ProjectID != sanitize.PathName(gitConfig.ProjectID) { + return false, fmt.Errorf("err: invalid projectID '%s', potential path injection detected", gitConfig.ProjectID) + } + + gitConfig.LocalPath = tempPath + gitConfig.ProjectID commit, err := SetupGitOps(GitUserFromContext(ctx), gitConfig) if err != nil { return false, errors.New("Failed to setup GitOps : " + err.Error()) @@ -320,7 +329,15 @@ func (g *gitOpsService) DeleteExperimentFromGit(ctx context.Context, projectID s return errors.New("Sync Error | " + err.Error()) } - experimentPath := ProjectDataPath + "/" + sanitize.PathName(gitConfig.ProjectID) + "/" + sanitize.PathName(experiment.ExperimentName) + ".yaml" + if gitConfig.ProjectID != sanitize.PathName(gitConfig.ProjectID) { + return fmt.Errorf("err: invalid projectID '%s', potential path injection detected", gitConfig.ProjectID) + } + + if experiment.ExperimentName != sanitize.PathName(experiment.ExperimentName) { + return fmt.Errorf("err: invalid experiment name '%s', potential path injection detected", experiment.ExperimentName) + } + + experimentPath := ProjectDataPath + "/" + gitConfig.ProjectID + "/" + experiment.ExperimentName + ".yaml" exists, err := PathExists(gitConfig.LocalPath + "/" + experimentPath) if err != nil { return errors.New("Cannot delete experiment from git : " + err.Error()) @@ -457,8 +474,14 @@ func (g *gitOpsService) SyncDBToGit(ctx context.Context, config GitConfig) error if !strings.HasSuffix(file, ".yaml") { continue } + + if file != sanitize.PathName(file) { + log.Errorf("Invalid file name '%s', potential path injection detected", file) + continue + } + // check if file was deleted or not - exists, err := PathExists(config.LocalPath + "/" + sanitize.PathName(file)) + exists, err := PathExists(config.LocalPath + "/" + file) if err != nil { return errors.New("Error checking file in local repo : " + file + " | " + err.Error()) } From 4af4f67e72453bd29100164fef7d9973ce7ab19a Mon Sep 17 00:00:00 2001 From: Jaeyeon Park Date: Wed, 9 Oct 2024 16:54:27 +0900 Subject: [PATCH 4/4] test: graphql server path injection with error Signed-off-by: Jaeyeon Park --- chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go b/chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go index af686001bdf..bb94c06531f 100644 --- a/chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go +++ b/chaoscenter/graphql/server/pkg/chaoshub/handler/handler_test.go @@ -164,6 +164,7 @@ func TestDownloadRemoteHub(t *testing.T) { Name: uuid.New().String() + "../path/injection", RepoURL: "https://github.com/litmuschaos/chaos-charts/archive/refs/heads/master.zip", }, + isError: true, }, } for _, tc := range testcases { @@ -224,6 +225,7 @@ func TestSyncRemoteRepo(t *testing.T) { Name: uuid.New().String() + "../path/injection", RepoURL: "https://github.com/litmuschaos/chaos-charts/archive/refs/heads/master.zip", }, + isError: true, }, } for _, tc := range testcases {