Skip to content

File permission of lock file #112

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

Closed
ghost opened this issue Oct 25, 2021 · 7 comments
Closed

File permission of lock file #112

ghost opened this issue Oct 25, 2021 · 7 comments

Comments

@ghost
Copy link

ghost commented Oct 25, 2021

The creation of a lockfile with …

lock = FileLock("/var/lock/foo.lock")

… leads to these file permissions: -rwxr-xr-x

Is there any way to prevent that the lock file becomes an executable with root ownership?

(Version: 3.0.12-2 in Ubuntu 20.04)

@gaborbernat
Copy link
Member

Can't you just change the file permissions after the lock file is created?

@ghost
Copy link
Author

ghost commented Oct 26, 2021

Sure. But this is a potential security problem and should not happen at all. Creating files which are world wide executable is quite the opposite to the principle of least surprise.

@gaborbernat
Copy link
Member

Feel free to open a PR with your proposed solution.

@ghost
Copy link
Author

ghost commented Oct 26, 2021

Ok. I just have a question about the code:

mode = (
            os.O_WRONLY  # open for writing only
            | os.O_CREAT
            | os.O_EXCL  # together with above raise EEXIST if the file specified by filename exists
            | os.O_TRUNC  # truncate the file to zero byte
        )
        try:
            fd = os.open(self._lock_file, mode)

If I understand the documentation of os.open and the C runtime docs correctly these parameters should be called flags instead of mode and mode would then instead define the file permissions like S_IRUSR, or am I missing something?

@JustAnotherArchivist
Copy link
Contributor

JustAnotherArchivist commented Jan 4, 2022

Your reading of the documentation is correct. The value of mode in the filelock code is passed into os.open's flags argument. This doesn't matter because os.open never sees the name of the variable being passed in, but I agree that mode should be renamed to flags to be less confusing. And then the third argument (= mode keyword argument) to os.open could be passed in whichever notation is preferred (os.S_IRUSR | os.S_IWUSR or 0o600, for example). I'm not sure what platform-specific tweaks might be necessary, especially on the Windows implementation.

@axoroll7
Copy link

Sure. But this is a potential security problem and should not happen at all. Creating files which are world wide executable is quite the opposite to the principle of least surprise.

The lock file is just an empty file ? Why is it a security problem ? I understand your motivation, this file should not be executable. It would be nice if you have an explication, because you seem to know the potential security problems.

@jahrules
Copy link
Contributor

@gaborbernat - I believe the recent PRs should resolve this one. Please let me know if you disagree or if you believe any additional work is needed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants