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.

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

File permission checks problematic for pipenv projects #152

Open
derula opened this issue Apr 5, 2021 · 7 comments · May be fixed by #163
Open

File permission checks problematic for pipenv projects #152

derula opened this issue Apr 5, 2021 · 7 comments · May be fixed by #163
Labels
bug

Comments

@derula
Copy link

derula commented Apr 5, 2021 •
edited
Loading

Introduction

First of all, thanks a lot for this, it's a great tool and very helpful for me!

Currently, when using pipenv, it refuses to switch virtualenv if the Pipfile's file permissions are "too weak," which I think serves little purpose. Worse, since Pipfile is customarily committed to version control, and e.g. git always checks out files with umask, which is 664 by default, pipenv users will have to chmod the Pipfile again after each checkout, which is annoying and potentially dangerous (see below).

How to reproduce

❯ mkdir test
❯ cd test
❯ chmod 666 Pipfile >> Pipfile
❯ cd .

Analysis

As far as I can tell, the security issue being addressed with this check was that an attacker could create a new virtualenv with a harmful activation script and then change the .venv file to point to that virtualenv. With pipenv, the path of the virtualenv is determined by a fixed set of rules, including a SHA256 hash of the Pipfile location and whether a .venv file or folder exists in the project directory. This leaves an attacker with two choices:

  • Place a .venv file / folder in the project directory to force pipenv to use that instead
  • Override the activation script in the existing virtualenv that pipenv would use

Neither scenario can be protected against with the permission check on the Pipfile. And fortunately, if a .venv file is present, the script already uses it as the file to check instead of Pipfile.

There is another, potentially dangerous side effect. If people get used to just doing chmod after each checkout, eventually they will become complacent and just do it automatically without thinking (because that's just what you need to to to get it to work). This is a problem, because the general security concern with .venv still exists. Now, if there actually is a .venv file with weak permissions, users may just automatically copy and run the chmod command as they're used to doing routinely, missing that this time the file name is .venv and not Pipfile. The .venv file could have been written to by a malicious actor, but the user would not watch out for this because they did not think that file existed in the first place.

Speculation

Does checking the Pipfile serve any purpose I'm not aware of, or does it simply happen "on accident" because of the way the script handles different package managers? If it does solve a legitimate purpose, checking only the Pipfile might be a security issue, because an attacker could easily modify the Pipfile.lock instead, completely circumventing the check.

Conclusion

Once again, thank you for this useful little thing! I'm grateful for the continued support and that you are looking out for security pitfalls as well! I hope my feedback is helpful to the project.
@rnc
Copy link
Contributor

rnc commented Apr 6, 2021

Related is I think #149

But I agree, I keep hitting this and its rather irritating! :-)

@derula
Copy link
Author

derula commented Apr 6, 2021

I would like to add the following:

  1. umask for non-root users is 664 be default, meaning the point I made about having to chmod after every checkout is not void unlike previously described.
  2. I did see Add option to disable security warnings #149, but while just disabling the check in general removes the "annoyance" factor, it will also bring back the actual security problem (because pipenv will also respect a .venv file.)
  3. The fact that people have to chmod after each checkout will eventually turn it into muscle memory and people will start doing chmod without thinking, even in cases where security is implicated.

I will update the wording in OP a little.

@MichaelAquilina
Copy link
Owner

I actually am also tempted to remove this feature from the plugin. It was something I added early on when I thought it might be useful - but as some others have pointed out there is not much benefit for the complexity it requires to run.

@derula
Copy link
Author

derula commented Sep 20, 2021

I think in general this security feature is sensible. I think the problem in the pipenv case is that it's barking at the wrong tree. Would it make more sense to check the result of pipenv --venv instead of the Pipfile itself?

After a recent change, it seems like it re-evaluates the file attributes even if the PWD didn't change. This makes this problem even more annoying, because every time I check out a different branch (where the Pipfile is different), it now kicks me out of the previously active virtualenv and displays the security warning. Maybe this change was by mistake (and maybe I shouldn't be using master anyway), it just made me think about this ticket again :)

@MichaelAquilina
Copy link
Owner

MichaelAquilina commented Oct 7, 2021 •
edited
Loading

part of the reason why this is not working correctly is that the "lenient" permission matching is not fully correct here:

zsh-autoswitch-virtualenv/autoswitch_virtualenv.plugin.zsh

Line 187 in ecc53e3

elif ! [[ "$file_permissions" =~ ^[64][04][04]$ ]]; then

Would it make more sense to check the result of pipenv --venv instead of the Pipfile itself?

The problem with this is that running pipenv is very slow. So it would result in very long times when switching directories.

I think a better fix to the problem is either remove this security warning or fix the line I've pointed to :)

Would you perhaps like to give it a go?

derula added a commit to derula/zsh-autoswitch-virtualenv that referenced this issue Oct 7, 2021
@derula
Make permission check more lenient.
1590ac6 
Instead of enforcing an arbitrary permission policy, check permissions against  current umask.

If permissions are changed from the default, this might be a hint that something shady is going on.

Resolves MichaelAquilina#152.
@derula derula mentioned this issue Oct 7, 2021
Closed
@derula
Copy link
Author

derula commented Oct 7, 2021 •
edited
Loading

@MichaelAquilina here's my take. Code may be a bit wonky and I haven't actually tested it, please let me know if it's going in the right direction though. I could potentially test it, but that would be a bit fiddly because I my experience with ZSH plugins is limited to installing them through a plugin manager. So it would be much appreciated if you could test it.

Basically the idea here is to ensure the file permissions match what umask dictates instead of choosing an arbitrary permission policy ourselves.

Could potentially be improved to allow default permissions or stronger, but I wanted to keep it simple to convey the idea better.

Since git obeys umask on checkout, my change should solve this specific issue. I am not sure, but I think this same check is also reasonable for other cases? Let me know if you disagree.

@derula derula linked a pull request Oct 7, 2021 that will close this issue
Open
@derula
Copy link
Author

derula commented Oct 7, 2021

(Oops, created the PR wrong originally. Added correct PR.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
3 participants
@derula @rnc @MichaelAquilina