From 8950ab3776ca56e828204673e24be232b6c8f055 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Pa=C3=9F?= <22845248+mpass99@users.noreply.github.com> Date: Fri, 21 Oct 2022 16:52:22 +0100 Subject: [PATCH] Add single quotes for inner command. Change to bash as interpreter. Forbid single quotes for user commands. --- api/swagger.yaml | 2 +- docs/configuration.md | 2 +- internal/api/runners.go | 8 +++++ internal/api/runners_test.go | 15 ++++++++ internal/nomad/nomad.go | 57 ++++++++++++++++-------------- internal/nomad/nomad_test.go | 26 ++++++-------- internal/runner/aws_runner_test.go | 6 ++-- pkg/dto/dto.go | 2 +- 8 files changed, 70 insertions(+), 48 deletions(-) diff --git a/api/swagger.yaml b/api/swagger.yaml index da31a87..0acf2a1 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -497,7 +497,7 @@ paths: type: object properties: command: - description: The command to be executed. The working directory for this execution is the working directory of the image of the execution environment + description: The command to be executed. The working directory for this execution is the working directory of the image of the execution environment. Single quotation ' can not be used. type: string example: python exercise.py privilegedExecution: diff --git a/docs/configuration.md b/docs/configuration.md index 7b46274..973e154 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -105,7 +105,7 @@ In order to function properly, Poseidon expects the following commands to be ava - `ls` - `mkfifo` - `rm` -- `sh` (not compatible with `zsh`) +- `bash` (not compatible with `sh` or `zsh`) - `sleep` - `tar` (including the `--absolute-names` option) - `true` diff --git a/internal/api/runners.go b/internal/api/runners.go index a4c0095..03e6b35 100644 --- a/internal/api/runners.go +++ b/internal/api/runners.go @@ -14,6 +14,7 @@ import ( "net/http" "net/url" "strconv" + "strings" ) const ( @@ -31,6 +32,8 @@ const ( PrivilegedExecutionKey = "privilegedExecution" ) +var ErrForbiddenCharacter = errors.New("use of forbidden character") + type RunnerController struct { manager runner.Accessor runnerRouter *mux.Router @@ -160,6 +163,11 @@ func (r *RunnerController) execute(writer http.ResponseWriter, request *http.Req if err := parseJSONRequestBody(writer, request, executionRequest); err != nil { return } + forbiddenCharacters := "'" + if strings.ContainsAny(executionRequest.Command, forbiddenCharacters) { + writeClientError(writer, ErrForbiddenCharacter, http.StatusBadRequest) + return + } var scheme string if config.Config.Server.TLS.Active { diff --git a/internal/api/runners_test.go b/internal/api/runners_test.go index 4f7bf78..6a1a9f1 100644 --- a/internal/api/runners_test.go +++ b/internal/api/runners_test.go @@ -236,6 +236,21 @@ func (s *RunnerRouteTestSuite) TestExecuteRoute() { s.Equal(http.StatusBadRequest, recorder.Code) }) + + s.Run("forbidden characters in command", func() { + recorder := httptest.NewRecorder() + executionRequest := dto.ExecutionRequest{ + Command: "echo 'forbidden'", + TimeLimit: 10, + } + body, err := json.Marshal(executionRequest) + s.Require().NoError(err) + request, err := http.NewRequest(http.MethodPost, path.String(), bytes.NewReader(body)) + s.Require().NoError(err) + + s.router.ServeHTTP(recorder, request) + s.Equal(http.StatusBadRequest, recorder.Code) + }) } func TestUpdateFileSystemRouteTestSuite(t *testing.T) { diff --git a/internal/nomad/nomad.go b/internal/nomad/nomad.go index ec86516..0f0207c 100644 --- a/internal/nomad/nomad.go +++ b/internal/nomad/nomad.go @@ -390,15 +390,15 @@ func (a *APIClient) LoadEnvironmentJobs() ([]*nomadApi.Job, error) { // both on the stdout stream. However, if the InteractiveStderr server config option is true, // we make sure that stdout and stderr are split correctly. // In order for the stderr splitting to work, the command must have the structure -// []string{..., "sh", "-c", "my-command"}. +// []string{..., "bash", "-c", "my-command"}. func (a *APIClient) ExecuteCommand(allocationID string, ctx context.Context, command []string, tty bool, privilegedExecution bool, stdin io.Reader, stdout, stderr io.Writer) (int, error) { if tty && config.Config.Server.InteractiveStderr { return a.executeCommandInteractivelyWithStderr(allocationID, ctx, command, privilegedExecution, stdin, stdout, stderr) } - exitCode, err := a.apiQuerier. - Execute(allocationID, ctx, setUserCommand(command, privilegedExecution), tty, stdin, stdout, stderr) + command = prepareCommandWithoutTTY(command, privilegedExecution) + exitCode, err := a.apiQuerier.Execute(allocationID, ctx, command, tty, stdin, stdout, stderr) if err != nil { return 1, fmt.Errorf("error executing command in API: %w", err) } @@ -414,10 +414,6 @@ func (a *APIClient) executeCommandInteractivelyWithStderr(allocationID string, c command []string, privilegedExecution bool, stdin io.Reader, stdout, stderr io.Writer) (int, error) { // Use current nano time to make the stderr fifo kind of unique. currentNanoTime := time.Now().UnixNano() - // We expect the command to be like []string{..., "sh", "-c", "my-command"}. - oldCommand := command[len(command)-1] - // Take the last command which is the one to be executed and wrap it to redirect stderr. - command[len(command)-1] = wrapCommandForStderrFifo(currentNanoTime, oldCommand) stderrExitChan := make(chan int) go func() { @@ -425,8 +421,7 @@ func (a *APIClient) executeCommandInteractivelyWithStderr(allocationID string, c defer cancel() // Catch stderr in separate execution. - stdErrCommand := setUserCommand(stderrFifoCommand(currentNanoTime), privilegedExecution) - exit, err := a.Execute(allocationID, ctx, stdErrCommand, true, + exit, err := a.Execute(allocationID, ctx, prepareCommandTTYStdErr(currentNanoTime, privilegedExecution), true, nullio.Reader{Ctx: readingContext}, stderr, io.Discard) if err != nil { log.WithError(err).WithField("runner", allocationID).Warn("Stderr task finished with error") @@ -434,7 +429,7 @@ func (a *APIClient) executeCommandInteractivelyWithStderr(allocationID string, c stderrExitChan <- exit }() - command = hideEnvironmentVariables(setUserCommand(command, privilegedExecution)) + command = prepareCommandTTY(command, currentNanoTime, privilegedExecution) exit, err := a.Execute(allocationID, ctx, command, true, stdin, stdout, io.Discard) // Wait until the stderr catch command finished to make sure we receive all output. @@ -444,7 +439,7 @@ func (a *APIClient) executeCommandInteractivelyWithStderr(allocationID string, c const ( // unsetEnvironmentVariablesFormat prepends the call to unset the passed variables before the actual command. - unsetEnvironmentVariablesFormat = "\"unset %s && %s\"" + unsetEnvironmentVariablesFormat = "unset %s && %s" // unsetEnvironmentVariablesPrefix is the prefix of all environment variables that will be filtered. unsetEnvironmentVariablesPrefix = "NOMAD_" // unsetEnvironmentVariablesShell is the shell functionality to get all environment variables starting with the prefix. @@ -460,8 +455,8 @@ const ( // stderrWrapperCommandFormat, if executed, is supposed to wait until a fifo exists (it sleeps 10ms to reduce load // cause by busy waiting on the system). Once the fifo exists, the given command is executed and its stderr // redirected to the fifo. - // Example: "until [ -e my.fifo ]; do sleep 0.01; done; (echo \"my.fifo exists\") 2> my.fifo". - stderrWrapperCommandFormat = "until [ -e %s ]; do sleep 0.01; done; (%s) 2> %s" + // Example: "'until [ -e my.fifo ]; do sleep 0.01; done; (echo \"my.fifo exists\") 2> my.fifo'". + stderrWrapperCommandFormat = "'until [ -e %s ]; do sleep 0.01; done; (%s) 2> %s'" // setUserBinaryPath is due to Poseidon requires the setuser script for Nomad environments. setUserBinaryPath = "/sbin/setuser" @@ -473,11 +468,31 @@ const ( UnprivilegedExecution = false ) -func hideEnvironmentVariables(commands []string) []string { - command := strings.Join(commands, " ") - return []string{"sh", "-c", fmt.Sprintf(unsetEnvironmentVariablesFormat, unsetEnvironmentVariablesShell, command)} +func prepareCommandWithoutTTY(commands []string, privilegedExecution bool) []string { + commands = setUserCommand(commands, privilegedExecution) + commands[len(commands)-1] = fmt.Sprintf("'%s'", commands[len(commands)-1]) + cmd := strings.Join(commands, " ") + return []string{"bash", "-c", fmt.Sprintf(unsetEnvironmentVariablesFormat, unsetEnvironmentVariablesShell, cmd)} } +func prepareCommandTTY(commands []string, currentNanoTime int64, privilegedExecution bool) []string { + commands = setUserCommand(commands, privilegedExecution) + // Take the last command which is the one to be executed and wrap it to redirect stderr. + stderrFifoPath := stderrFifo(currentNanoTime) + commands[len(commands)-1] = + fmt.Sprintf(stderrWrapperCommandFormat, stderrFifoPath, commands[len(commands)-1], stderrFifoPath) + cmd := strings.Join(commands, " ") + return []string{"bash", "-c", fmt.Sprintf(unsetEnvironmentVariablesFormat, unsetEnvironmentVariablesShell, cmd)} +} + +func prepareCommandTTYStdErr(currentNanoTime int64, privilegedExecution bool) []string { + stderrFifoPath := stderrFifo(currentNanoTime) + command := []string{"bash", "-c", fmt.Sprintf(stderrFifoCommandFormat, stderrFifoPath, stderrFifoPath, stderrFifoPath)} + return setUserCommand(command, privilegedExecution) +} + +// setUserCommand prefixes the passed command with the setUser command. +// The passed command needs to be wrapped in single quotes to avoid escaping the setUser binary. func setUserCommand(command []string, privilegedExecution bool) []string { if privilegedExecution { return command @@ -486,16 +501,6 @@ func setUserCommand(command []string, privilegedExecution bool) []string { } } -func stderrFifoCommand(id int64) []string { - stderrFifoPath := stderrFifo(id) - return []string{"sh", "-c", fmt.Sprintf(stderrFifoCommandFormat, stderrFifoPath, stderrFifoPath, stderrFifoPath)} -} - -func wrapCommandForStderrFifo(id int64, command string) string { - stderrFifoPath := stderrFifo(id) - return fmt.Sprintf(stderrWrapperCommandFormat, stderrFifoPath, command, stderrFifoPath) -} - func stderrFifo(id int64) string { return fmt.Sprintf(stderrFifoFormat, id) } diff --git a/internal/nomad/nomad_test.go b/internal/nomad/nomad_test.go index 3eca282..9818f68 100644 --- a/internal/nomad/nomad_test.go +++ b/internal/nomad/nomad_test.go @@ -657,8 +657,8 @@ type ExecuteCommandTestSuite struct { func (s *ExecuteCommandTestSuite) SetupTest() { s.allocationID = "test-allocation-id" s.ctx = context.Background() - s.testCommand = "echo 'do nothing'" - s.testCommandArray = []string{"sh", "-c", s.testCommand} + s.testCommand = "echo \"do nothing\"" + s.testCommandArray = []string{"bash", "-c", s.testCommand} s.expectedStdout = "stdout" s.expectedStderr = "stderr" s.apiMock = &apiQuerierMock{} @@ -684,19 +684,17 @@ func (s *ExecuteCommandTestSuite) TestWithSeparateStderr() { writer, ok := args.Get(5).(io.Writer) s.Require().True(ok) - s.Require().Equal(5, len(calledCommand)) - isStderrCommand, err := regexp.MatchString("mkfifo.*", calledCommand[4]) - s.Require().NoError(err) - if isStderrCommand { + if isStderrCommand := len(calledCommand) == 5; isStderrCommand { calledStderrCommand = calledCommand - _, err = writer.Write([]byte(s.expectedStderr)) + _, err := writer.Write([]byte(s.expectedStderr)) + s.Require().NoError(err) call.ReturnArguments = mock.Arguments{stderrExitCode, nil} } else { calledStdoutCommand = calledCommand - _, err = writer.Write([]byte(s.expectedStdout)) + _, err := writer.Write([]byte(s.expectedStdout)) + s.Require().NoError(err) call.ReturnArguments = mock.Arguments{commandExitCode, nil} } - s.Require().NoError(err) }) exitCode, err := s.nomadAPIClient.ExecuteCommand(s.allocationID, s.ctx, s.testCommandArray, withTTY, @@ -735,15 +733,11 @@ func (s *ExecuteCommandTestSuite) TestWithSeparateStderrReturnsCommandError() { calledCommand, ok := args.Get(2).([]string) s.Require().True(ok) - s.Require().Equal(5, len(calledCommand)) - isStderrCommand, err := regexp.MatchString("mkfifo.*", calledCommand[4]) - s.Require().NoError(err) - if isStderrCommand { + if isStderrCommand := len(calledCommand) == 5; isStderrCommand { call.ReturnArguments = mock.Arguments{1, nil} } else { call.ReturnArguments = mock.Arguments{1, tests.ErrDefault} } - s.Require().NoError(err) }) _, err := s.nomadAPIClient.ExecuteCommand(s.allocationID, s.ctx, s.testCommandArray, withTTY, UnprivilegedExecution, @@ -757,7 +751,7 @@ func (s *ExecuteCommandTestSuite) TestWithoutSeparateStderr() { commandExitCode := 42 // mock regular call - expectedCommand := setUserCommand(s.testCommandArray, UnprivilegedExecution) + expectedCommand := prepareCommandWithoutTTY(s.testCommandArray, UnprivilegedExecution) s.mockExecute(expectedCommand, commandExitCode, nil, func(args mock.Arguments) { stdout, ok := args.Get(5).(io.Writer) s.Require().True(ok) @@ -781,7 +775,7 @@ func (s *ExecuteCommandTestSuite) TestWithoutSeparateStderr() { func (s *ExecuteCommandTestSuite) TestWithoutSeparateStderrReturnsCommandError() { config.Config.Server.InteractiveStderr = false - expectedCommand := setUserCommand(s.testCommandArray, UnprivilegedExecution) + expectedCommand := prepareCommandWithoutTTY(s.testCommandArray, UnprivilegedExecution) s.mockExecute(expectedCommand, 1, tests.ErrDefault, func(args mock.Arguments) {}) _, err := s.nomadAPIClient.ExecuteCommand(s.allocationID, s.ctx, s.testCommandArray, withTTY, UnprivilegedExecution, nullio.Reader{}, io.Discard, io.Discard) diff --git a/internal/runner/aws_runner_test.go b/internal/runner/aws_runner_test.go index 9b04f52..99a8d94 100644 --- a/internal/runner/aws_runner_test.go +++ b/internal/runner/aws_runner_test.go @@ -95,7 +95,7 @@ func TestAWSFunctionWorkload_ExecuteInteractively(t *testing.T) { cancel() expectedRequestData := "{\"action\":\"" + environment.Image() + - "\",\"cmd\":[\"env\",\"CODEOCEAN=true\",\"sh\",\"-c\",\"unset \\\"${!AWS@}\\\" \\u0026\\u0026 " + command + + "\",\"cmd\":[\"env\",\"CODEOCEAN=true\",\"bash\",\"-c\",\"unset \\\"${!AWS@}\\\" \\u0026\\u0026 " + command + "\"],\"files\":{}}" assert.Equal(t, expectedRequestData, awsMock.receivedData) }) @@ -129,8 +129,8 @@ func TestAWSFunctionWorkload_UpdateFileSystem(t *testing.T) { execCancel() expectedRequestData := "{\"action\":\"" + environment.Image() + - "\",\"cmd\":[\"env\",\"CODEOCEAN=true\",\"sh\",\"-c\",\"unset \\\"${!AWS@}\\\" \\u0026\\u0026 " + command + "\"]," + - "\"files\":{\"" + string(myFile.Path) + "\":\"" + base64.StdEncoding.EncodeToString(myFile.Content) + "\"}}" + "\",\"cmd\":[\"env\",\"CODEOCEAN=true\",\"bash\",\"-c\",\"unset \\\"${!AWS@}\\\" \\u0026\\u0026 " + command + + "\"]," + "\"files\":{\"" + string(myFile.Path) + "\":\"" + base64.StdEncoding.EncodeToString(myFile.Content) + "\"}}" assert.Equal(t, expectedRequestData, awsMock.receivedData) } diff --git a/pkg/dto/dto.go b/pkg/dto/dto.go index 702d767..c062ef6 100644 --- a/pkg/dto/dto.go +++ b/pkg/dto/dto.go @@ -37,7 +37,7 @@ func (er *ExecutionRequest) FullCommand() []string { for variable, value := range er.Environment { command = append(command, fmt.Sprintf("%s=%s", variable, value)) } - command = append(command, "sh", "-c", er.Command) + command = append(command, "bash", "-c", er.Command) return command }