Summary:	ASTERISK-13291: [patch] temporary files should be in temporary directory
Reporter:	Tzafrir Cohen (tzafrir)	Labels:
Date Opened:	2009-01-01 16:26:12.000-0600	Date Closed:	2009-01-22 15:26:18.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 20090104__bug14160.diff.txt ( 1) tmp_under_spool.diff
Description:	Asterisk has a number of places that use either a constant of a predictable file name in either /var/tmp or in /tmp . This may allow a local attacker to delete an arbitrary file that is writable by Asterisk, or maybe even run over such a file. In practice asterisk tends to be able to write enough interesting files. However Asterisk already has a temporary directory that is writable by Asterisk and not writable by other users: /var/spool/asterisk/tmp . I attach an initial patch to fix this in a rather brute-force way: use the hard-wired default value from defaults.h . This also means including defaults.h in quite a few files, which is probably not a good idea either. But as it is constant it makes the changes simpler. Note that the patch also changed some places that already had created the temporary file in a safe way (e.g.: using mkstemp). I mainly used the following two greps:  grep /tmp */*.[ch]  grep tmp/ */*.[ch]
Comments:	By: Tilghman Lesher (tilghman) 2009-01-02 13:10:44.000-0600 I don't think most of these are that necessary.  In some cases, you're talking about DEBUG files, which should not be enabled for production anyway.  In other cases, you're talking about files which use the extremely safe mkstemp(). Finally, simply changing the directory in which temporary files are written does ZERO for the overall security of the process; it just means that an attacker has to look in a different directory for his attack. In summary, I think this patch does precisely NOTHING except be a PITA of developers. By: Tzafrir Cohen (tzafrir) 2009-01-02 17:40:51.000-0600 The difference is that an attacher (who is not Asterisk) can write to /tmp and /var/tmp but cannot write to /var/spool/asterisk/tmp . This is not supposed to protect from an attacker who runs with the permissions of Asterisk. By: Tilghman Lesher (tilghman) 2009-01-03 00:32:04.000-0600 So why not fix the process to create files safely, instead of changing directories to a directory which could, very easily, be made +t by a sysadmin, without realizing the danger that causes? By: Tzafrir Cohen (tzafrir) 2009-01-03 08:36:27.000-0600 You want the refs file to have an unpredictable name? And no, I don't see why the tmp directory should be world writable. It has never been defined to be that way. By: Tilghman Lesher (tilghman) 2009-01-03 10:33:59.000-0600 No, you do it the same way we do lock files:  create the file with mkstemp, then use link(2) to link the file over to the name you want, unlink(2)ing as necessary any existing file, then unlink(2) the mkstemp filename. By: Tilghman Lesher (tilghman) 2009-01-03 10:34:51.000-0600 And furthermore, I don't think the refs files need any special treatment; as I noted before, they are for debugging only, not for production use. By: Tilghman Lesher (tilghman) 2009-01-04 19:39:54.000-0600 Of the files you've suggested a change towards, the only one that I think both merits a change and is a change that won't be instantly reverted by the maintainer is the one to abstract_jb.c.  A patch is uploaded.  Please note that while you may have good ideas for both chan_usbradio.c and app_rpt.c, any changes not made by the maintainer get reverted soon afterwards, so I wouldn't bother messing with either of them. By: Jeff Peeler (jpeeler) 2009-01-05 10:27:59.000-0600 Since this bug is private, I'm going to debug here the email notification stuff. By: Digium Subversion (svnbot) 2009-01-22 15:25:24.000-0600 Repository: asterisk Revision: 170307 U   trunk/main/abstract_jb.c ------------------------------------------------------------------------ r170307 | tilghman | 2009-01-22 15:25:23 -0600 (Thu, 22 Jan 2009) | 6 lines Create logfile safely. (closes issue ASTERISK-13291) Reported by: tzafrir Patches:       20090104__bug14160.diff.txt uploaded by Corydon76 (license 14) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=170307 By: Digium Subversion (svnbot) 2009-01-22 15:26:18.000-0600 Repository: asterisk Revision: 170308 _U  branches/1.6.1/ ------------------------------------------------------------------------ r170308 | tilghman | 2009-01-22 15:26:18 -0600 (Thu, 22 Jan 2009) | 12 lines Blocked revisions 170307 via svnmerge ........  r170307 | tilghman | 2009-01-22 15:25:46 -0600 (Thu, 22 Jan 2009) | 6 lines    Create logfile safely.  (closes issue ASTERISK-13291)   Reported by: tzafrir   Patches:         20090104__bug14160.diff.txt uploaded by Corydon76 (license 14) ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=170308