8000 Unsafe Installation · Issue #94 · Meliox/PVE-mods · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Unsafe Installation #94

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.

Sign up for GitHub

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

Jump to bottom
Open
dolfs opened this issue Apr 6, 2025 · 4 comments
Open

Unsafe Installation #94

dolfs opened this issue Apr 6, 2025 · 4 comments
Labels
bug Something isn't working

Comments

@dolfs
Copy link
dolfs commented Apr 6, 2025

Describe the bug
I am running PVE 8.3.5 and did not notice the script was for 8.3.4. I incorrectly attempted to run the mod script:

$ bash <(curl -fsSL https://raw.githubusercontent.com/Meliox/PVE-mods/refs/heads/main/pve-mod-gui-sensors.sh) install

This produced an error partway through the script (text was unfortunately lost). I figured out the correct way to run it:

$ bash <(curl -fsSL https://raw.githubusercontent.com/Meliox/PVE-mods/refs/heads/main/pve-mod-gui-sensors.sh) install

When I did that, I got the message that things were already installed. I was never asked for installation options, and there were no backup files. I suspect the result of the first attempt.

the steps to reproduce also describe how backup files are not created and how the script does not check if they were successfully created. This should be tested before proceeding to make changes.

To Reproduce
Steps to reproduce the behavior:

  1. bash <(curl -fsSL https://raw.githubusercontent.com/Meliox/PVE-mods/refs/heads/main/pve-mod-gui-sensors.sh) install
  2. bash <(curl -fsSL https://raw.githubusercontent.com/Meliox/PVE-mods/refs/heads/main/pve-mod-gui-sensors.sh) install
  3. Get message mod already installed (when it is not)
  4. Manually edit Nodes.pm to remove the added stuff (just a few lines)
  5. Run step 2 again, this time answering prompts
  6. Get a message: Backup of "/usr/share/perl5/PVE/API2/Nodes.pm" saved to "/proc/123649/fd/backup/Nodes.pm.2025-04-05_22-11-56" followed by cp: cannot create regular file '/proc/123649/fd/backup/pvemanagerlib.js.2025-04-05_22-11-56': No such file or directory. Ditto for pvemanagerlib.js
  7. Nonetheless, the files were modified.

Expected behavior

  1. Document the above way to execute the script in one go, without downloading to a file first
  2. Do not attempt tp use /proc/<numbers> as backup-dir
  3. Verify backups made and identical to the original (SHA) before proceeding with mods

Screenshots
If applicable, add screenshots to help explain your problem.

Sensors output
If applicable, provide sensor datadump (use the argument save-sensors-data)

Additional context
Add any other context about the problem here.
@dolfs dolfs added the bug Something isn't working label Apr 6, 2025
@Meliox Meliox mentioned this issue Apr 6, 2025
Merged
@Meliox
Copy link
Owner
Meliox commented Apr 6, 2025 •
edited
Loading

Hi,
Thanks for the report! The listed version is just versions that have been tested - newer may work. They have been so for the past 2 years

  1. Is implemented with Verify backup files are created and identifical implements part of (#94) #97.

1+2) There is configuration in the script, which does include the possibility to modify the path to the backup directory. Given this I do not think it will be a good idea to support that approach. Currently it is using the path relative to where you execute the script from. Additionally safety-wise, it is the responsibility of the user to verify what is being downloaded and executed (code is open-source, so all can review, but still).

Meliox added a commit that referenced this issue Apr 6, 2025
@Meliox
Verify backup files are created and identifical implements part of (#94)
51ab839 
Co-authored-by: Meliox <na>
@dolfs
Copy link
Author
dolfs commented Apr 6, 2025
A136

I suppose we were both dealing with 8.3.5 at more or less the same time. I made some changes to address the issues I mention, but do not have time to fork and create pull request. Instead, patch file attached.

  • Creates an alternative BACKUP_DIR if necessary and checks before anything else happens
  • Produces errors if the creation of backup copies fails
  • Produces errors if the hashes don't match
  • Changed two error messages to be slightly easier to understand

patches.patch

@dolfs
Copy link
Author
dolfs commented Apr 6, 2025

I agree that if one downloads and inspects the script, one can modify the definition of the BACKUP_DIR. One should perhaps verify what they are about to execute. However, many scripts available these days do provide the option of one-line execution to activate them. This is particularly attractive to those who lack the skills to inspect and understand code but still want to use it.

For one, I always look at the code on Git Hub, but I prefer the one-line execution so no files remain behind on the machine where the installation happens. To that end, I used the second form of execution in my original report. That then reveals:

  • For the script used that way, the current working directory is on the proc file system, resulting in a non-functional BACKUP_DIR. Hence, my patch is to at least switch to an alternate directory.
  • Unless you absolutely do not want to support the one-liner approach (which stops nobody from doing it your current way), the patch supplied basically implements all these safety measures.

@Meliox
Copy link
Owner
Meliox commented Apr 8, 2025

Thanks. Will have a look and see if further improvements van be implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dolfs @Meliox
0