Skip to content

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

Warren Gifford requested to merge ig/gitserver-exec-hardening into main

Created by: indradhanush

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.

Merge request reports