Insecure pid file creation

[ThiefMaster]

The pid file is not created with secure permissions (0600) which may result in a world-writable PID file.
This results in a security hole as calling loop() with daemonize arguments again will kill whatever pid is listed in that file.
And depending on what you use your program for it might be even killed from a process running with root privileges which is certainly bad.

[seb-m]

Thanks for your report, I have a question though: does file(pid_file, "wb") (see http://github.com/seb-m/pyinotify/blob/master/python2/pyinotify.py#L1269 ) not opening pid_file in mode 0644 (with usual umask equal to 022)?

[ThiefMaster]

It's caused by os.umask(0) - that umask resoluts in 0666.
I think the proper solution would be to os.fchmod() the opened file to 0644 or 0600.
os.chmod() would also work, but keep the risk of a race condition (create file, write to it, someone else overwrites it, you chmod it -> the bad pid is still in there)

[seb-m]

I'm agree with you even if in my tests for whatever reason the pid file seems to always have the write right set only for the owner. Anyway, you're absolutely right, the rights were too loose and must be tightened. Please review and test this commit http://github.com/seb-m/pyinotify/commit/155e2f5d0dfeff210198856b2b03d53d8e810c52 I've implemented these ideas and I also removed the os.kill() instructions it is more prudent. Thanks for your help.

[ThiefMaster]

You could check if there's still a process with the pid in the file - a pid file of a dead process (e.g. after a kernel panic or hard shutdown) blocking a program from starting up isn't really nice.
Or you could make the pid argument really optional - i.e. not create a pid file at all if it's omitted.

[seb-m]

Yep I'm agree it isn't really nice, I think I'm going to implement option 2 for now, at least it will be better than nothing.

[seb-m]

Related commit http://github.com/seb-m/pyinotify/commit/909f985842fcae8a45d9591017112186e68a723d

[seb-m]

I'm going to close this issue if we had to discuss about reimplementing the old pid_file behavior it will be more appropriate to create a new ticket.