Bug #69561 for App-Daemon: Unsafe use of temporary directory for pidfile and logfile

Mon Jul 18 22:55:06 2011 john [...] nixnuts.net - Ticket created

Subject:

Unsafe use of temporary directory for pidfile and logfile

The defaults for the pidfile and logfile and predictable filenames under /tmp. These are trivial to exploit on a multiuser system to overwrite arbitrary files, trick someone into killing arbitrary processes, prevent a daemon from starting, etc. Oddly, App::Daemon::detach() also sets the umask to 0 so that the pidfile itself gets world writable permissions regardless of the directory its written into.

Tue Jul 19 01:22:50 2011 MSCHILLI [...] cpan.org - Correspondence added

On Mon Jul 18 22:55:06 2011, lightsey wrote: Show quoted text

> The defaults for the pidfile and logfile and predictable filenames under > /tmp. These are trivial to exploit on a multiuser system to overwrite > arbitrary files, trick someone into killing arbitrary processes, prevent > a daemon from starting, etc.

Thanks, yeah, I've always wanted to fix that ... here's a patch that puts them into the current directory (serious daemons will specify locations in /var/ explicitly, but it's good practice to use a reasonable default): https://github.com/mschilli/app-daemon/tree/41babec7a67b7b7942058a2a3c180c1473b12c27 Show quoted text

> Oddly, App::Daemon::detach() also sets the umask to 0 so that the > pidfile itself gets world writable permissions regardless of the > directory its written into.

Yeah, seems odd, think I copied it from http://www.netzmafia.de/skripten/unix/linux-daemon-howto.html#ss4.2 which upon closer examination sounds like bad advice ... I went ahead and fixed it in https://github.com/mschilli/app-daemon/commit/6ab07fb2a6cfa19f04bb29dcc8169c495f95cf58 to use 0133 instead. Do you see any issues?

Tue Jul 19 01:22:51 2011 The RT System itself - Status changed from 'new' to 'open'

Tue Jul 19 22:28:46 2011 john [...] nixnuts.net - Correspondence added

Subject:	Re: [rt.cpan.org #69561] Unsafe use of temporary directory for pidfile and logfile
Date:	Tue, 19 Jul 2011 21:28:17 -0500
To:	bug-App-Daemon [...] rt.cpan.org
From:	John Lightsey <john [...] nixnuts.net>

On 07/19/2011 12:22 AM, Michael_Schilli via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=69561 > > > On Mon Jul 18 22:55:06 2011, lightsey wrote:

>> The defaults for the pidfile and logfile and predictable filenames under >> /tmp. These are trivial to exploit on a multiuser system to overwrite >> arbitrary files, trick someone into killing arbitrary processes, prevent >> a daemon from starting, etc.

> > Thanks, yeah, I've always wanted to fix that ... here's a patch that > puts them into the current directory (serious daemons will specify > locations in /var/ explicitly, but it's good practice to use a > reasonable default): > > https://github.com/mschilli/app-daemon/tree/41babec7a67b7b7942058a2a3c180c1473b12c27

Looks reasonable. If you really want to avoid /var/log and /var/run as defaults, I'd suggest using $HOME instead of ./ though. Having a pid file doesn't make much sense if it's operation is dependent on the current directory. Show quoted text

>> Oddly, App::Daemon::detach() also sets the umask to 0 so that the >> pidfile itself gets world writable permissions regardless of the >> directory its written into.

> > Yeah, seems odd, think I copied it from > > http://www.netzmafia.de/skripten/unix/linux-daemon-howto.html#ss4.2 > > which upon closer examination sounds like bad advice ... I went ahead > and fixed it in > > > https://github.com/mschilli/app-daemon/commit/6ab07fb2a6cfa19f04bb29dcc8169c495f95cf58 > > to use 0133 instead. > > Do you see any issues?

I'd suggest using sysopen() for the pid file if you want to set the permissions explicitly. Any subprocesses are going to inherit the umask you're setting. It's odd IMHO for a module to change the umask globally without any indication in the POD or ability to control it. Thanks for the fast response.

Wed Jul 20 00:02:56 2011 MSCHILLI [...] cpan.org - Correspondence added

On Tue Jul 19 22:28:46 2011, lightsey wrote: Show quoted text

> I'd suggest using $HOME instead of ./ though.

I've added an explanation to the docs that the current directory is just for testing purposes, I kind of like to have to pid/log files close for testing. Show quoted text

> I'd suggest using sysopen()

Yeah, makes sense, I got rid of the umask: https://github.com/mschilli/app-daemon/commit/1e4324d94b9ab930ead1951dd0057ddb0e2ece60 Released in 0.13, thanks for your feedback!

Wed Jul 20 00:03:03 2011 MSCHILLI [...] cpan.org - Status changed from 'open' to 'resolved'