-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
Remove use of Environment Python #904
base: beta
Are you sure you want to change the base?
Conversation
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.
You're absolutely right, although it would be better to allow the packages' maintainers to specify the path because it isn't always guaranteed to be in /usr/bin.
Apologies I have not had a chance to review and test your changes, my laptop died last Monday and I'm still out of a laptop. I'd say that just on a skim your changes look good 👍🏾 |
Thanks @musikid Co-authored-by: Sayafdine Said <[email protected]>
Hi, my laptop is still dead and ASUS want to charge me £1200 so I'll be out of a laptop a bit longer. I'm making this PR from a backup PC, but I don't have any cameras, so whether this works is uncertain, but it compiles and I used grep to check the strings have been correctly replaced. I HIGHLY ADVISE SOMEONE WHO CAN TEST IT PLEASE TEST IT. Otherwise I think this is ready to merge in. EDIT: Also I use Arch (btw), so I have tested the makepkg and that works fine, but I haven't tested the Debian or Fedora package |
Anyone been able to test this? (My laptop is still dead) |
Don't think this is super important nor does it fix the actual vulnerability. But the reasoning is you don't have login nor file permissions to do so. More important is fixing the externally_managed_environment error (and not by removing or renaming EXTERNALLY-MANAGED file but having the right packages for your python code in apt available to install without getting that error). |
My argument is mainly that calling a local environment version of python is susceptible in a scenario where for example a user has sudo privileges and a malicious program in user space with user privileges could point to a malicious user-owned python3 executable. This exploit would already require user privileges, but could be an escalation of privileges. It would also fix the issue where people with user-mode python shims or similar can't use Howdy and enable maintainers to better pin Howdy to a specific python and associated dependencies that is also packaged by them. I will say I am not an official maintainer anywhere, so I wouldn't know if this is a huge issue, but I often see tickets about it when I scroll through. |
Good point. However in current state you have to take a risk to mess up your main python dependencies due to that externally-managed error you get during the apt install (just pointing that this was the main reason for me not to use howdy yet on my own machine). And security wise I'd like to see some more parts converted to c or have some more binaries involved that are less easy to tamper. Same also applies to all python scripts and installed packages for howdy: they must be chowned/chmodded just right to make sure your same argument about the ENV here does not apply to a python source file used by howdy during login. |
This is indeed an issue and package maintainers should patch it (which it is in my Fedora copr repo).
Should be fine for any distro! |
|
||
datadir = get_option('prefix') / get_option('datadir') / 'howdy' | ||
py_conf = configuration_data(paths_dict) | ||
py_conf.set('data_dir', datadir) | ||
|
||
py = import('python').find_installation(paths_dict.get('python_path')) |
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.
Not sure about this, python_path could be another path that the packager would like to set?
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.
Except for the above, it should good.
Hi,
I was running this on Arch Kernel 6.8.2 and I recently moved from the howdy AUR package to the howdy-beta-git package.
I use Pyenv to manage multiple python versions and howdy seems to always want to use the environment python, which I believe is incorrect behavior, especially from a security standpoint wherein (this example is admittedly quite stupid, but I hope it tracks) a malicious program could launch a subprocess with sudo that uses howdy and a modified environment to launch an executable of the attackers choice from the PAM context as long as it is named python.
This patch removes the use of the "!/usr/bin/env python3" and replaces it with "!/usr/bin/python3" and also changes the python3 executable to "/usr/bin/python3" in "compare.py".
I have patched and installed this on my local system and it seems to work fine.
I hope this helps!