Input Validation / Boundary validation

Proof-of-concept (conceptual, not a live exploit): Assuming a vulnerable system (Grafana 12.4.0 or earlier) where the provisioning backend builds a shell command or otherwise concatenates the ref into a backend operation without proper sanitization, an attacker could attempt to inject arbitrary commands via the ref parameter. Attack vector (HTTP boundary): - GET /api/provisioning/files/{repo}/path/to/dashboard.json?ref=main;bash -c 'echopwn'>/tmp/pwn Expected outcome on a vulnerable system: - If the backend concatenates the ref into a shell command, the injected command executes. For example, a command like: git fetch origin main;bash -c 'echo pwn > /tmp/pwn' would create /tmp/pwn with the string 'pwn'. - In a hardened system with proper validation, the ref is checked by IsValidRef and such input is rejected with ErrInvalidRef before any backend call. Note: The PoC above is a conceptual demonstration of how unvalidated input at HTTP boundary could be abused if the backend were to interpolate the ref into shell commands. The fix prevents this by validating refs at the HTTP boundary and rejecting invalid values early.

diff --git a/apps/provisioning/pkg/repository/git/branch.go b/apps/provisioning/pkg/repository/git/branch.go index 28d4ec7b84cda..fc5d2d9e2b447 100644 --- a/apps/provisioning/pkg/repository/git/branch.go +++ b/apps/provisioning/pkg/repository/git/branch.go @@ -9,6 +9,22 @@ import ( // it does not cover all cases as positive lookaheads are not supported in Go's regexp var basicGitBranchNameRegex = regexp.MustCompile(`^[a-zA-Z0-9\-\_\/\.]+$`) +// commitHashRegex matches a 7–40 character hex string covering short and full git SHAs. +var commitHashRegex = regexp.MustCompile(`^[0-9a-fA-F]{7,40}$`) + +// IsValidRef reports whether ref is a valid git ref to forward to a backend. +// An empty ref is considered valid: callers default it to the configured branch. +// A non-empty ref must be either a valid git branch name or a 7–40 char commit SHA. +func IsValidRef(ref string) bool { + if ref == "" { + return true + } + if commitHashRegex.MatchString(ref) { + return true + } + return IsValidGitBranchName(ref) +} + // IsValidGitBranchName checks if a branch name is valid. // It uses the following regexp `^[a-zA-Z0-9\-\_\/\.]+$` to validate the branch name with some additional checks that must satisfy the following rules: // 1. The branch name must have at least one character and must not be empty. diff --git a/apps/provisioning/pkg/repository/git/branch_test.go b/apps/provisioning/pkg/repository/git/branch_test.go index fd6d3259a81e6..6e6971b5c294e 100644 --- a/apps/provisioning/pkg/repository/git/branch_test.go +++ b/apps/provisioning/pkg/repository/git/branch_test.go @@ -1,11 +1,75 @@ package git import ( + "strings" "testing" "github.com/stretchr/testify/assert" ) +func TestIsValidRef(t *testing.T) { + tests := []struct { + name string + ref string + want bool + }{ + // Empty: callers default to the configured branch downstream. + {"empty", "", true}, + + // Valid branch names. + {"simple branch", "main", true}, + {"branch with slash", "feature/my-branch", true}, + {"branch with hyphen", "feature-x", true}, + {"branch with underscore", "feature_x", true}, + {"branch with dot", "v1.0", true}, + {"release branch", "release/v2.10.3", true}, + + // Valid commit SHAs. + {"short sha 7 chars", "abc1234", true}, + {"sha 8 chars", "abc12345", true}, + {"full sha 40 chars", "abcdef0123456789abcdef0123456789abcdef01", true}, + {"upper case sha", "ABCDEF0123456789ABCDEF0123456789ABCDEF01", true}, + {"mixed case sha", "AbCdEf0123456789", true}, + + // Invalid: shell metacharacters and other dangerous chars. + {"shell injection semicolon", "main; rm -rf /", false}, + {"shell injection pipe", "main|cat", false}, + {"shell injection backtick", "main`whoami`", false}, + {"shell injection dollar", "main$(whoami)", false}, + {"shell injection ampersand", "main&", false}, + {"space", "main branch", false}, + {"colon", "main:foo", false}, + {"question mark", "main?", false}, + {"asterisk", "main*", false}, + {"tilde", "main~1", false}, + {"caret", "main^", false}, + {"left bracket", "main[", false}, + {"backslash", "main\\branch", false}, + + // Invalid: git branch naming rules. + {"leading slash", "/main", false}, + {"trailing slash", "main/", false}, + {"trailing dot", "main.", false}, + {"double dots", "feature/..bad", false}, + {"double slashes", "feature//bad", false}, + {"trailing .lock", "feature.lock", false}, + + // Hex strings shorter than the SHA minimum still pass branch-name validation, + // so they are accepted as branches (callers cannot tell the difference and + // the backend resolves the ambiguity). + {"6-char hex is valid branch", "abcdef", true}, + {"41-char hex is valid branch", strings.Repeat("a", 41), true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsValidRef(tt.ref); got != tt.want { + t.Errorf("IsValidRef(%q) = %v, want %v", tt.ref, got, tt.want) + } + }) + } +} + func TestIsValidGitBranchName(t *testing.T) { tests := []struct { name string diff --git a/apps/provisioning/pkg/repository/github/repository_test.go b/apps/provisioning/pkg/repository/github/repository_test.go index 8cdc30a796b0d..610eaa4567afc 100644 --- a/apps/provisioning/pkg/repository/github/repository_test.go +++ b/apps/provisioning/pkg/repository/github/repository_test.go @@ -489,6 +489,78 @@ func TestGitHubRepositoryHistory(t *testing.T) { }, expectedError: errors.New("get commits: API error"), }, + { + name: "valid branch name with slashes is accepted", + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + GitHub: &provisioning.GitHubRepositoryConfig{ + Branch: "main", + Path: "dashboards", + }, + }, + }, + path: "dashboard.json", + ref: "feature/my-branch", + mockSetup: func(m *MockClient) { + m.On("Commits", mock.Anything, "grafana", "grafana", "dashboards/dashboard.json", "feature/my-branch"). + Return([]Commit{}, nil) + }, + expectedResult: []provisioning.HistoryItem{}, + }, + { + name: "valid short commit SHA is accepted", + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + GitHub: &provisioning.GitHubRepositoryConfig{ + Branch: "main", + Path: "dashboards", + }, + }, + }, + path: "dashboard.json", + ref: "abc1234", + mockSetup: func(m *MockClient) { + m.On("Commits", mock.Anything, "grafana", "grafana", "dashboards/dashboard.json", "abc1234"). + Return([]Commit{}, nil) + }, + expectedResult: []provisioning.HistoryItem{}, + }, + { + name: "valid full commit SHA is accepted", + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + GitHub: &provisioning.GitHubRepositoryConfig{ + Branch: "main", + Path: "dashboards", + }, + }, + }, + path: "dashboard.json", + ref: "abcdef0123456789abcdef0123456789abcdef01", + mockSetup: func(m *MockClient) { + m.On("Commits", mock.Anything, "grafana", "grafana", "dashboards/dashboard.json", "abcdef0123456789abcdef0123456789abcdef01"). + Return([]Commit{}, nil) + }, + expectedResult: []provisioning.HistoryItem{}, + }, + { + name: "6-char hex ref is treated as a valid branch name and forwarded", + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + GitHub: &provisioning.GitHubRepositoryConfig{ + Branch: "main", + Path: "dashboards", + }, + }, + }, + path: "dashboard.json", + ref: "abcdef", + mockSetup: func(m *MockClient) { + m.On("Commits", mock.Anything, "grafana", "grafana", "dashboards/dashboard.json", "abcdef"). + Return([]Commit{}, nil) + }, + expectedResult: []provisioning.HistoryItem{}, + }, } for _, tt := range tests { diff --git a/apps/provisioning/pkg/repository/repository.go b/apps/provisioning/pkg/repository/repository.go index 4020984fece93..1b08eff12c903 100644 --- a/apps/provisioning/pkg/repository/repository.go +++ b/apps/provisioning/pkg/repository/repository.go @@ -78,6 +78,9 @@ var ErrTooManyItems error = &apierrors.StatusError{ErrStatus: metav1.Status{ var ErrRepositoryMismatch = apierrors.NewBadRequest("repository mismatch") +// ErrInvalidRef indicates that a provided git ref (branch or commit SHA) failed validation. +var ErrInvalidRef = apierrors.NewBadRequest("invalid ref") + type FileInfo struct { // Path to the file on disk. // No leading or trailing slashes will be contained within. diff --git a/pkg/registry/apis/provisioning/files.go b/pkg/registry/apis/provisioning/files.go index 4bec708589e8f..02d73188e0184 100644 --- a/pkg/registry/apis/provisioning/files.go +++ b/pkg/registry/apis/provisioning/files.go @@ -17,6 +17,7 @@ import ( provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/apps/provisioning/pkg/quotas" "github.com/grafana/grafana/apps/provisioning/pkg/repository" + "github.com/grafana/grafana/apps/provisioning/pkg/repository/git" "github.com/grafana/grafana/apps/provisioning/pkg/safepath" "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/utils" @@ -197,6 +198,12 @@ func (c *filesConnector) parseRequestOptions(r *http.Request, name string, repo Branch: repo.Config().Branch(), } + // Reject unvalidated refs before they reach any backend. Empty is allowed and + // is defaulted to the configured branch downstream. + if !git.IsValidRef(opts.Ref) { + return opts, repository.ErrInvalidRef + } + path, err := pathAfterPrefix(r.URL.Path, fmt.Sprintf("/%s/files", name)) if err != nil { return opts, err diff --git a/pkg/registry/apis/provisioning/files_test.go b/pkg/registry/apis/provisioning/files_test.go index cba6f596f35b4..92e9823e75cf6 100644 --- a/pkg/registry/apis/provisioning/files_test.go +++ b/pkg/registry/apis/provisioning/files_test.go @@ -424,6 +424,53 @@ func TestParseRequestOptionsPathValidation(t *testing.T) { } } +func TestParseRequestOptionsRefValidation(t *testing.T) { + tests := []struct { + name string + ref string + wantErr bool + }{ + {name: "empty ref is allowed", ref: ""}, + {name: "valid branch name", ref: "main"}, + {name: "valid branch with slash", ref: "feature/my-branch"}, + {name: "valid short commit SHA", ref: "abc1234"}, + {name: "valid full commit SHA", ref: "abcdef0123456789abcdef0123456789abcdef01"}, + {name: "invalid ref with semicolon", ref: "main; rm -rf /", wantErr: true}, + {name: "invalid ref with space", ref: "main branch", wantErr: true}, + {name: "invalid ref with backtick", ref: "main`whoami`", wantErr: true}, + {name: "invalid ref with double dots", ref: "feature/..bad", wantErr: true}, + {name: "invalid ref with newline", ref: "main\nfoo", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockRepo := repository.NewMockRepository(t) + mockRepo.On("Config").Return(&provisioningapi.Repository{ + Spec: provisioningapi.RepositorySpec{ + Title: "test-repo", + }, + }).Maybe() + + connector := &filesConnector{} + r := httptest.NewRequest(http.MethodGet, "/test-repo/files/dashboard.json", nil) + if tt.ref != "" { + q := r.URL.Query() + q.Set("ref", tt.ref) + r.URL.RawQuery = q.Encode() + } + + _, err := connector.parseRequestOptions(r, "test-repo", mockRepo) + + if tt.wantErr { + require.Error(t, err) + require.ErrorIs(t, err, repository.ErrInvalidRef) + } else { + require.NoError(t, err) + } + }) + } +} + func TestHandleGetRawFile(t *testing.T) { tests := []struct { name string diff --git a/pkg/registry/apis/provisioning/history.go b/pkg/registry/apis/provisioning/history.go index fa8a55b69b3b1..c4a11d7729812 100644 --- a/pkg/registry/apis/provisioning/history.go +++ b/pkg/registry/apis/provisioning/history.go @@ -13,6 +13,7 @@ import ( "github.com/grafana/grafana-app-sdk/logging" provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/apps/provisioning/pkg/repository" + "github.com/grafana/grafana/apps/provisioning/pkg/repository/git" "github.com/grafana/grafana/apps/provisioning/pkg/safepath" ) @@ -61,15 +62,22 @@ func (h *historySubresource) Connect(ctx context.Context, name string, opts runt } return WithTimeout(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + query := r.URL.Query() + ref := query.Get("ref") + + // Reject unvalidated refs before they reach any backend. Empty is allowed + // and defaulted by the backend to the configured branch. + if !git.IsValidRef(ref) { + responder.Error(repository.ErrInvalidRef) + return + } + versioned, ok := repo.(repository.Versioned) if !ok { responder.Error(apierrors.NewBadRequest("this repository does not support history")) return } - query := r.URL.Query() - ref := query.Get("ref") - filePath, err := pathAfterPrefix(r.URL.Path, fmt.Sprintf("/%s/history/", name)) if err != nil { responder.Error(apierrors.NewBadRequest(err.Error())) diff --git a/pkg/registry/apis/provisioning/history_test.go b/pkg/registry/apis/provisioning/history_test.go new file mode 100644 index 0000000000000..58bc67cd6d4a2 --- /dev/null +++ b/pkg/registry/apis/provisioning/history_test.go @@ -0,0 +1,85 @@ +package provisioning + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + + provisioningapi "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/apps/provisioning/pkg/repository" +) + +// versionedFakeRepo embeds MockRepository and adds a History method so the +// connector's `repo.(repository.Versioned)` assertion succeeds. We track +// whether History was called to assert backend invocation order. +type versionedFakeRepo struct { + *repository.MockRepository + historyCalled bool +} + +func (v *versionedFakeRepo) History(_ context.Context, _, _ string) ([]provisioningapi.HistoryItem, error) { + v.historyCalled = true + return []provisioningapi.HistoryItem{}, nil +} + +func (v *versionedFakeRepo) LatestRef(_ context.Context) (string, error) { return "", nil } +func (v *versionedFakeRepo) ListRefs(_ context.Context) ([]provisioningapi.RefItem, error) { + return nil, nil +} +func (v *versionedFakeRepo) CompareFiles(_ context.Context, _, _ string) ([]repository.VersionedFileChange, error) { + return nil, nil +} + +func TestHistorySubresource_RefValidation(t *testing.T) { + tests := []struct { + name string + ref string + wantInvalidRefErr bool + wantHistoryCalled bool + }{ + {name: "empty ref forwarded", ref: "", wantHistoryCalled: true}, + {name: "valid branch", ref: "main", wantHistoryCalled: true}, + {name: "valid branch with slash", ref: "feature/my-branch", wantHistoryCalled: true}, + {name: "valid short SHA", ref: "abc1234", wantHistoryCalled: true}, + {name: "valid full SHA", ref: "abcdef0123456789abcdef0123456789abcdef01", wantHistoryCalled: true}, + {name: "invalid ref with semicolon", ref: "main; rm -rf /", wantInvalidRefErr: true}, + {name: "invalid ref with backtick", ref: "main`whoami`", wantInvalidRefErr: true}, + {name: "invalid ref with double dots", ref: "feature/..bad", wantInvalidRefErr: true}, + {name: "invalid ref with space", ref: "main branch", wantInvalidRefErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockRepo := repository.NewMockRepository(t) + mockRepo.On("Config").Return(&provisioningapi.Repository{}).Maybe() + fakeRepo := &versionedFakeRepo{MockRepository: mockRepo} + + h := &historySubresource{repoGetter: &fakeRepoGetter{repo: fakeRepo}} + responder := &fakeResponder{} + + handler, err := h.Connect(context.Background(), "test-repo", nil, responder) + require.NoError(t, er ... [truncated]