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

gitserver: Only allow acceptable commands and POST method in /exec #30833

Merged
merged 8 commits into from Feb 10, 2022

Conversation

indradhanush
Copy link
Member

@indradhanush indradhanush commented Feb 8, 2022

Patch for https://github.com/sourcegraph/security-issues/issues/213.

Note to reviewers

This PR also fixes an edge case with the isAllowedGitCmd function. So it's suggested to look at the commits one after the other during review as this function and related functions and variables have also been moved from internal/vcs/git to internal/gitserver/gitdomain.

Without this move, golangci-lint caught a cyclic dependency between cmd/gitserver/server and internal/vcs/git:

  1. internal/vcs/git/main_test.go already imports cmd/gitserver/server
  2. cmd/gitserver/server/server.go would now be importing internal/vcs/git if we didn't move IsAllowedGitCmd to internal/gitserver/gitdomain package.

Regression concerns

On scanning the PR commit by commit, reviewers should also notice the addition of multiple new commands to the allow list. This does not introduce new behavior, but adds the existing set of commands which are already in use in the /exec endpoint. With the check for the allow list in in func (s *Server) exec, existing tests were failing unless these commands and in some cases extra args were added to the allow list. My concern here is that if we have existing commands in use for which test cases do not exist, this PR could lead to a severe regression.

My thoughts at this point are to keep the code around the allow list checking guarded behind a feature flag while at the same time doing a dry run check and logging any commands that fail the check for the next few days. Observing the logs in production should give us a concrete list of commands that are not present in the allow list at the moment. I'd appreciate any thoughts or suggestions on the best path to take forward.

Update

The PR now does the following in order to avoid regressions:

  1. Increment the prometheus metric src_gitserver_exec_blocked_command_received if a blocked command is received by gitserver's /exec endpoint
  2. Always log blocked commands as a warning
  3. If the feature flag experimentalFeatures.enableGitServerCommandExecFilter is set to true, block the command from being executed or else default to current behavior of letting the command be executed

Note: By default the feature flag is disabled and merging this PR WILL NOT fix the linked issue.

Test Plan

  1. Merge the PR and deploy to cloud
  2. Monitor the new metric src_gitserver_exec_blocked_command_received for 2-3 days
  3. If the metric has a non-zero value after this initial monitoring period, inspect logs to identify commands that are logged as blocked commands
  4. Depending on the command update the allow list
  5. GOTO 1 until the metric's value is 0 (Note: Use rate or increase as this is a monotonically increasing counter)

Tagging @sourcegraph/security for suggestions on testing plan and timeline with respect to SLA.

@cla-bot cla-bot bot added the cla-signed label Feb 8, 2022
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Feb 8, 2022

Codenotify: Notifying subscribers in CODENOTIFY files for diff 984cac8...c00d7c0.

Notify File(s)
@keegancsmith internal/vcs/git/exec.go
@ryanslade cmd/gitserver/server/server.go
cmd/gitserver/server/server_test.go

@indradhanush indradhanush marked this pull request as draft February 8, 2022 12:51
@david-sandy
Copy link
Contributor

Other than the few questions I posted, this looks really nice! Very good job organizing this code to make it very readable and understandable. 🥇

I also added @andreeleuterio to review this change.

This commit fixes the following test:

internal/gitserer/TestClient_Archive
@david-sandy
Copy link
Contributor

I am also going to try running this locally and see what I can do.

@indradhanush indradhanush marked this pull request as ready for review February 9, 2022 10:30
@indradhanush indradhanush requested review from a team and keegancsmith February 9, 2022 10:30
@ryanslade
Copy link
Contributor

I think doing a first pass that logs commands that would have been blocked is a good idea. I'm not sure whether we need it behind a feature flag if it's only logging though.

@indradhanush
Copy link
Member Author

I'm not sure whether we need it behind a feature flag if it's only logging though.

Sorry I didn't clarify it well. What I meant was:

  1. Log by default
  2. Guard the behavior to return on a blocked command behind a feature flag and have it disabled by default

That way we can flip it back if we see any new errors.

@ryanslade
Copy link
Contributor

Makes sense. Also worth adding a counter since this would a nice things to visualise.

I think something as simple counting the number of blocked / allowed commands would be enough and the details of the actual commands can be in the logs.

@indradhanush
Copy link
Member Author

Makes sense. Also worth adding a counter since this would a nice things to visualise.

I think something as simple counting the number of blocked / allowed commands would be enough and the details of the actual commands can be in the logs.

Good idea. Will go ahead and implement the feature flag and metric related changes.

image

The feature flag is turned off by default and allows us to flip it
when we are confident with the changes in production after observing
the logs. Also, it lets us flip it back to the default in case there
are any regressions.

This commit also adds test to validate existing behaviour with an
without the feature flag enabled.
@indradhanush
Copy link
Member Author

Ready for a fresh round of reviews. Updated with feature flag and metric related changes and updated PR description with the latest status and suggested testing plan.

@indradhanush indradhanush merged commit 7b581de into main Feb 10, 2022
21 checks passed
@indradhanush indradhanush deleted the ig/gitserver-exec-hardening branch February 10, 2022 10:28
@indradhanush
Copy link
Member Author

This will require a gitserver rollout.

davejrt pushed a commit that referenced this pull request Feb 15, 2022
…30833)

Patch for sourcegraph/security-issues#213.

This feature is guarded behind a feature flag: 
experimentalFeatures.enableGitServerCommandExecFilter

The feature flag is turned off by default and allows us to flip it
when we are confident with the changes in production after observing
the logs. Also, it lets us flip it back to the default in case there
are any regressions.

This commit also adds a test to validate existing behaviour with and
without the feature flag enabled.

Finaly, this commit adds a metric to count the number of blocked
command executions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants