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-0600Date Closed:2009-01-22 15:26:18.000-0600
Versions:Frequency of
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
      20090104__bug14160.diff.txt uploaded by Corydon76 (license 14)



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
        20090104__bug14160.diff.txt uploaded by Corydon76 (license 14)