Skip to content

Conversation

felixxm
Copy link
Member

@felixxm felixxm commented Aug 27, 2020

This caused permission errors when user didn't have permissions to all parents directories of Django installation path.

Thanks tytusd and leonyxz for reports.

Regression in edeec12 and 26554cf.

ticket-31912

@felixxm felixxm requested review from a team and claudep August 27, 2020 07:07
@MarkusH
Copy link
Member

MarkusH commented Aug 27, 2020

I don't understand how this this can even occur. How can one have access to /some/path/to/a/python3.6/site-packages/django/contrib/auth/password_validation.py but not /some/path/to/a/python3.6/site-packages/django/contrib/auth/common-passwords.txt.gz. Because access to the former must exist, otherwise the user wouldn't be able to run that code.

@claudep
Copy link
Member

claudep commented Aug 27, 2020

@MarkusH, in your example, the problem is for example that you don't have access to /some/, but you have access to /some/path/ and below. This is a weird configuration, I admit, but still possible.

@MarkusH
Copy link
Member

MarkusH commented Aug 27, 2020

@MarkusH, in your example, the problem is for example that you don't have access to /some/, but you have access to /some/path/ and below. This is a weird configuration, I admit, but still possible.

Yes, I get it. But how do I get to such a config?

$ sudo ls -laR /tmp/foo
/tmp/foo:
total 0
drwx------  3 root   root     60 Aug 27 09:43 .
drwxrwxrwt 29 root   root   1340 Aug 27 09:44 ..
drwxr-xr-x  2 markus markus   60 Aug 27 09:44 bar

/tmp/foo/bar:
total 4
drwxr-xr-x 2 markus markus 60 Aug 27 09:44 .
drwx------ 3 root   root   60 Aug 27 09:43 ..
-rw-r--r-- 1 markus markus 12 Aug 27 09:44 buz.txt
$ sudo cat /tmp/foo/bar/buz.txt
hello world
$ ls -laR /tmp/foo 
ls: cannot open directory '/tmp/foo': Permission denied
$ cat /tmp/foo/bar/buz.txt     
cat: /tmp/foo/bar/buz.txt: Permission denied

@adamchainz
Copy link
Member

@MarkusH it's possible with a parent directory that's owned by root, and has executable flag set for the group but not the read flag. The executable flag allows traversal of children, but not listing of the parent:

$ ls -alh example
ls: example: Permission denied
$ sudo ls -alh example
total 0
d-w---x---   3 root    staff    96B 27 Aug 08:49 .
drwxr-xr-x  18 chainz  staff   576B 27 Aug 08:49 ..
drwxr-xr-x   3 chainz  staff    96B 27 Aug 08:49 subexample
$ ls example/subexample
myfile.txt

@adamchainz
Copy link
Member

@felixxm There other instances of path.resolve(strict=True) in the codebase, at least in the autoreloader and the new settings file template - we should remove the strict flag from all of them right?

@carltongibson
Copy link
Member

...we should remove the strict flag from all of them right?

This was my question. (Didn't have time to really think about it yet. Technically On holiday™)

@felixxm
Copy link
Member Author

felixxm commented Aug 27, 2020

@MarkusH I reproduced this issue with:

drwx------ 3 root   root   4096 sie 27 09:40 cos
drwxr-xr-x 3 root   root   4096 sie 27 09:40 cos/local
drwxr-xr-x 3 root   root   4096 sie 27 09:40 cos/local/bin
drwxr-xr-x 2 felixx felixx 4096 sie 27 09:50 cos/local/bin/felixx
-rw-rw-r-- 1 felixx felixx   62 sie 27 09:50 cos/local/bin/felixx/script.py

@felixxm
Copy link
Member Author

felixxm commented Aug 27, 2020

... - we should remove the strict flag from all of them right?

I'm not sure, I really have many issues with creating a testable Django project in such configuration 😞

@MarkusH
Copy link
Member

MarkusH commented Aug 27, 2020

@MarkusH it's possible with a parent directory that's owned by root, and has executable flag set for the group but not the read flag. The executable flag allows traversal of children, but not listing of the parent:

Thanks @adamchainz. That's what I was missing.

@carltongibson
Copy link
Member

carltongibson commented Aug 27, 2020

I'm not sure...

OK, I'm inclined to say we can leave it unless an actual bug arises. Two occurrences:

  1. The auto-reloader: are people really using that in one of these strange permissions set-ups, which I'm guessing are managed hosts for deployment?
  2. The settings file: surely (🙂) folks have permissions there, and in the unlikely even they don't, they can edit the settings file.

On the other hand, strict=True says "and this path really exists" (IIUC) — I'm not sure we really need that in either case. (An error would show up soon enough if not...)

Inclined to leave it since it's the smaller change, but not strongly attached to that.

@felixxm
Copy link
Member Author

felixxm commented Aug 27, 2020

OK, I'm inclined to say we can leave it unless an actual bug arises. Two occurrences:

Yes, please ✔️ Reporter confirmed that everything works for them with this small patch.

  1. The auto-reloader: are people really using that in one of these strange permissions set-ups, which I'm guessing are managed hosts for deployment?

Also, this was changed in Django 3.0 and we didn't get any related report.

  1. The settings file: surely (slightly_smiling_face) folks have permissions there, and in the unlikely even they don't, they can edit the settings file.

IMO, we can leave it.

@adamchainz
Copy link
Member

IMO, we can leave it.

I think we should fix the settings file at least - if there's any chance this would trip up a beginner, they might give up and go to the Node.js side.

@felixxm felixxm changed the title Fixed #31945 -- Removed strict=True in Path.resolve() of CommonPasswordValidator. Fixed #31912 -- Removed strict=True in Path.resolve() in project template and CommonPasswordValidator. Aug 27, 2020
@tytusd
Copy link

tytusd commented Aug 27, 2020

Many thanks for the support on this, much appreciated!

Just for your information, I use https://www.mydevil.net/ which offers partially managed environment suited very much for Django. A lot of people use it for this purpose and this fix helps all of them.

…late and CommonPasswordValidator.

This caused permission errors when user didn't have permissions to
intermediate directories in a Django installation path.

Thanks tytusd and leonyxz for reports.

Regression in edeec12 and
26554cf.
@felixxm
Copy link
Member Author

felixxm commented Aug 27, 2020

@adamchainz Done.

@felixxm felixxm merged commit e39e727 into django:master Aug 28, 2020
@felixxm felixxm deleted the issue-31945 branch August 28, 2020 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants