Review security of user-provided postStart shell interpolation
MR: Review security of postStart shell interpolation (!201017 - merged)
Description
The following discussion from !193265 (merged) should be addressed:
-
@gl-service-sec-app-sast-bot started a discussion: (+1 comment) We appear to be using string interpolation in a string that will be used as a shell command. Verify that the variables are sanitized. If youre not using interpolation, you should start your heredoc with
This automation belongs to AppSec. Use appsec-sasthelpful or appsec-sastunhelpful for quick feedback. To stop the bot from further commenting, you can use the appsec-saststop label. For any detailed feedback, add a comment here.<<~SHor<<-SH. @gitlab-com/gl-security/appsec please review this finding. Apply the appsec-sast-pingresolved label after reviewing.
Response from @cwoolley-gitlab:
This is a good point. Since these are currently all internal scripts, and not user-provided, and don't inject any user-provided values, we should be safe.
However, as we proceed with supporting user-provided scripts as part of Startup scripts for Remote Development workspac... (&15602) • Chad Woolley, Daniyal Arshad, we may need to be concerned about this. cc @daniyalAD
It should fundamentally be no different than existing functionality, such as users having inline shell scripts in their CI config files, which need to be subsequently processed.
Also, this may be a different case, because the execution context for the shell script is the workspace environment, not the Rails server environment.
In any case, lets make this a follow-up issue and add it to Startup scripts for Remote Development workspac... (&15602) • Chad Woolley, Daniyal Arshad so we can think it through carefully.
Acceptance criteria
-
Investigate and ensure that all shell interpolation is safe from a security perspective -
Also review other places that might not have been caught by this bot - e.g. using formatto inject values into shell scripts -
Add comments next to all places that the bot may flag in the future, to indicate that we have reviewed it for security.
Implementation plan
TODO: Fill out or delete (optional)
[Provide a high-level plan for implementation of this issue, including relevant technical and/or design details.]