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

Fixes Linux shebangs #6110

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Fixes Linux shebangs #6110

wants to merge 3 commits into from

Conversation

LuNeder
Copy link

@LuNeder LuNeder commented Jun 9, 2024

The linux scripts were using #!/bin/bash, which causes problems in some Linux distributions (such as NixOS) because that file doesn't exist on that place. #!/usr/bin/env bash should used instead, in order to get bash from the correct place from the user's PATH variable.

I've tested and can confirm that these changes are enough to make the software work on NixOS.

Changed files

  • cmd_linux.sh
  • start_linux.sh

Checklist:

@polarathene
Copy link

Probably not relevant, but there are some caveats to using env for shebang, there's also the security concern that delegating resolution to the PATH env has potential for abuse by malware.

Is /usr/bin/bash not valid in the scenarios where you find /bin/bash invalid? How many environments with /bin/bash aren't likely to have /usr/bin/bash? (especially since you're now relying on /usr/bin for env?)

@LuNeder
Copy link
Author

LuNeder commented Jun 25, 2024

Is /usr/bin/bash not valid in the scenarios where you find /bin/bash invalid? How many environments with /bin/bash aren't likely to have /usr/bin/bash?

/usr/bin/bash is not valid for NixOS.

This is /bin and /usr/bin on NixOS:

ls /usr/bin /bin                     
/bin:
sh

/usr/bin:
env

/usr/bin/bash will also probably not be valid for most systems which don't have /bin linkled to /usr/bin

If no bash-specific features are used on those scripts, an even better option would be to use sh instead. Not all linux distros even have bash, tho I'm yet to see a distro without /bin/sh.

@polarathene
Copy link

/usr/bin/bash is not valid for NixOS.

NixOS is quite a bit different, especially when you're stating it only has sh and env available in separate locations 🤷‍♂️

/usr/bin/env apparently isn't always present either from what I had read, so I don't think you're going to have much luck choosing a shebang that is always going to work.

Maintainers may be less likely to consider merging this since someone else may raise a PR to do the same when /usr/bin/env doesn't work for them. The demand is likely low and niche, with better alternatives? Perhaps /bin/sh is viable, I haven't looked over the scripts themselves to check.

I still think it's not ideal to choose /usr/bin/env given the typical user base of the project and the security risk that brings? It wouldn't look good for the project if there was a compromise related to that, with the justification for enabling it being to support NixOS niche requirement (only one user requesting it).


Perhaps it might be better to use your experience with NixOS to have a nix package that makes the necessary adjustments? That can satisfy the current shebang expectation, or it could alter the shebang for these files.

Without broader demand or a wider audience outside of NixOS, the value of upstreaming the change is questionable.

@LuNeder
Copy link
Author

LuNeder commented Jun 26, 2024

I don’t see how using env could cause any malware problems. If env selects a malicious bash, the user already is compromised when running the script anyway. If the user has a bad bash in their environment, it would have already been triggered no matter what oobabooga did.

Aditionally, even the link you sent yourself, that mentions this “problem” with env states that using env is still a better option regardless:

In general, use env unless you know you are writing in one of these environments that scrutinize the minute details of risk.

You also mentioned not all distros have /usr/bin/env, but I’ve never seen a distro that didn’t have it. Do you have any examples at all?

@mbrla0
Copy link

mbrla0 commented Jun 26, 2024

[...] there's also the security concern that delegating resolution to the PATH env has potential for abuse by malware.

If you don't mind me asking, in which scenarios do you think that could become an actual concern for this project?

@polarathene
Copy link

polarathene commented Jun 26, 2024

in which scenarios do you think that could become an actual concern for this project?

Those where an attacker for some reason has the ability to have precedence in the PATH ENV?

It's perhaps unlikely that they're able to introduce only that as a change to the system if they had that sort of control... but what is more likely is the user is fooled into installing something that has them (or the environment itself) update/prepend to PATH. There was a recent security issue with ComfyUI if you're familiar with that, a custom module/plugin was malicious but had users downloading it unaware.

Environments that you run software in will vary too, such as with a Docker container, environment can be managed differently there. This project appears to have had an official image / Dockerfile for over a year, whereas with ComfyUI there is none last I checked. Users may choose something that seems legitimate and trust that. That can still happen for this project too though, I wouldn't be surprised 😅


Regardless, it's still an increase to attack surface by relying on env to resolve via PATH vs being more strict/specific.

@polarathene
Copy link

polarathene commented Jun 26, 2024

Sorry didn't see your reply.

If env selects a malicious bash, the user already is compromised when running the script anyway.

No? The bash could be anywhere on the system, it'd only be run if you provided direct path to it, or relied on env to get it via PATH environment variable? That's why env would be exploited for malicious use more easily than a script that wants to run bash using an explicit path.

Just to be clear, the script itself is not malicious. It's the malicious bash in the PATH env waiting to hijack when called.

If the user has a bad bash in their environment, it would have already been triggered no matter what oobabooga did.

