Review command validation

[tcompa]

No, as in my example you can still insert \n and it causes multiple commands to be executed. We can add \n and \r to the __NOT_ALLOWED_FOR_COMMANDS__ list but maybe there are still other ways to bypass that I can't see. I was thinking about adding stricter validation in the slurm_accounts field. Maybe a regex like r"^[A-Za-z0-9._-]{3,20}$"? Which are typical slurm_accounts values?

Originally posted by @zonia3000 in #3416 (comment)

We should review whether the current command-validation function is sufficiently robust - starting with some \r or \n payload.

EDIT: To be more precise, we should review this together with the actual use case. That is, we should check how it behaves e.g. in two environments: (1) the SSH run-command, a typical subprocess.run call (e.g. with shlex.split for the argument).

[zonia3000]

One thing that I find weird is that we are validating the whole command for not allowed characters. Usually one validates the parameters that come from untrusted sources, not the whole command. For example, when executing a SQL query using prepared statements, the system escapes the parameters, not the query itself. I would expect a similar approach here.

Think for example about a malicious input like foo --unexpected-flag=3. This is not arbitrary command execution, but it can still produce unwanted behavior. There is no way to prevent this by validating the whole command, unless we decide to limit the command syntax a lot. But one can prevent this by escaping the single untrusted parameters.

[tcompa]

I'll add a bit of context (and/or we can continue the discussion elsewhere).

1. Even if validate_cmd runs on the whole command, this is often redundant since we already validated the user-provided part of the command (in a similar or identical way).
   • We sometimes use SafeNonEmptyStr, see https://github.com/search?q=repo%3Afractal-analytics-platform%2Ffractal-server%20SafeNonEmptyStr&type=code. The context here is that e.g. project names end up being used when creating folders, and a project name like name; rm / would end up in mkdir name; rm /. Note that this same issue was also mitigated/fixed by sanitize_string (https://github.com/search?q=repo%3Afractal-analytics-platform%2Ffractal-server+sanitize_string&type=code), which we tend to use when creating paths based on unsafe variables.
   • Despite its name, validate_cmd is sometimes used with the same scope as SafeNonEmptyStr - e.g. here. That is, it is used to validate some user-provided bits of what will end up being part of a command (a pip install command, in this specific case). This fits more with your suggestion.
2. The same validate_cmd is also used for some of the task commands. In that case, the user is actually allowed to run an arbitrary command - but a typical task command will never e.g. include ;. In this context, the validation of the task command is rather a way to catch some likely malformed commands.
3. As for "Think for example about a malicious input like foo --unexpected-flag=3", there would be nothing wrong in rejecting kind of inputs. The complexity, in my opinion, is that it'd be a per-command validation and not a single centralized one like validate_cmd.

(the discussion is relevant, to be continued)