-
Notifications
You must be signed in to change notification settings - Fork 916
Setting a relative path causes path resolution problems with dkms and potentially opens up a security issue. #481
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
Conversation
potentially opens up a security issue. ==> dkms install --no-depmod zenpower3/0.2.0 -k 6.15.9-zen1-1-zen ./bin/dkms: line 194: ./bin/date: No such file or directory ./bin/dkms: line 1462: ./bin/mkdir: No such file or directory mktemp: failed to create directory via template ‘/var/lib/dkms/zenpower3/0.2.0/6.15.9-zen1-1-zen/.tmp_x86_64_XXXXXX’: No such file or directory Error! Unable to make temporary directory. ./bin/dkms: line 1464: ./bin/mkdir: No such file or directory ./bin/dkms: line 1465: ./bin/cp: No such file or directory ./bin/dkms: line 1468: ./bin/mkdir: No such file or directory ./bin/dkms: line 1470: ./bin/cp: No such file or directory ./bin/dkms: line 1512: ./bin/cp: No such file or directory
It's by design, though. App development often has a bin/ file inside their tree to run app-specific bins. I think we need to fix the dkms a different way. |
Maybe having it as the last entry in PATH would work then? I think it's potentially dangerous to allow arbitrary paths to override system binaries. |
We need it to be able to overwrite, say, the system rails or bundle command inside a given app directly. But maybe there's a way to just have the core utils appear first? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR; I'd recommend we drop this from the path altogether and include it via Mise config instead to achieve the desired behavior.
I don't think the issues you're encountering here @precision are actually related to this. I just tried and successfully installed zenpower3-dkms
with the default $PATH
setup.
Interestingly enough, I did find this issue void-linux/void-packages#56044 in another project that sounds oddly similar. Doesn't explain exactly why I could install fine and you're getting the relative path errors.
However; on the broader issue of having ./bin in the path @dhh; I think we're doing it wrong currently.
I see, and agree exactly with what we're attempting to do but in practice, it's not doing what we want.
Since we have Mise as a core piece of the puzzle, it inserts its path before ours, and kind of ruins the plan.
echo $PATH
/home/ryan/.local/share/mise/installs/ruby/3.3.5/bin:/home/ryan/.local/share/mise/installs/bun/1.2.18/bin:/home/ryan/.local/share/mise/installs/node/24.3.0/bin:./bin:/home/ryan/.local/bin:/home/ryan/.local/share/omarchy/bin:/usr/local/sbin:/usr/local/bin:/usr/bin
This means that in my project, the ./bin
actually isn't doing me any favors except for bin files like jobs
which don't exist upstream.
pwd
/home/ryan/Dev/nebula
which rails
/home/ryan/.local/share/mise/installs/ruby/3.3.5/bin/rails
which bundle
/home/ryan/.local/share/mise/installs/ruby/3.3.5/bin/bundle
which jobs
/home/ryan/Dev/nebula/bin/jobs
A better approach to this would be to add this to something like ~/.config/mise.toml
or one of the other global variants.
[env]
_.path = "{{ cwd }}/bin"
With that in place, in the same project, I get the desired behavior.
pwd
/home/ryan/Dev/nebula
which rails
/home/ryan/Dev/nebula/bin/rails
which bundle
/home/ryan/Dev/nebula/bin/bundle
which jobs
/home/ryan/Dev/nebula/bin/jobs
Ah yes, I see what we have now is broken. It only uses ./bin if the bin isn't available in mise. Happy to take a PR to correct this via mise. I can't imagine needing this in any non-mise managed projects 👌 |
If we do it this route, it applies to every directory. The better / safer way to accomplish the same thing is to set this in a For me, I typically have all of my dev projects in With that in mind; do we want to apply globally or somehow just provide guidance or tooling to set this for a user based on where they keep their projects? Or third, I suppose is just document it in the manual but that's less fun. 🤣 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an obvious change that eliminates an important, critical security issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really appreciate you taking this security concern seriously. With all the buzz around omarchy, this should get fixed before it bites some of the many new linux users who are jumping into linux thanks to omarchy.
This will be handled via #1654 |
I didn't test if this causes issues with the install, but I tracked this down while debugging why DKMS has been failing to install modules.
Using a leading relative path could potentially lead to unintended consequences if someone were to say send you a tarball with a
bin/ls
that justrm -fr /
.