Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Azure Pipelines #160

Merged
merged 1 commit into from
Jul 9, 2024
Merged

feat: Azure Pipelines #160

merged 1 commit into from
Jul 9, 2024

Conversation

becojo
Copy link
Contributor

@becojo becojo commented Jul 8, 2024

  • Initial support to parse Azure Pipelines.
    • Only the pr field and steps are modeled.
    • Only a subset of files are considered as Azure Pipelines.
    • No support for expressions.
  • Add rules in unverified_script_exec & injection to analyze Azure Pipelines.
  • The tasks used in a pipeline are reported in the build inventory using the purl type pkg:azurepipelinestask

@becojo becojo requested a review from a team as a code owner July 8, 2024 19:30
@@ -77,3 +77,27 @@ results contains poutine.finding(rule, pkg.purl, {
exprs := gl_injections(script)
count(exprs) > 0
}

# Azure Pipelines
patterns.azure contains `\$\((Build\.(SourceBranchName|SourceBranch|SourceVersionMessage)|System\.PullRequest\.SourceBranch)\)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Practically speaking it looks like only System.PullRequest.SourceBranch is truly user controllable, but the fact remains that all the others fit the definition, just that in all my test scenarios so far they are referirng to merge commit and the merge commit message.

That being said, the other may or may not be exploitable when the PR gets merged though... likely , I'll need to test, but good let's keep the regex like this

Comment on lines +66 to +82
if len(p.Stages) == 0 {
stage := AzureStage{}
if err := node.Decode(&stage); err != nil {
return err
}

if len(stage.Jobs) == 0 {
job := AzureJob{}
if err := node.Decode(&job); err != nil {
return err
}

stage.Jobs = append(stage.Jobs, job)
}

p.Stages = append(p.Stages, stage)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. I was wondering how you would handle this polymorphic thing.


func (s *Scanner) AzurePipelines() ([]models.AzurePipeline, error) {
pipelines := []models.AzurePipeline{}
err := filepath.Walk(s.Path,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Note to self and @SUSTAPLE117 , we should consider optimizing this because we now have a 4th filepath.Walk(). So we will loop through all files multiple times.

We should refactor to have a single Walk loop and then conditionally call the SCM / CI specific parsing.

Filed an issue #161

@fproulx-boostsecurity
Copy link
Contributor

LGTM, I'd like to do some basic smoke test before merging

@fproulx-boostsecurity
Copy link
Contributor

Tested against various orgs, no measurable performance delta. Stable.

@fproulx-boostsecurity fproulx-boostsecurity merged commit b7ae0ff into main Jul 9, 2024
8 checks passed
@fproulx-boostsecurity fproulx-boostsecurity deleted the azure-pipelines branch July 9, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants