From ba1bc13f6440030872a720ea10f54fdd27e74d76 Mon Sep 17 00:00:00 2001 From: Joseph Moukarzel Date: Wed, 4 Mar 2026 16:51:23 +0100 Subject: [PATCH] feat(controls): Add unsafe variable expansion control for user-filled predefined variables --- .plumber.yaml | 67 ++- README.md | 49 +- cmd/analyze.go | 38 ++ configuration/plumberconfig.go | 38 ++ configuration/plumberconfig_test.go | 1 + .../controlGitlabPipelineVariableInjection.go | 284 ++++++++++ ...rolGitlabPipelineVariableInjection_test.go | 518 ++++++++++++++++++ control/mrcomment.go | 19 + control/task.go | 20 +- control/types.go | 1 + gitlab/utilsCI.go | 30 + 11 files changed, 1060 insertions(+), 5 deletions(-) create mode 100644 control/controlGitlabPipelineVariableInjection.go create mode 100644 control/controlGitlabPipelineVariableInjection_test.go diff --git a/.plumber.yaml b/.plumber.yaml index 17fe60f..21f16c4 100644 --- a/.plumber.yaml +++ b/.plumber.yaml @@ -164,7 +164,7 @@ controls: # Set to false to disable this control enabled: true - # Version patterns considered forbidden - includes using these will be flagged + # Version patterns considered forbidden: includes using these will be flagged forbiddenVersions: - latest - "~latest" @@ -263,3 +263,68 @@ controls: forbiddenVariables: - CI_DEBUG_TRACE - CI_DEBUG_SERVICES + + # =========================================== + # Pipeline must not use unsafe variable expansion + # =========================================== + # Detects user-controlled CI variables passed to commands that + # re-interpret their input as shell code. This is OWASP CICD-SEC-1. + # + # GitLab sets CI variables as environment variables. The shell does + # NOT re-parse expanded values for command substitution, so normal + # usage is safe. Only commands that re-interpret arguments as code + # create an injection surface. + # + # Flagged (re-interpretation contexts): + # - eval "$CI_COMMIT_BRANCH" + # - sh -c "$CI_MERGE_REQUEST_TITLE" + # - bash -c "$CI_COMMIT_MESSAGE" + # - dash -c / zsh -c / ksh -c + # - source <(echo "$CI_COMMIT_REF_NAME") + # - envsubst '$CI_COMMIT_MESSAGE' < tpl.sh | sh + # - echo "$CI_COMMIT_BRANCH" | xargs sh + # + # Not flagged (safe — shell doesn't re-parse env var values): + # - echo $CI_COMMIT_BRANCH + # - echo "$CI_COMMIT_MESSAGE" + # - curl -d "$CI_MERGE_REQUEST_TITLE" https://... + # - git checkout $CI_COMMIT_REF_NAME + # - printf '%s' "$CI_COMMIT_MESSAGE" + # + # Not caught (known limitation): + # - sh -c $BRANCH (where BRANCH: $CI_COMMIT_BRANCH in variables:) + # Indirect aliasing is not tracked; only direct variable names. + pipelineMustNotUseUnsafeVariableExpansion: + # Set to false to disable this control + enabled: true + + # CI/CD variables whose values come from user input and must not + # appear in shell re-interpretation contexts (eval, sh -c, bash -c, etc.) + dangerousVariables: + - CI_MERGE_REQUEST_TITLE + - CI_MERGE_REQUEST_DESCRIPTION + - CI_COMMIT_MESSAGE + - CI_COMMIT_TITLE + - CI_COMMIT_TAG_MESSAGE + - CI_COMMIT_REF_NAME + - CI_COMMIT_REF_SLUG + - CI_COMMIT_BRANCH + - CI_MERGE_REQUEST_SOURCE_BRANCH_NAME + - CI_EXTERNAL_PULL_REQUEST_SOURCE_BRANCH_NAME + + # Script lines + # sh -c "helm upgrade myapp . --set image.tag=$CI_COMMIT_SHA" + # sh -c "terraform workspace select $CI_COMMIT_REF_SLUG" + # sh -c "docker build --build-arg BRANCH=$CI_COMMIT_BRANCH ." + # sh -c "aws s3 sync dist/ s3://my-bucket/$CI_COMMIT_REF_SLUG/" + # bash -c "make deploy BRANCH=$CI_COMMIT_BRANCH" + # + # Allow with patterns: + # (Escape $ as \\$, {} as \\{ \\} in patterns.) + # allowedPatterns: + # - "helm.*--set.*\\$CI_" + # - "terraform workspace select.*\\$CI_" + # - "docker build.*--build-arg.*\\$CI_" + # - "aws s3 sync.*\\$CI_" + # - "make deploy.*\\$CI_" + allowedPatterns: [] \ No newline at end of file diff --git a/README.md b/README.md index ae4f96d..13e1c2a 100644 --- a/README.md +++ b/README.md @@ -36,6 +36,7 @@ Plumber is a compliance scanner for GitLab. It reads your `.gitlab-ci.yml` and r - Forbidden version patterns (e.g., `main`, `HEAD`) - Missing required components or templates - Debug trace variables (`CI_DEBUG_TRACE`) leaking secrets in job logs +- Unsafe variable injection via `eval`/`sh -c`/`bash -c` (OWASP CICD-SEC-1) **How does it work?** Plumber connects to your GitLab instance via API, analyzes your pipeline configuration, and reports any issues it finds. You define what's allowed in a config file (`.plumber.yaml`), and Plumber tells you if your project complies. When running locally from your git repo, Plumber uses your **local `.gitlab-ci.yml`** allowing you to validate changes before pushing. @@ -274,7 +275,7 @@ This creates `.plumber.yaml` with sensible [defaults](./.plumber.yaml). Customiz ### Available Controls -Plumber includes 9 compliance controls. Each can be enabled/disabled and customized in [.plumber.yaml](.plumber.yaml): +Plumber includes 10 compliance controls. Each can be enabled/disabled and customized in [.plumber.yaml](.plumber.yaml):
1. Container images must not use forbidden tags @@ -466,6 +467,47 @@ pipelineMustNotEnableDebugTrace:
+
+10. Pipeline must not use unsafe variable expansion + +Detects user-controlled CI variables (MR title, commit message, branch name) passed to commands that re-interpret their input as shell code. An attacker can craft a branch name or MR title to inject arbitrary commands: this is [OWASP CICD-SEC-1](https://owasp.org/www-project-top-10-ci-cd-security-risks/). + +GitLab sets CI variables as environment variables. The shell does **not** re-parse expanded values for command substitution, so normal usage is safe. Only commands that re-interpret their arguments as code are flagged: + +**Flagged**: re-interpretation contexts: +- `eval "$CI_COMMIT_BRANCH"` +- `sh -c "$CI_MERGE_REQUEST_TITLE"` / `bash -c` / `dash -c` / `zsh -c` / `ksh -c` +- `source <(echo "$CI_COMMIT_REF_NAME")` +- `envsubst '$CI_COMMIT_MESSAGE' < tpl.sh | sh` +- `echo "$CI_COMMIT_BRANCH" | xargs sh` + +**Not flagged**: safe, shell doesn't re-parse env var values: +- `echo $CI_COMMIT_BRANCH` / `echo "$CI_COMMIT_MESSAGE"` +- `curl -d "$CI_MERGE_REQUEST_TITLE" https://...` +- `git checkout $CI_COMMIT_REF_NAME` +- `printf '%s' "$CI_COMMIT_MESSAGE"` + +> **Limitation:** only direct variable names are detected. Indirect aliasing (`variables: { B: $CI_COMMIT_BRANCH }` then `sh -c $B`) is not tracked. + +```yaml +pipelineMustNotUseUnsafeVariableExpansion: + enabled: true + dangerousVariables: + - CI_MERGE_REQUEST_TITLE + - CI_MERGE_REQUEST_DESCRIPTION + - CI_COMMIT_MESSAGE + - CI_COMMIT_TITLE + - CI_COMMIT_TAG_MESSAGE + - CI_COMMIT_REF_NAME + - CI_COMMIT_REF_SLUG + - CI_COMMIT_BRANCH + - CI_MERGE_REQUEST_SOURCE_BRANCH_NAME + - CI_EXTERNAL_PULL_REQUEST_SOURCE_BRANCH_NAME + allowedPatterns: [] +``` + +
+ ### Selective Control Execution You can run or skip specific controls using their YAML key names from `.plumber.yaml`. This is useful for iterative debugging or targeted CI checks. @@ -509,6 +551,7 @@ Controls not selected are reported as **skipped** in the output. The `--controls | `pipelineMustIncludeTemplate` | | `pipelineMustNotEnableDebugTrace` | | `pipelineMustNotIncludeHardcodedJobs` | +| `pipelineMustNotUseUnsafeVariableExpansion` | @@ -646,10 +689,10 @@ brew install plumber To install a specific version: ```bash -brew install getplumber/plumber/plumber@0.1.51 +brew install getplumber/plumber/plumber@0.1.52 ``` -> **Note:** Versioned formulas are keg-only. Use the full path for example `/usr/local/opt/plumber@0.1.51/bin/plumber` or run `brew link plumber@0.1.51` to add it to your PATH. +> **Note:** Versioned formulas are keg-only. Use the full path for example `/usr/local/opt/plumber@0.1.52/bin/plumber` or run `brew link plumber@0.1.52` to add it to your PATH. ### Mise diff --git a/cmd/analyze.go b/cmd/analyze.go index 433cebc..9538b06 100644 --- a/cmd/analyze.go +++ b/cmd/analyze.go @@ -298,6 +298,11 @@ func runAnalyze(cmd *cobra.Command, args []string) error { controlCount++ } + if result.VariableInjectionResult != nil && !result.VariableInjectionResult.Skipped { + complianceSum += result.VariableInjectionResult.Compliance + controlCount++ + } + // Calculate average compliance // If no controls ran (e.g., data collection failed), compliance is 0% - we can't verify anything var compliance float64 = 0 @@ -972,6 +977,39 @@ func outputText(result *control.AnalysisResult, threshold, compliance float64, c fmt.Println() } + // Control 10: Pipeline must not use unsafe variable expansion + if result.VariableInjectionResult != nil { + ctrl := controlSummary{ + name: "Pipeline must not use unsafe variable expansion", + compliance: result.VariableInjectionResult.Compliance, + issues: len(result.VariableInjectionResult.Issues), + skipped: result.VariableInjectionResult.Skipped, + } + controls = append(controls, ctrl) + + printControlHeader("Pipeline must not use unsafe variable expansion", result.VariableInjectionResult.Compliance, result.VariableInjectionResult.Skipped) + + if result.VariableInjectionResult.Skipped { + fmt.Printf(" %sStatus: SKIPPED (disabled in configuration)%s\n", colorDim, colorReset) + } else { + fmt.Printf(" Jobs Checked: %d\n", result.VariableInjectionResult.Metrics.JobsChecked) + fmt.Printf(" Script Lines Checked: %d\n", result.VariableInjectionResult.Metrics.TotalScriptLinesChecked) + fmt.Printf(" Unsafe Expansions: %d\n", result.VariableInjectionResult.Metrics.UnsafeExpansionsFound) + + if len(result.VariableInjectionResult.Issues) > 0 { + fmt.Printf("\n %sUnsafe Variable Expansions Found:%s\n", colorYellow, colorReset) + for _, issue := range result.VariableInjectionResult.Issues { + if issue.JobName == "(global)" { + fmt.Printf(" %s•%s $%s in global %s: %s\n", colorYellow, colorReset, issue.VariableName, issue.ScriptBlock, issue.ScriptLine) + } else { + fmt.Printf(" %s•%s $%s in job '%s' %s: %s\n", colorYellow, colorReset, issue.VariableName, issue.JobName, issue.ScriptBlock, issue.ScriptLine) + } + } + } + } + fmt.Println() + } + // Summary Section printSectionHeader("Summary") fmt.Println() diff --git a/configuration/plumberconfig.go b/configuration/plumberconfig.go index 0a979c4..a08e9a8 100644 --- a/configuration/plumberconfig.go +++ b/configuration/plumberconfig.go @@ -41,6 +41,9 @@ var validControlSchema = map[string][]string{ "pipelineMustNotEnableDebugTrace": { "enabled", "forbiddenVariables", }, + "pipelineMustNotUseUnsafeVariableExpansion": { + "enabled", "dangerousVariables", "allowedPatterns", + }, } // validControlKeys returns the list of known control names. @@ -96,6 +99,9 @@ type ControlsConfig struct { // PipelineMustNotEnableDebugTrace control configuration PipelineMustNotEnableDebugTrace *DebugTraceControlConfig `yaml:"pipelineMustNotEnableDebugTrace,omitempty"` + + // PipelineMustNotUseUnsafeVariableExpansion control configuration + PipelineMustNotUseUnsafeVariableExpansion *VariableInjectionControlConfig `yaml:"pipelineMustNotUseUnsafeVariableExpansion,omitempty"` } // ImageForbiddenTagsControlConfig configuration for the forbidden image tags control @@ -236,6 +242,20 @@ type DebugTraceControlConfig struct { ForbiddenVariables []string `yaml:"forbiddenVariables,omitempty"` } +// VariableInjectionControlConfig configuration for the unsafe variable expansion control +type VariableInjectionControlConfig struct { + // Enabled controls whether this check runs + Enabled *bool `yaml:"enabled,omitempty"` + + // DangerousVariables is a list of CI/CD variable names whose values come from user input + // and should not appear in script blocks where shell injection is possible + DangerousVariables []string `yaml:"dangerousVariables,omitempty"` + + // AllowedPatterns is a list of regex patterns. Script lines matching any of these + // patterns will not be flagged even if they contain a dangerous variable. + AllowedPatterns []string `yaml:"allowedPatterns,omitempty"` +} + // RequiredTemplatesControlConfig configuration for the required templates control type RequiredTemplatesControlConfig struct { // Enabled controls whether this check runs @@ -500,6 +520,24 @@ func (c *DebugTraceControlConfig) IsEnabled() bool { return *c.Enabled } +// GetPipelineMustNotUseUnsafeVariableExpansionConfig returns the control configuration +// Returns nil if not configured +func (c *PlumberConfig) GetPipelineMustNotUseUnsafeVariableExpansionConfig() *VariableInjectionControlConfig { + if c == nil { + return nil + } + return c.Controls.PipelineMustNotUseUnsafeVariableExpansion +} + +// IsEnabled returns whether the control is enabled +// Returns false if not properly configured +func (c *VariableInjectionControlConfig) IsEnabled() bool { + if c == nil || c.Enabled == nil { + return false + } + return *c.Enabled +} + // IsEnabled returns whether the control is enabled // Returns false if not properly configured func (c *RequiredTemplatesControlConfig) IsEnabled() bool { diff --git a/configuration/plumberconfig_test.go b/configuration/plumberconfig_test.go index bf7ca0c..c6ff1eb 100644 --- a/configuration/plumberconfig_test.go +++ b/configuration/plumberconfig_test.go @@ -327,6 +327,7 @@ func TestValidControlNames(t *testing.T) { "pipelineMustIncludeTemplate", "pipelineMustNotEnableDebugTrace", "pipelineMustNotIncludeHardcodedJobs", + "pipelineMustNotUseUnsafeVariableExpansion", } if len(names) != len(expected) { diff --git a/control/controlGitlabPipelineVariableInjection.go b/control/controlGitlabPipelineVariableInjection.go new file mode 100644 index 0000000..4f87f10 --- /dev/null +++ b/control/controlGitlabPipelineVariableInjection.go @@ -0,0 +1,284 @@ +package control + +import ( + "fmt" + "regexp" + "strings" + + "github.com/getplumber/plumber/collector" + "github.com/getplumber/plumber/configuration" + "github.com/getplumber/plumber/gitlab" + "github.com/sirupsen/logrus" +) + +const ControlTypeGitlabPipelineVariableInjectionVersion = "0.1.0" + +////////////////// +// Control conf // +////////////////// + +// GitlabPipelineVariableInjectionConf holds the configuration for unsafe variable expansion detection +type GitlabPipelineVariableInjectionConf struct { + Enabled bool `json:"enabled"` + DangerousVariables []string `json:"dangerousVariables"` + AllowedPatterns []string `json:"allowedPatterns"` +} + +// GetConf loads configuration from PlumberConfig +func (p *GitlabPipelineVariableInjectionConf) GetConf(plumberConfig *configuration.PlumberConfig) error { + if plumberConfig == nil { + p.Enabled = false + return nil + } + + cfg := plumberConfig.GetPipelineMustNotUseUnsafeVariableExpansionConfig() + if cfg == nil { + l.Debug("pipelineMustNotUseUnsafeVariableExpansion control configuration is missing from .plumber.yaml file, skipping") + p.Enabled = false + return nil + } + + if cfg.Enabled == nil { + return fmt.Errorf("pipelineMustNotUseUnsafeVariableExpansion.enabled field is required in .plumber.yaml config file") + } + + p.Enabled = cfg.IsEnabled() + p.DangerousVariables = cfg.DangerousVariables + p.AllowedPatterns = cfg.AllowedPatterns + + l.WithFields(logrus.Fields{ + "enabled": p.Enabled, + "dangerousVariables": len(p.DangerousVariables), + "allowedPatterns": len(p.AllowedPatterns), + }).Debug("pipelineMustNotUseUnsafeVariableExpansion control configuration loaded from .plumber.yaml file") + + return nil +} + +//////////////////////////// +// Control data & metrics // +//////////////////////////// + +// GitlabPipelineVariableInjectionMetrics holds metrics about unsafe variable expansion detection +type GitlabPipelineVariableInjectionMetrics struct { + JobsChecked uint `json:"jobsChecked"` + TotalScriptLinesChecked uint `json:"totalScriptLinesChecked"` + UnsafeExpansionsFound uint `json:"unsafeExpansionsFound"` +} + +// GitlabPipelineVariableInjectionResult holds the result of the control +type GitlabPipelineVariableInjectionResult struct { + Issues []GitlabPipelineVariableInjectionIssue `json:"issues"` + Metrics GitlabPipelineVariableInjectionMetrics `json:"metrics"` + Compliance float64 `json:"compliance"` + Version string `json:"version"` + CiValid bool `json:"ciValid"` + CiMissing bool `json:"ciMissing"` + Skipped bool `json:"skipped"` + Error string `json:"error,omitempty"` +} + +//////////////////// +// Control issues // +//////////////////// + +// GitlabPipelineVariableInjectionIssue represents a dangerous variable found in a code-execution context +type GitlabPipelineVariableInjectionIssue struct { + JobName string `json:"jobName"` + VariableName string `json:"variableName"` + ScriptLine string `json:"scriptLine"` + ScriptBlock string `json:"scriptBlock"` // "script", "before_script", "after_script" +} + +/////////////////////// +// Control functions // +/////////////////////// + +// Patterns that introduce a shell re-interpretation context. +// A variable expanded by the outer shell and passed to one of these +// is re-parsed as code, enabling command injection. +var shellReparsePatterns = []*regexp.Regexp{ + regexp.MustCompile(`\beval\b`), + regexp.MustCompile(`\bsh\s+-c\b`), + regexp.MustCompile(`\bbash\s+-c\b`), + regexp.MustCompile(`\bdash\s+-c\b`), + regexp.MustCompile(`\bzsh\s+-c\b`), + regexp.MustCompile(`\bksh\s+-c\b`), + regexp.MustCompile(`\benvsubst\b.*\|\s*(sh|bash|dash|zsh)`), + regexp.MustCompile(`\bxargs\s+(sh|bash)\b`), + regexp.MustCompile(`\bsource\b`), + regexp.MustCompile(`^\s*\.(\s|$)`), +} + +// isShellReparseContext returns true if the line contains a command that +// re-interprets its arguments as shell code (eval, sh -c, bash -c, etc.). +func isShellReparseContext(line string) bool { + for _, re := range shellReparsePatterns { + if re.MatchString(line) { + return true + } + } + return false +} + +// Run executes the unsafe variable expansion detection control. +// +// GitLab CI sets CI variables as environment variables; the shell does +// NOT re-parse expanded values for command substitution. So plain usage +// like `echo $CI_COMMIT_BRANCH` is safe: the shell treats the expanded +// value as an inert string. +// +// The real injection surface is commands that RE-INTERPRET their input +// as shell code: eval, sh -c, bash -c, source, etc. A user-controlled +// variable passed to these is executed as code. +func (p *GitlabPipelineVariableInjectionConf) Run(pipelineOriginData *collector.GitlabPipelineOriginData) *GitlabPipelineVariableInjectionResult { + l := l.WithFields(logrus.Fields{ + "control": "GitlabPipelineVariableInjection", + "controlVersion": ControlTypeGitlabPipelineVariableInjectionVersion, + }) + l.Info("Start unsafe variable expansion detection control") + + result := &GitlabPipelineVariableInjectionResult{ + Issues: []GitlabPipelineVariableInjectionIssue{}, + Metrics: GitlabPipelineVariableInjectionMetrics{}, + Compliance: 100.0, + Version: ControlTypeGitlabPipelineVariableInjectionVersion, + CiValid: pipelineOriginData.CiValid, + CiMissing: pipelineOriginData.CiMissing, + Skipped: false, + } + + if !p.Enabled { + l.Info("Unsafe variable expansion detection control is disabled, skipping") + result.Skipped = true + return result + } + + if len(p.DangerousVariables) == 0 { + l.Info("No dangerous variables configured, skipping") + result.Skipped = true + return result + } + + mergedConf := pipelineOriginData.MergedConf + if mergedConf == nil { + l.Warn("Merged CI configuration not available, cannot check scripts") + result.Compliance = 0 + result.Error = "merged CI configuration not available" + return result + } + + // Build regexes for each dangerous variable. + // Match $VAR or ${VAR} ensuring the unbraced form has a word boundary. + varRegexes := make(map[string]*regexp.Regexp, len(p.DangerousVariables)) + for _, v := range p.DangerousVariables { + pattern := fmt.Sprintf(`\$(?:\{%s\}|%s(?:[^a-zA-Z0-9_]|$))`, regexp.QuoteMeta(v), regexp.QuoteMeta(v)) + varRegexes[v] = regexp.MustCompile(pattern) + } + + // Compile allowed patterns + var allowedRegexes []*regexp.Regexp + for _, pat := range p.AllowedPatterns { + re, err := regexp.Compile(pat) + if err != nil { + l.WithError(err).WithField("pattern", pat).Warn("Invalid allowed pattern, ignoring") + continue + } + allowedRegexes = append(allowedRegexes, re) + } + + // Check global before_script and after_script + p.scanScriptBlock(mergedConf.BeforeScript, "(global)", "before_script", varRegexes, allowedRegexes, result) + p.scanScriptBlock(mergedConf.AfterScript, "(global)", "after_script", varRegexes, allowedRegexes, result) + + // Check per-job scripts + for jobName, jobContent := range mergedConf.GitlabJobs { + job, err := gitlab.ParseGitlabCIJob(jobContent) + if err != nil { + l.WithError(err).WithField("job", jobName).Debug("Unable to parse job, skipping") + continue + } + if job == nil { + continue + } + + result.Metrics.JobsChecked++ + + p.scanScriptBlock(job.Script, jobName, "script", varRegexes, allowedRegexes, result) + p.scanScriptBlock(job.BeforeScript, jobName, "before_script", varRegexes, allowedRegexes, result) + p.scanScriptBlock(job.AfterScript, jobName, "after_script", varRegexes, allowedRegexes, result) + } + + if len(result.Issues) > 0 { + result.Compliance = 0.0 + l.WithField("issuesCount", len(result.Issues)).Info("Unsafe variable expansions found, setting compliance to 0") + } + + l.WithFields(logrus.Fields{ + "jobsChecked": result.Metrics.JobsChecked, + "totalScriptLines": result.Metrics.TotalScriptLinesChecked, + "unsafeExpansionsFound": result.Metrics.UnsafeExpansionsFound, + "compliance": result.Compliance, + }).Info("Unsafe variable expansion detection control completed") + + return result +} + +// scanScriptBlock scans a script block for dangerous variables used in +// shell re-interpretation contexts (eval, sh -c, bash -c, etc.). +func (p *GitlabPipelineVariableInjectionConf) scanScriptBlock( + scriptField interface{}, + jobName string, + blockType string, + varRegexes map[string]*regexp.Regexp, + allowedRegexes []*regexp.Regexp, + result *GitlabPipelineVariableInjectionResult, +) { + lines := gitlab.GetScriptLines(scriptField) + for _, line := range lines { + result.Metrics.TotalScriptLinesChecked++ + + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + + if !isShellReparseContext(trimmed) { + continue + } + + if isAllowedLine(trimmed, allowedRegexes) { + continue + } + + for varName, re := range varRegexes { + if re.MatchString(line) { + result.Issues = append(result.Issues, GitlabPipelineVariableInjectionIssue{ + JobName: jobName, + VariableName: varName, + ScriptLine: truncateLine(trimmed, 200), + ScriptBlock: blockType, + }) + result.Metrics.UnsafeExpansionsFound++ + } + } + } +} + +// isAllowedLine returns true if the line matches any of the allowed patterns +func isAllowedLine(line string, allowedRegexes []*regexp.Regexp) bool { + for _, re := range allowedRegexes { + if re.MatchString(line) { + return true + } + } + return false +} + +// truncateLine shortens a script line for display, appending "..." if truncated +func truncateLine(line string, maxLen int) string { + if len(line) <= maxLen { + return line + } + return line[:maxLen] + "..." +} diff --git a/control/controlGitlabPipelineVariableInjection_test.go b/control/controlGitlabPipelineVariableInjection_test.go new file mode 100644 index 0000000..2b2ea61 --- /dev/null +++ b/control/controlGitlabPipelineVariableInjection_test.go @@ -0,0 +1,518 @@ +package control + +import ( + "testing" + + "github.com/getplumber/plumber/collector" + "github.com/getplumber/plumber/gitlab" +) + +func buildPipelineOriginDataWithJobs(jobs map[string]interface{}) *collector.GitlabPipelineOriginData { + mergedConf := &gitlab.GitlabCIConf{ + GitlabJobs: jobs, + } + return &collector.GitlabPipelineOriginData{ + MergedConf: mergedConf, + CiValid: true, + CiMissing: false, + } +} + +func TestVariableInjection_Disabled(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: false, + DangerousVariables: []string{"CI_COMMIT_MESSAGE"}, + } + data := buildPipelineOriginDataWithJobs(nil) + + result := conf.Run(data) + + if !result.Skipped { + t.Fatal("expected control to be skipped when disabled") + } + if result.Compliance != 100.0 { + t.Fatalf("expected compliance 100 when skipped, got %v", result.Compliance) + } +} + +func TestVariableInjection_NoDangerousVariablesConfigured(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{}, + } + data := buildPipelineOriginDataWithJobs(nil) + + result := conf.Run(data) + + if !result.Skipped { + t.Fatal("expected control to be skipped when no dangerous variables configured") + } +} + +func TestVariableInjection_NilMergedConf(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_MESSAGE"}, + } + data := &collector.GitlabPipelineOriginData{ + MergedConf: nil, + CiValid: true, + CiMissing: false, + } + + result := conf.Run(data) + + if result.Compliance != 0 { + t.Fatalf("expected compliance 0 when merged conf unavailable, got %v", result.Compliance) + } + if result.Error == "" { + t.Fatal("expected error message when merged conf unavailable") + } +} + +// -- Safe patterns: shell does NOT re-parse expanded env vars -- + +func TestVariableInjection_EchoIsSafe(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_MESSAGE"}, + } + + tests := []struct { + name string + script string + }{ + {"unquoted", "echo $CI_COMMIT_MESSAGE"}, + {"double quoted", `echo "$CI_COMMIT_MESSAGE"`}, + {"braced", "echo ${CI_COMMIT_MESSAGE}"}, + {"braced quoted", `echo "${CI_COMMIT_MESSAGE}"`}, + {"printf", `printf '%s\n' "$CI_COMMIT_MESSAGE"`}, + {"curl data", `curl -d "$CI_COMMIT_MESSAGE" https://example.com`}, + {"git checkout", "git checkout $CI_COMMIT_MESSAGE"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + jobContent := map[interface{}]interface{}{ + "script": []interface{}{tt.script}, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + if len(result.Issues) != 0 { + t.Fatalf("script %q should be safe, but got %d issues", tt.script, len(result.Issues)) + } + if result.Compliance != 100.0 { + t.Fatalf("expected compliance 100, got %v", result.Compliance) + } + }) + } +} + +// -- Dangerous patterns: commands that re-interpret args as shell code -- + +func TestVariableInjection_EvalIsDangerous(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_MERGE_REQUEST_TITLE"}, + } + + tests := []struct { + name string + script string + }{ + {"eval bare", "eval $CI_MERGE_REQUEST_TITLE"}, + {"eval quoted", `eval "echo $CI_MERGE_REQUEST_TITLE"`}, + {"eval braced", `eval "${CI_MERGE_REQUEST_TITLE}"`}, + {"eval braced no quotes", `eval ${CI_MERGE_REQUEST_TITLE}`}, + {"eval in middle", `RESULT=$(eval "echo $CI_MERGE_REQUEST_TITLE")`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + jobContent := map[interface{}]interface{}{ + "script": []interface{}{tt.script}, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + if len(result.Issues) != 1 { + t.Fatalf("script %q should be dangerous, expected 1 issue, got %d", tt.script, len(result.Issues)) + } + }) + } +} + +func TestVariableInjection_ShCIsDangerous(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_BRANCH"}, + } + + tests := []struct { + name string + script string + }{ + // sh -c variants + {"sh -c quoted", `sh -c "echo $CI_COMMIT_BRANCH"`}, + {"sh -c unquoted", `sh -c $CI_COMMIT_BRANCH`}, + {"sh -c braced", `sh -c ${CI_COMMIT_BRANCH}`}, + {"sh -c braced quoted", `sh -c "${CI_COMMIT_BRANCH}"`}, + // bash -c variants + {"bash -c quoted", `bash -c "deploy $CI_COMMIT_BRANCH"`}, + {"bash -c unquoted", `bash -c $CI_COMMIT_BRANCH`}, + {"bash -c braced quoted", `bash -c "${CI_COMMIT_BRANCH}"`}, + // dash -c variants + {"dash -c quoted", `dash -c "$CI_COMMIT_BRANCH"`}, + {"dash -c braced", `dash -c ${CI_COMMIT_BRANCH}`}, + // zsh -c variants + {"zsh -c quoted", `zsh -c "$CI_COMMIT_BRANCH"`}, + {"zsh -c unquoted", `zsh -c $CI_COMMIT_BRANCH`}, + {"zsh -c braced quoted", `zsh -c "${CI_COMMIT_BRANCH}"`}, + // ksh -c variants + {"ksh -c unquoted", `ksh -c $CI_COMMIT_BRANCH`}, + {"ksh -c quoted", `ksh -c "$CI_COMMIT_BRANCH"`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + jobContent := map[interface{}]interface{}{ + "script": []interface{}{tt.script}, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"deploy": jobContent}) + result := conf.Run(data) + if len(result.Issues) != 1 { + t.Fatalf("script %q should be dangerous, expected 1 issue, got %d", tt.script, len(result.Issues)) + } + }) + } +} + +func TestVariableInjection_XargsShIsDangerous(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_BRANCH"}, + } + + tests := []struct { + name string + script string + }{ + {"xargs sh", `echo "$CI_COMMIT_BRANCH" | xargs sh`}, + {"xargs bash", `echo "$CI_COMMIT_BRANCH" | xargs bash`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + jobContent := map[interface{}]interface{}{ + "script": []interface{}{tt.script}, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + if len(result.Issues) != 1 { + t.Fatalf("script %q should be dangerous, expected 1 issue, got %d", tt.script, len(result.Issues)) + } + }) + } +} + +func TestVariableInjection_ShellWithoutCFlagIsSafe(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_BRANCH"}, + } + + tests := []struct { + name string + script string + }{ + {"bash filename", `bash "$CI_COMMIT_BRANCH"`}, + {"bash filename braced", `bash ${CI_COMMIT_BRANCH}`}, + {"dash filename", `dash $CI_COMMIT_BRANCH`}, + {"zsh filename", `zsh "$CI_COMMIT_BRANCH"`}, + {"ksh filename", `ksh $CI_COMMIT_BRANCH`}, + {"envsubst no pipe", `envsubst "$CI_COMMIT_BRANCH"`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + jobContent := map[interface{}]interface{}{ + "script": []interface{}{tt.script}, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + if len(result.Issues) != 0 { + t.Fatalf("script %q should be safe (no -c flag), but got %d issues", tt.script, len(result.Issues)) + } + }) + } +} + +func TestVariableInjection_SourceIsDangerous(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_REF_NAME"}, + } + + jobContent := map[interface{}]interface{}{ + "script": []interface{}{ + `source <(echo "$CI_COMMIT_REF_NAME")`, + }, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + + if len(result.Issues) != 1 { + t.Fatalf("source with dangerous var should flag, got %d issues", len(result.Issues)) + } +} + +func TestVariableInjection_EnvsubstPipeIsDangerous(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_MESSAGE"}, + } + + jobContent := map[interface{}]interface{}{ + "script": []interface{}{ + `envsubst '$CI_COMMIT_MESSAGE' < template.sh | sh`, + }, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + + if len(result.Issues) != 1 { + t.Fatalf("envsubst | sh with dangerous var should flag, got %d issues", len(result.Issues)) + } +} + +// -- Aliasing through variables: block does NOT help for eval/sh -c -- + +func TestVariableInjection_AliasedVarInEvalStillDangerous(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_BRANCH"}, + } + + jobContent := map[interface{}]interface{}{ + "variables": map[interface{}]interface{}{ + "BRANCH": "$CI_COMMIT_BRANCH", + }, + "script": []interface{}{ + `eval "deploy $CI_COMMIT_BRANCH"`, + }, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"deploy": jobContent}) + result := conf.Run(data) + + if len(result.Issues) != 1 { + t.Fatalf("eval with dangerous var should flag even when aliased, got %d issues", len(result.Issues)) + } +} + +// -- Edge cases -- + +func TestVariableInjection_SkipsComments(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_MESSAGE"}, + } + + jobContent := map[interface{}]interface{}{ + "script": []interface{}{ + "# eval $CI_COMMIT_MESSAGE", + " # sh -c $CI_COMMIT_MESSAGE", + }, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + + if len(result.Issues) != 0 { + t.Fatalf("comment lines should be skipped, got %d issues", len(result.Issues)) + } +} + +func TestVariableInjection_DoesNotMatchLongerVariable(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_BRANCH"}, + } + + jobContent := map[interface{}]interface{}{ + "script": []interface{}{ + "eval $CI_COMMIT_BRANCH_NAME_EXTRA", + }, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + + if len(result.Issues) != 0 { + t.Fatalf("longer variable name should not match, got %d issues", len(result.Issues)) + } +} + +func TestVariableInjection_MultipleVarsMultipleJobs(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_MESSAGE", "CI_COMMIT_REF_NAME"}, + } + + job1 := map[interface{}]interface{}{ + "script": []interface{}{ + `eval "echo $CI_COMMIT_MESSAGE"`, + }, + } + job2 := map[interface{}]interface{}{ + "script": []interface{}{ + `bash -c "deploy $CI_COMMIT_REF_NAME"`, + }, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{ + "build": job1, + "deploy": job2, + }) + result := conf.Run(data) + + if len(result.Issues) != 2 { + t.Fatalf("expected 2 issues, got %d", len(result.Issues)) + } + if result.Compliance != 0.0 { + t.Fatalf("expected compliance 0, got %v", result.Compliance) + } +} + +func TestVariableInjection_DetectsInBeforeScript(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_REF_NAME"}, + } + + jobContent := map[interface{}]interface{}{ + "script": "echo hello", + "before_script": []interface{}{`sh -c "setup $CI_COMMIT_REF_NAME"`}, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"deploy": jobContent}) + result := conf.Run(data) + + if len(result.Issues) != 1 { + t.Fatalf("expected 1 issue, got %d", len(result.Issues)) + } + if result.Issues[0].ScriptBlock != "before_script" { + t.Fatalf("expected scriptBlock 'before_script', got %s", result.Issues[0].ScriptBlock) + } +} + +func TestVariableInjection_DetectsInAfterScript(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_TITLE"}, + } + + jobContent := map[interface{}]interface{}{ + "script": "echo hello", + "after_script": []interface{}{`eval "notify $CI_COMMIT_TITLE"`}, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"notify": jobContent}) + result := conf.Run(data) + + if len(result.Issues) != 1 { + t.Fatalf("expected 1 issue, got %d", len(result.Issues)) + } + if result.Issues[0].ScriptBlock != "after_script" { + t.Fatalf("expected scriptBlock 'after_script', got %s", result.Issues[0].ScriptBlock) + } +} + +func TestVariableInjection_GlobalBeforeScript(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_BRANCH"}, + } + + mergedConf := &gitlab.GitlabCIConf{ + BeforeScript: []interface{}{ + `eval "setup $CI_COMMIT_BRANCH"`, + }, + GitlabJobs: map[string]interface{}{}, + } + data := &collector.GitlabPipelineOriginData{ + MergedConf: mergedConf, + CiValid: true, + CiMissing: false, + } + result := conf.Run(data) + + if len(result.Issues) != 1 { + t.Fatalf("expected 1 issue from global before_script, got %d", len(result.Issues)) + } + if result.Issues[0].JobName != "(global)" { + t.Fatalf("expected job '(global)', got %s", result.Issues[0].JobName) + } +} + +func TestVariableInjection_AllowedPattern(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_REF_NAME"}, + AllowedPatterns: []string{`deploy\.sh`}, + } + + jobContent := map[interface{}]interface{}{ + "script": []interface{}{ + `sh -c "deploy.sh $CI_COMMIT_REF_NAME"`, + `sh -c "echo $CI_COMMIT_REF_NAME"`, + }, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + + if len(result.Issues) != 1 { + t.Fatalf("expected 1 issue (second line only), got %d", len(result.Issues)) + } +} + +func TestVariableInjection_CleanConfig(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_MESSAGE", "CI_MERGE_REQUEST_TITLE"}, + } + + jobContent := map[interface{}]interface{}{ + "script": []interface{}{ + "echo $CI_COMMIT_MESSAGE", + "make build", + `printf '%s' "$CI_MERGE_REQUEST_TITLE"`, + }, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + + if result.Compliance != 100.0 { + t.Fatalf("expected compliance 100, got %v", result.Compliance) + } + if len(result.Issues) != 0 { + t.Fatalf("expected no issues for safe usage, got %d", len(result.Issues)) + } +} + +func TestVariableInjection_MixedSafeAndDangerous(t *testing.T) { + conf := &GitlabPipelineVariableInjectionConf{ + Enabled: true, + DangerousVariables: []string{"CI_COMMIT_BRANCH"}, + } + + jobContent := map[interface{}]interface{}{ + "script": []interface{}{ + "echo $CI_COMMIT_BRANCH", + `git checkout "$CI_COMMIT_BRANCH"`, + `eval "deploy $CI_COMMIT_BRANCH"`, + }, + } + data := buildPipelineOriginDataWithJobs(map[string]interface{}{"build": jobContent}) + result := conf.Run(data) + + if len(result.Issues) != 1 { + t.Fatalf("expected 1 issue (only the eval line), got %d", len(result.Issues)) + } + if result.Issues[0].ScriptLine != `eval "deploy $CI_COMMIT_BRANCH"` { + t.Fatalf("expected eval line flagged, got %s", result.Issues[0].ScriptLine) + } +} diff --git a/control/mrcomment.go b/control/mrcomment.go index 06927bf..1fed312 100644 --- a/control/mrcomment.go +++ b/control/mrcomment.go @@ -192,6 +192,12 @@ func generateMRComment(result *AnalysisResult, compliance, threshold float64) st totalIssues += len(r.Issues) } } + if r := result.VariableInjectionResult; r != nil { + controls = append(controls, controlEntry{"Pipeline must not use unsafe variable expansion", r.Compliance, len(r.Issues), r.Skipped}) + if !r.Skipped { + totalIssues += len(r.Issues) + } + } // Controls summary table b.WriteString("### Controls\n\n") @@ -339,4 +345,17 @@ func writeIssueDetails(b *strings.Builder, result *AnalysisResult) { } b.WriteString("\n") } + + // Variable injection + if r := result.VariableInjectionResult; r != nil && !r.Skipped && len(r.Issues) > 0 { + b.WriteString("**Pipeline must not use unsafe variable expansion:**\n") + for _, issue := range r.Issues { + if issue.JobName == "(global)" { + fmt.Fprintf(b, "- `$%s` used in global `%s`: `%s`\n", issue.VariableName, issue.ScriptBlock, issue.ScriptLine) + } else { + fmt.Fprintf(b, "- `$%s` used in job `%s` `%s`: `%s`\n", issue.VariableName, issue.JobName, issue.ScriptBlock, issue.ScriptLine) + } + } + b.WriteString("\n") + } } diff --git a/control/task.go b/control/task.go index cc76698..542d91f 100644 --- a/control/task.go +++ b/control/task.go @@ -22,6 +22,7 @@ const ( controlPipelineMustIncludeComponent = "pipelineMustIncludeComponent" controlPipelineMustIncludeTemplate = "pipelineMustIncludeTemplate" controlPipelineMustNotEnableDebugTrace = "pipelineMustNotEnableDebugTrace" + controlPipelineMustNotUseUnsafeVariableExpansion = "pipelineMustNotUseUnsafeVariableExpansion" ) // shouldRunControl applies --controls / --skip-controls filtering for a control. @@ -72,7 +73,7 @@ func clearProgressLine(conf *configuration.Configuration) { } // analysisStepCount is the total number of progress steps reported during analysis. -const analysisStepCount = 13 +const analysisStepCount = 14 // RunAnalysis executes the complete pipeline analysis for a GitLab project func RunAnalysis(conf *configuration.Configuration) (*AnalysisResult, error) { @@ -437,6 +438,23 @@ func RunAnalysis(conf *configuration.Configuration) (*AnalysisResult, error) { debugTraceResult := debugTraceConf.Run(pipelineOriginData) result.DebugTraceResult = debugTraceResult + // 12. Run Pipeline Must Not Use Unsafe Variable Expansion control + reportProgress(conf, 13, analysisStepCount, "Checking unsafe variable expansion") + l.Info("Running Pipeline Must Not Use Unsafe Variable Expansion control") + + variableInjectionConf := &GitlabPipelineVariableInjectionConf{} + if shouldRunControl(controlPipelineMustNotUseUnsafeVariableExpansion, conf) { + if err := variableInjectionConf.GetConf(conf.PlumberConfig); err != nil { + l.WithError(err).Error("Failed to load VariableInjection config from .plumber.yaml file") + return result, fmt.Errorf("invalid configuration: %w", err) + } + } else { + variableInjectionConf.Enabled = false + } + + variableInjectionResult := variableInjectionConf.Run(pipelineOriginData) + result.VariableInjectionResult = variableInjectionResult + reportProgress(conf, analysisStepCount, analysisStepCount, "Analysis complete") l.WithFields(logrus.Fields{ diff --git a/control/types.go b/control/types.go index cbe93f7..23b5a3d 100644 --- a/control/types.go +++ b/control/types.go @@ -36,6 +36,7 @@ type AnalysisResult struct { RequiredComponentsResult *GitlabPipelineRequiredComponentsResult `json:"requiredComponentsResult,omitempty"` RequiredTemplatesResult *GitlabPipelineRequiredTemplatesResult `json:"requiredTemplatesResult,omitempty"` DebugTraceResult *GitlabPipelineDebugTraceResult `json:"debugTraceResult,omitempty"` + VariableInjectionResult *GitlabPipelineVariableInjectionResult `json:"variableInjectionResult,omitempty"` // Raw collected data (not included in JSON output, used for PBOM generation) PipelineImageData *collector.GitlabPipelineImageData `json:"-"` diff --git a/gitlab/utilsCI.go b/gitlab/utilsCI.go index 8962f8d..4d80e9c 100644 --- a/gitlab/utilsCI.go +++ b/gitlab/utilsCI.go @@ -598,6 +598,36 @@ func GetExtends(extendsInterface interface{}) ([]string, error) { } } +// GetScriptLines extracts script lines from a script field (string or []interface{}). +// Returns nil for nil input. Multi-line strings are split on newline boundaries. +func GetScriptLines(scriptInterface interface{}) []string { + if scriptInterface == nil { + return nil + } + + switch script := scriptInterface.(type) { + case string: + if script == "" { + return nil + } + return strings.Split(script, "\n") + + case []interface{}: + var lines []string + for _, v := range script { + str, ok := v.(string) + if !ok { + continue + } + lines = append(lines, str) + } + return lines + + default: + return nil + } +} + // ParseGitlabCI parses a .gitlab-ci.yml file func ParseGitlabCI(fileContent []byte) (*GitlabCIConf, error) { l := logrus.WithFields(logrus.Fields{