Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bash shouldn't be assumed to be in /bin for portability #1534

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jgedarovich
Copy link

No description provided.

@pda
Copy link
Member

pda commented Mar 1, 2022

Howdy @jgedarovich 👋🏼
Sorry for taking a long time to reply.

Could you describe the problem you're encountering?

The bash scripts this PR changes can be categorised into:

  • .buildkite/* — CI build & release process.
  • *_test.go — some integration tests that write and execute temporary shell scripts.
  • install.sh— bash install script (designed to be piped into bash -c?)
  • packaging/docker/*/entrypoint.sh — docker entrypoints for specific base images
  • packaging/linux/root/usr/share/buildkite-agent/hooks/*.sample — sample hooks, could run on any system
  • scripts/*.sh — I believe these are primarily used in the CI build & release process

Of those, I can imagine these might have portability issues:

  • *_test.go — for running integration tests on machines without /bin/bash
  • …/hooks/*.sample — when installed on machines without /bin/bash, and then customised by the user without fixing the shebang.

While /bin/bash is less flexible/portable, the tradeoff of /usr/bin/env bash is susceptibility to $PATH manipulation; where an attacker could trick the agent or its operator into running a malicious bash command elsewhere on the system. Admittedly this seems like a far-fetched scenario, and control over $PATH would be bad anyway.

I'd be interested to hear if you're running into one of the described scenarios, or something I didn't think of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants