Skip to content

Commit ea4e4ce

Browse files
CopilotCopilot
authored
fix: resolve CWE-22 uncontrolled data used in path expression vulnerabilities (#6254)
Signed-off-by: nitrocode <7775707+nitrocode@users.noreply.github.com> Signed-off-by: Rui Chen <rui@chenrui.dev> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent 06b90ef commit ea4e4ce

10 files changed

Lines changed: 480 additions & 11 deletions

File tree

‎server/core/config/raw/project.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,18 @@ func (p Project) Validate() error {
124124
return nil
125125
}
126126

127+
validWorkspace := func(value any) error {
128+
strPtr := value.(*string)
129+
if strPtr == nil || *strPtr == "" {
130+
return nil
131+
}
132+
ws := *strPtr
133+
if strings.Contains(ws, "..") || strings.ContainsAny(ws, "/\\") {
134+
return errors.New("cannot contain '..', '/', or '\\'")
135+
}
136+
return nil
137+
}
138+
127139
// Validate that name doesn't contain glob patterns - glob expansion only works for 'dir'
128140
if p.Name != nil && ContainsGlobPattern(*p.Name) {
129141
return errors.New("name: cannot contain glob pattern characters ('*', '?', '['); glob expansion is only supported in the 'dir' field")
@@ -137,6 +149,7 @@ func (p Project) Validate() error {
137149

138150
return validation.ValidateStruct(&p,
139151
validation.Field(&p.Dir, validation.Required, validation.By(validDir)),
152+
validation.Field(&p.Workspace, validation.By(validWorkspace)),
140153
validation.Field(&p.PlanRequirements, validation.By(validPlanReq)),
141154
validation.Field(&p.ApplyRequirements, validation.By(validApplyReq)),
142155
validation.Field(&p.ImportRequirements, validation.By(validImportReq)),

‎server/core/config/raw/project_test.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -420,6 +420,46 @@ func TestProject_Validate(t *testing.T) {
420420
},
421421
expErr: "",
422422
},
423+
{
424+
description: "workspace with ..",
425+
input: raw.Project{
426+
Dir: String("."),
427+
Workspace: String("../evil"),
428+
},
429+
expErr: "workspace: cannot contain '..', '/', or '\\'.",
430+
},
431+
{
432+
description: "workspace beginning with /",
433+
input: raw.Project{
434+
Dir: String("."),
435+
Workspace: String("/etc"),
436+
},
437+
expErr: "workspace: cannot contain '..', '/', or '\\'.",
438+
},
439+
{
440+
description: "workspace with embedded /",
441+
input: raw.Project{
442+
Dir: String("."),
443+
Workspace: String("sub/dir"),
444+
},
445+
expErr: "workspace: cannot contain '..', '/', or '\\'.",
446+
},
447+
{
448+
description: "workspace with backslash",
449+
input: raw.Project{
450+
Dir: String("."),
451+
Workspace: String("sub\\dir"),
452+
},
453+
expErr: "workspace: cannot contain '..', '/', or '\\'.",
454+
},
455+
{
456+
description: "valid workspace",
457+
input: raw.Project{
458+
Dir: String("."),
459+
Workspace: String("my-workspace"),
460+
},
461+
expErr: "",
462+
},
423463
}
424464
validation.ErrorTag = "yaml"
425465
for _, c := range cases {

‎server/events/models/models.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,9 @@ func NewRepo(vcsHostType VCSHostType, repoFullName string, cloneURL string, vcsU
165165
if strings.Contains(repo, "/") {
166166
return Repo{}, fmt.Errorf("invalid repo format %q, repo %q should not contain any /'s", repoFullName, owner)
167167
}
168+
if strings.Contains(owner, "..") || strings.Contains(repo, "..") {
169+
return Repo{}, fmt.Errorf("invalid repo format %q, owner or repo cannot contain '..'", repoFullName)
170+
}
168171

169172
return Repo{
170173
FullName: repoFullName,

‎server/events/models/models_test.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,19 @@ func TestNewRepo_FullNameWrongFormat(t *testing.T) {
9898
"/b",
9999
`invalid repo format "/b", owner "" or repo "b" was empty`,
100100
},
101+
{
102+
"owner../repo",
103+
`invalid repo format "owner../repo", owner or repo cannot contain '..'`,
104+
},
105+
{
106+
"owner/..repo",
107+
`invalid repo format "owner/..repo", owner or repo cannot contain '..'`,
108+
},
109+
// Trailing ".." in repo name
110+
{
111+
"owner/repo..",
112+
`invalid repo format "owner/repo..", owner or repo cannot contain '..'`,
113+
},
101114
}
102115
for _, c := range cases {
103116
t.Run(c.repoFullName, func(t *testing.T) {
@@ -106,6 +119,15 @@ func TestNewRepo_FullNameWrongFormat(t *testing.T) {
106119
ErrEquals(t, c.expErr, err)
107120
})
108121
}
122+
123+
// GitLab allows subgroups (slashes in owner), so the ".." check fires for
124+
// a path like "group/subgroup/../../../etc/repo" where the owner contains "..".
125+
t.Run("gitlab subgroup owner with ..", func(t *testing.T) {
126+
repoFullName := "group/subgroup/../../etc/repo"
127+
cloneURL := fmt.Sprintf("https://gitlab.com/%s.git", repoFullName)
128+
_, err := models.NewRepo(models.Gitlab, repoFullName, cloneURL, "u", "p", "")
129+
ErrEquals(t, `invalid repo format "group/subgroup/../../etc/repo", owner or repo cannot contain '..'`, err)
130+
})
109131
}
110132

111133
// If the clone url doesn't end with .git, and VCS is not Azure DevOps, it is appended

‎server/events/project_command_runner.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"github.com/runatlantis/atlantis/server/events/vcs"
2121
"github.com/runatlantis/atlantis/server/events/webhooks"
2222
"github.com/runatlantis/atlantis/server/logging"
23+
"github.com/runatlantis/atlantis/server/utils"
2324
)
2425

2526
const OperationComplete = true
@@ -457,6 +458,15 @@ func (p *DefaultProjectCommandRunner) doPolicyCheck(ctx command.ProjectContext)
457458
return nil, "", err
458459
}
459460
absPath := filepath.Join(repoDir, ctx.RepoRelDir)
461+
if err := utils.EnsureSubPath(repoDir, absPath); err != nil {
462+
463+
// let's unlock here since something probably nuked our directory between the plan and policy check phase
464+
if unlockErr := lockAttempt.UnlockFn(); unlockErr != nil {
465+
ctx.Log.Err("error unlocking state after plan error: %v", unlockErr)
466+
}
467+
468+
return nil, "", fmt.Errorf("project path traversal detected: %w", err)
469+
}
460470
if _, err = os.Stat(absPath); os.IsNotExist(err) {
461471

462472
// let's unlock here since something probably nuked our directory between the plan and policy check phase
@@ -702,6 +712,12 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model
702712
}
703713

704714
projAbsPath := filepath.Join(repoDir, ctx.RepoRelDir)
715+
if err := utils.EnsureSubPath(repoDir, projAbsPath); err != nil {
716+
if unlockErr := lockAttempt.UnlockFn(); unlockErr != nil {
717+
ctx.Log.Err("error unlocking state after plan error: %v", unlockErr)
718+
}
719+
return nil, "", fmt.Errorf("project path traversal detected: %w", err)
720+
}
705721
if _, err = os.Stat(projAbsPath); os.IsNotExist(err) {
706722
if unlockErr := lockAttempt.UnlockFn(); unlockErr != nil {
707723
ctx.Log.Err("error unlocking state after plan error: %v", unlockErr)
@@ -750,6 +766,9 @@ func (p *DefaultProjectCommandRunner) doApply(ctx command.ProjectContext) (apply
750766
return "", "", err
751767
}
752768
absPath := filepath.Join(repoDir, ctx.RepoRelDir)
769+
if err := utils.EnsureSubPath(repoDir, absPath); err != nil {
770+
return "", "", fmt.Errorf("project path traversal detected: %w", err)
771+
}
753772
if _, err = os.Stat(absPath); os.IsNotExist(err) {
754773
return "", "", DirNotExistErr{RepoRelDir: ctx.RepoRelDir}
755774
}
@@ -809,6 +828,9 @@ func (p *DefaultProjectCommandRunner) doVersion(ctx command.ProjectContext) (ver
809828
return "", "", err
810829
}
811830
absPath := filepath.Join(repoDir, ctx.RepoRelDir)
831+
if err := utils.EnsureSubPath(repoDir, absPath); err != nil {
832+
return "", "", fmt.Errorf("project path traversal detected: %w", err)
833+
}
812834
if _, err = os.Stat(absPath); os.IsNotExist(err) {
813835
return "", "", DirNotExistErr{RepoRelDir: ctx.RepoRelDir}
814836
}
@@ -835,6 +857,9 @@ func (p *DefaultProjectCommandRunner) doImport(ctx command.ProjectContext) (out
835857
return nil, "", cloneErr
836858
}
837859
projAbsPath := filepath.Join(repoDir, ctx.RepoRelDir)
860+
if err = utils.EnsureSubPath(repoDir, projAbsPath); err != nil {
861+
return nil, "", fmt.Errorf("project path traversal detected: %w", err)
862+
}
838863
if _, err = os.Stat(projAbsPath); os.IsNotExist(err) {
839864
return nil, "", DirNotExistErr{RepoRelDir: ctx.RepoRelDir}
840865
}
@@ -881,6 +906,9 @@ func (p *DefaultProjectCommandRunner) doStateRm(ctx command.ProjectContext) (out
881906
return nil, "", cloneErr
882907
}
883908
projAbsPath := filepath.Join(repoDir, ctx.RepoRelDir)
909+
if err = utils.EnsureSubPath(repoDir, projAbsPath); err != nil {
910+
return nil, "", fmt.Errorf("project path traversal detected: %w", err)
911+
}
884912
if _, err = os.Stat(projAbsPath); os.IsNotExist(err) {
885913
return nil, "", DirNotExistErr{RepoRelDir: ctx.RepoRelDir}
886914
}

‎server/events/project_command_runner_test.go‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"fmt"
1010
"os"
1111
"path/filepath"
12+
"strings"
1213
"testing"
1314

1415
"github.com/hashicorp/go-version"
@@ -2582,3 +2583,139 @@ func TestDefaultProjectCommandRunner_ApprovePolicies_HashAwareApproval(t *testin
25822583
})
25832584
}
25842585
}
2586+
2587+
func TestDefaultProjectCommandRunner_PathTraversal(t *testing.T) {
2588+
const defaultTraversalPattern = "../../../../etc"
2589+
2590+
cases := []struct {
2591+
name string
2592+
traversalPatterns []string
2593+
expectUnlock bool
2594+
runFn func(runner *events.DefaultProjectCommandRunner, ctx command.ProjectContext) error
2595+
setupWorkingDir func(mockWorkingDir *mocks.MockWorkingDir, repoDir string)
2596+
}{
2597+
{
2598+
name: "Plan",
2599+
traversalPatterns: []string{
2600+
"../../../../etc",
2601+
"../etc",
2602+
"sub/../../etc",
2603+
},
2604+
expectUnlock: true,
2605+
setupWorkingDir: func(mockWorkingDir *mocks.MockWorkingDir, repoDir string) {
2606+
When(mockWorkingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Any[string]())).
2607+
ThenReturn(repoDir, nil)
2608+
When(mockWorkingDir.MergeAgain(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Any[string]())).
2609+
ThenReturn(false, nil)
2610+
},
2611+
runFn: func(runner *events.DefaultProjectCommandRunner, ctx command.ProjectContext) error {
2612+
return runner.Plan(ctx).Error
2613+
},
2614+
},
2615+
{
2616+
name: "Apply",
2617+
traversalPatterns: []string{defaultTraversalPattern},
2618+
setupWorkingDir: func(mockWorkingDir *mocks.MockWorkingDir, repoDir string) {
2619+
When(mockWorkingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).
2620+
ThenReturn(repoDir, nil)
2621+
},
2622+
runFn: func(runner *events.DefaultProjectCommandRunner, ctx command.ProjectContext) error {
2623+
return runner.Apply(ctx).Error
2624+
},
2625+
},
2626+
{
2627+
name: "PolicyCheck",
2628+
traversalPatterns: []string{defaultTraversalPattern},
2629+
expectUnlock: true,
2630+
setupWorkingDir: func(mockWorkingDir *mocks.MockWorkingDir, repoDir string) {
2631+
When(mockWorkingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).
2632+
ThenReturn(repoDir, nil)
2633+
},
2634+
runFn: func(runner *events.DefaultProjectCommandRunner, ctx command.ProjectContext) error {
2635+
return runner.PolicyCheck(ctx).Error
2636+
},
2637+
},
2638+
{
2639+
name: "Version",
2640+
traversalPatterns: []string{defaultTraversalPattern},
2641+
setupWorkingDir: func(mockWorkingDir *mocks.MockWorkingDir, repoDir string) {
2642+
When(mockWorkingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).
2643+
ThenReturn(repoDir, nil)
2644+
},
2645+
runFn: func(runner *events.DefaultProjectCommandRunner, ctx command.ProjectContext) error {
2646+
return runner.Version(ctx).Error
2647+
},
2648+
},
2649+
{
2650+
name: "Import",
2651+
traversalPatterns: []string{defaultTraversalPattern},
2652+
setupWorkingDir: func(mockWorkingDir *mocks.MockWorkingDir, repoDir string) {
2653+
When(mockWorkingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Any[string]())).
2654+
ThenReturn(repoDir, nil)
2655+
},
2656+
runFn: func(runner *events.DefaultProjectCommandRunner, ctx command.ProjectContext) error {
2657+
return runner.Import(ctx).Error
2658+
},
2659+
},
2660+
{
2661+
name: "StateRm",
2662+
traversalPatterns: []string{defaultTraversalPattern},
2663+
setupWorkingDir: func(mockWorkingDir *mocks.MockWorkingDir, repoDir string) {
2664+
When(mockWorkingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Any[string]())).
2665+
ThenReturn(repoDir, nil)
2666+
},
2667+
runFn: func(runner *events.DefaultProjectCommandRunner, ctx command.ProjectContext) error {
2668+
return runner.StateRm(ctx).Error
2669+
},
2670+
},
2671+
}
2672+
2673+
for _, tc := range cases {
2674+
for _, pattern := range tc.traversalPatterns {
2675+
t.Run(tc.name+" rejects traversal pattern "+pattern, func(t *testing.T) {
2676+
RegisterMockTestingT(t)
2677+
mockWorkingDir := mocks.NewMockWorkingDir()
2678+
mockLocker := mocks.NewMockProjectLocker()
2679+
runner := &events.DefaultProjectCommandRunner{
2680+
Locker: mockLocker,
2681+
LockURLGenerator: mockURLGenerator{},
2682+
WorkingDir: mockWorkingDir,
2683+
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
2684+
}
2685+
repoDir := t.TempDir()
2686+
tc.setupWorkingDir(mockWorkingDir, repoDir)
2687+
unlockCalled := false
2688+
When(mockLocker.TryLock(
2689+
Any[logging.SimpleLogging](),
2690+
Any[models.PullRequest](),
2691+
Any[models.User](),
2692+
Any[string](),
2693+
Any[models.Project](),
2694+
AnyBool(),
2695+
)).ThenReturn(&events.TryLockResponse{
2696+
LockAcquired: true,
2697+
LockKey: "lock-key",
2698+
UnlockFn: func() error {
2699+
unlockCalled = true
2700+
return nil
2701+
},
2702+
}, nil)
2703+
ctx := command.ProjectContext{
2704+
Log: logging.NewNoopLogger(t),
2705+
Workspace: "default",
2706+
RepoRelDir: pattern,
2707+
RePlanCmd: "atlantis plan -d .",
2708+
}
2709+
err := tc.runFn(runner, ctx)
2710+
Assert(t, err != nil, "expected error for RepoRelDir %q in runner %q", pattern, tc.name)
2711+
Assert(t,
2712+
strings.Contains(err.Error(), "project path traversal detected"),
2713+
"expected traversal error for runner %q with RepoRelDir %q, got: %s", tc.name, pattern, err,
2714+
)
2715+
if tc.expectUnlock {
2716+
Assert(t, unlockCalled, "expected runner %q with RepoRelDir %q to release project lock", tc.name, pattern)
2717+
}
2718+
})
2719+
}
2720+
}
2721+
}

0 commit comments

Comments
 (0)