zsh-autoswitch-virtualenv
zsh-autoswitch-virtualenv copied to clipboard
File permission checks problematic for pipenv projects
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 forcepipenv
to use that instead - Override the activation script in the existing
virtualenv
thatpipenv
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.
Related is I think #149
But I agree, I keep hitting this and its rather irritating! :-)
I would like to add the following:
-
umask
for non-root users is664
be default, meaning the point I made about having tochmod
after every checkout is not void unlike previously described. - I did see #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.) - The fact that people have to
chmod
after each checkout will eventually turn it into muscle memory and people will start doingchmod
without thinking, even in cases where security is implicated.
I will update the wording in OP a little.
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.
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 :)
part of the reason why this is not working correctly is that the "lenient" permission matching is not fully correct here: https://github.com/MichaelAquilina/zsh-autoswitch-virtualenv/blob/ecc53e3b01f8553a78a890f35b90f94a761c2156/autoswitch_virtualenv.plugin.zsh#L187
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?
@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.
(Oops, created the PR wrong originally. Added correct PR.)