Pinned Build Script Linter Changes #812
Conversation
| @@ -1,140 +1,139 @@ | |||
| #!/usr/bin/env bash | |||
| #!/bin/bash | |||
| set -eo pipefail | |||
There was a problem hiding this comment.
I'm not sure I correctly understand the purpose of this change, at least from the description in the commit:
Looking for BASH in the environment instead of invoking it directly enables attacks where malware could escalate privileges by pointing BASH to itself and tricking the user into entering a superuser password when a user invokes our script, a familiar request the user may be expecting.
This seems to be saying we want to guard our script from being compromised by a malicious application that is in the user's PATH that matches the name as an application the script uses. If that's really the case, then all executables need to have an absolute path. apt-get, cmake, gcc, clang, tar, etc etc because any of those could be something malicious.
The practicality of trying to enforce this for the pinned_build.sh script seems tough. Maybe it's more doable in install_deps.sh, and maybe it's more pertinent in install_deps.sh since that one needs to be run as root and/or might be more expected to prompt a password.
There was a problem hiding this comment.
I agree with everything you wrote and I am certainly not advocating that we call other binaries in the script (such as aptitude) directly instead of relying on the system PATH. This shebang convention is something I researched many years ago and stuck to, so my explanation in the commit message as to why probably leaves a lot to be desired.
I looked it up again and found that, while the PATH exploit is a potential concern, the env binary itself is a bigger concern and it has been the subject of high-profile exploits in the past.
Yes, there have been examples of malware that have exploited the use of the
envcommand to execute malicious code.One such example is the "Shellshock" vulnerability, which was a series of security flaws discovered in the BASH shell in 2014. One of the exploits of the Shellshock vulnerability involved setting the
BASH_ENVenvironment variable to a malicious script that was executed when a new BASH shell was launched. Attackers were able to use this exploit to execute arbitrary code on vulnerable systems.Another example is the "Dvmap" malware, which was discovered in 2017 and targeted Android devices. The malware exploited a vulnerability in the env command to install itself as a root application, giving attackers full control over the compromised device.
In general, any command or utility that is used to execute code on a system can be vulnerable to exploits, including the
envcommand. It's important to ensure that your system is up to date with the latest security patches and to follow best practices for securing your environment to minimize the risk of exploitation.
Another disadvantage to using env is that you cannot pass more than one argument to the binary you are looking for on some systems. Of course, we are not trying to pass arguments here.
People also argue for its use.
I do not feel strongly enough to spend a ton of time on this, I will revert the change if you want. But, in general because of the security considerations and the fact that it has been exploited in the real world, I personally prefer to avoid using env in shebangs when possible.
There was a problem hiding this comment.
imo it doesn't really do much for security. The rest of the applications the script runs (directly or indirectly) will suffer from the same problem. Plus, if you're to the point where you're worried about PATH being compromised what about LD_PRELOAD or others?
The script becomes less portable (bash on freebsd is in /usr/local/bin, for example) while not really gaining anything. But, portability is somewhat academic for this only-supported-on-ubuntu script anyways.
… written to stdout/stderr.
…eserve whitespace/symbols (or eval as string).
Looking for BASH in the environment instead of invoking it directly enables attacks where malware could escalate privileges by pointing BASH to itself and tricking the user into entering a superuser password when a user invokes our script, a familiar request the user may be expecting.
From issue 735, I was having trouble running
scripts/pinned_build.shin a clean Ubuntu 22.04 container on theHEADofmain, which is expected to work, so I ran the bashate and shellcheck BASH linters on the script to look for errors.This pull request is only linting changes, I have introduced them in a pull request separate from my work to try to make the peer review less painful.
This will probably be easiest to review by ignoring whitespace...
...and/or reviewing the commits individually, instead of as a group. The commits each have one linter rule in them they intend to address, which you can find online here for
bashate(the errors beginning with 'E'), or here for ShellCheck.See Also
Pull Request 832 - Rollinstall_deps.shintopinned_build.shlibcurl4-openssl-devREADME.mdFormatting