[DRE-maint] Is Ruby's Tempfile secure?

[Michael Schutte]

Charles, Thijs,

On Mon, Sep 01, 2008 at 10:17:46PM +0900, Charles Plessy wrote:
> In the course of solving a security bug in a Ruby script, we came to wonder if
> the "Tempfile" class is secure or not. Can we have your opinion?
> 
> Le Mon, Sep 01, 2008 at 02:53:27PM +0200, Thijs Kinkhorst a écrit :
> > On Mon, September 1, 2008 14:44, Charles Plessy wrote:
> > > for your information, Upstream solved the problem using the Tempfile
> > > class, with calls like this:
> > >
> > > temp_vf = Tempfile.new("_vf").path
> > >
> > > I suppose it is as good as stemp?
> > 
> > I'm afraid the Tempfile class doesn't look secure to me:
> > "Creates a temporary file of mode 0600 in the temporary directory whose
> > name is basename.pid.n"
> > 
> > Also the first sentence of the Ruby-stemp website reads:
> > "Pure Ruby tempfile implementations appear to suffer from well-known race
> > conditions."
> > http://ruby-stemp.rubyforge.org/

As far as I can tell, there is no dangerous race condition in the
Tempfile implementation (at least in 1.8.5-4etch2 and 1.8.7.22-3, I
didn’t look at other versions).  This is the relevant line from
tempfile.rb:

    @tmpfile = File.open(tmpname, File::RDWR|File::CREAT|File::EXCL, 0600)

File::EXCL (O_EXCL, see open(2)) ensures that the temporary file will be
newly created; if it is already present, an exception is raised, which
terminates the script.  Even if the unlikely case of a duplicate name
occurs, this should be safe.

> […]
>
> For the race condition, I really do not know. I think that the upstream author
> is quite busy and was kind enough to help me to get a working patch quickly
> (the one I submitted to you for review did not work in the end). Unless our
> Ruby experts confirm that Tempfile should not be used, I would not like to ask
> him to revise his patch.

The patch [1] does some unusual things.  Tempfile.new(template) creates
a new, empty file; snippets like

    system( mafftpath + " --help > #{temp_vf} 2>&1" )
    pfp = File.open( "#{temp_vf}", 'r' )

cause a subshell to re-open the file and write the output of a command
into it.  The Ruby script then opens the file *again* for reading.  From
a security standpoint, this is okay — you won’t want to change the
overall structure of the script too much in a patch like this.  There is
room for improvement, though; using pipes makes much more sense for
later revisions.

I also fail to see the reason for using eight temporary files.  If you
insist on using them at all, one should usually be enough.

[1] http://svn.debian.org/wsvn/debian-med/trunk/packages/mafft/trunk/debian/patches/Securisation-by-mktemp-usage.patch?op=file&rev=0&sc=0

Hope that helps,
-- 
Michael Schutte <michi at uiae.at>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 489 bytes
Desc: Digital signature
Url : http://lists.alioth.debian.org/pipermail/pkg-ruby-extras-maintainers/attachments/20080902/aa48bfad/attachment.pgp