It depends on the system and how bash is run outside of this context for any other avenues of attack.

I'm not saying that in this case the user isn't already in trouble regardless. Just that the proposed change introduces that added risk for the small sake of convenience to satisfy niche demand.

Many of the security exploits / CVEs that get spoken about these days require conditions of compromise already in place.


Aditionally, even the link you sent yourself, that mentions this “problem” with env states that using env is still a better option regardless:

In general, use env unless you know you are writing in one of these environments that scrutinize the minute details of risk.

This isn't a decision that an individual is doing on their system. It's a decision you want to contribute that affects a broad audience of users that aren't likely to notice that change. All for the benefit of satisfying a much smaller set of users at their expense.

The change doesn't seem necessary for the majority, and the drawback with reluctance has been communicated clearly I hope.


You also mentioned not all distros have /usr/bin/env, but I’ve never seen a distro that didn’t have it. Do you have any examples at all?

So nothing that great example wise. I assume pragmatically most linux distros are going to have bash at the expected location. NixOS differs from a typical distro, and probably should resolve the concern with nix tbh 🤷‍♂️

@polarathene
Copy link

polarathene commented Jun 30, 2024

TL;DR: Despite concerns raised being valid, it seems like /usr/bin/env bash is worthwhile with minimum drawback.

  • 👍 Portability concerns don't seem to regress, and those identified are very niche. Although this change may only really benefit NixOS users using git clone instead of a dedicated maintained package?
  • 👍 Security concerns seem mostly redundant given the context of attack vectors?

Apologies for the noise I introduced.


I understand you don't like my feedback with the 👎 but that doesn't dismiss the validity for why this PR has more disadvantages than benefits:

  • No issue referenced:
  • NixOS is quite different from other Linux distros:
    • Not FHS compliant?
    • As shown at the end of the references, /usr/bin/env isn't always available when using nix. Some users instead patch such shebangs or use other workarounds on their end, like was suggested in my prior feedback. Thus you're likely to swap one problem for another?
  • The change claims to be motivated by improved portability:
    • The actual improvement to portability in this case is unclear. It seems limited to a niche set of users and as noted will likely inconvenience others as a regression (ironically other NixOS users?).
    • Increases attack surface, reducing security in favor of convenience for assumed broader compatibility.
      • While exploiting this may be low, it cannot be denied that changing from absolute path to implicit via env is more secure.
      • Especially concerning to introduce to a broader userbase when weighing the pros/cons for the change.
      • EDIT: A scenario where PATH is altered in a way for an attack here may be argued that any command could then be exploited with an executable at that non-standard location (eg: ls / cat).
        • In that sense /usr/bin/env bash isn't really going to make much difference, however sometimes the environment may not rely upon a user inputting commands but is automated with absolute paths (deployed server, docker image, etc).
        • If the environment was that controlled, they'd not likely be a need for a modified PATH, thus the attacker has nothing to leverage.

So handwave away these concerns all you like the 👎 , but this PR is motivated for your benefit at the expense of others (given the above points).

UPDATE:

  • I engaged with the NixOS community and have since become more familiar with the sandbox concern. When you can't rely on /usr/bin/env (such as with sandbox for building a package), you'd rely on the patch-shebangs.sh hook. AFAIK that's valid regardless of the scripts shebang, but is more effort (you have to package the project, or rely on someone else who has).
  • Regarding the /usr/bin/env bash security concerns, I've edited an update above as I can't really think of a valid scenario where /usr/bin/env usage realistically contributes to the exploit (beyond targeting a common command to resolve).

References (remainder of this comment)

One of my initial feedback questions:

Is /usr/bin/bash not valid in the scenarios where you find /bin/bash invalid? How many environments with /bin/bash aren't likely to have /usr/bin/bash?

Assuming bash is required, this query has only received a response that NixOS isn't compatible with the absolute path and thus uses /usr/bin/env bash as a way to resolve that.

The most common reason elsewhere for this requirement is like with macOS shipping an old bash at the absolute path and wanting to run a script with a different install of bash provided elsewhere. Not sure how likely that'd be relevant in this context with start_linux.sh though.

Portability / Security

https://tecadmin.net/choose-the-best-shebang-for-your-shell-scripts/

image

image


https://unix.stackexchange.com/questions/206350/what-is-the-difference-if-i-start-bash-with-bin-bash-or-usr-bin-env-bash/206366#206366

image


https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard#FHS_compliance

Portability / Compatibility:

image


https://en.wikipedia.org/wiki/Shebang_(Unix)#Program_location

Portability / Compatibility:

image

Security:

image


https://burnthewhich.github.io/shbangenv/shbangenv

Portability / Compatibility:

image

Security:

image

image


Packaging (including Nix issues)

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shebang_lines

image

https://www.debian.org/doc/packaging-manuals/python-policy/#interpreter-location

image

NixOS/nix#1205

image

image

image

image

image

NixOS/nixpkgs#6227 (comment)

image

image

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.

None yet

3 participants