Add single quotes for inner command.

Change to bash as interpreter.
Forbid single quotes for user commands.

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:

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`

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 {

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) {

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.
+	command = prepareCommandWithoutTTY(command, privilegedExecution)
-		Execute(allocationID, ctx, setUserCommand(command, privilegedExecution), tty, stdin, stdout, stderr)
+	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, prepareCommandTTYStdErr(currentNanoTime, privilegedExecution), true,
-		exit, err := a.Execute(allocationID, ctx, stdErrCommand, 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".
+	// 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"
+	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 {
+func prepareCommandWithoutTTY(commands []string, privilegedExecution bool) []string {
-	command := strings.Join(commands, " ")
+	commands = setUserCommand(commands, privilegedExecution)
-	return []string{"sh", "-c", fmt.Sprintf(unsetEnvironmentVariablesFormat, unsetEnvironmentVariablesShell, command)}
+	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)
 }

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.testCommand = "echo \"do nothing\""
-	s.testCommandArray = []string{"sh", "-c", s.testCommand}
+	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))
+		if isStderrCommand := len(calledCommand) == 5; isStderrCommand {
-		isStderrCommand, err := regexp.MatchString("mkfifo.*", calledCommand[4])
-		s.Require().NoError(err)
-		if 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))
+		if isStderrCommand := len(calledCommand) == 5; isStderrCommand {
-		isStderrCommand, err := regexp.MatchString("mkfifo.*", calledCommand[4])
-		s.Require().NoError(err)
-		if 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)

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 + "\"]," +
+		"\",\"cmd\":[\"env\",\"CODEOCEAN=true\",\"bash\",\"-c\",\"unset \\\"${!AWS@}\\\" \\u0026\\u0026 " + command +
-		"\"files\":{\"" + string(myFile.Path) + "\":\"" + base64.StdEncoding.EncodeToString(myFile.Content) + "\"}}"
+		"\"]," + "\"files\":{\"" + string(myFile.Path) + "\":\"" + base64.StdEncoding.EncodeToString(myFile.Content) + "\"}}"
 	assert.Equal(t, expectedRequestData, awsMock.receivedData)
 }
 

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
 }