305353 - [SECURITY] Insecure temporary filename handling in syncshadowdb

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Dave Miller [:justdave]]

From: Javier Fernández-Sanguino Peña <[e-mail]>
Date: Sat, 6 Aug 2005 11:05:50 +0200
To: Alexis Sukrieh <[e-mail]>, RACmi Perrot <[e-mail]>,
	[e-mail]
Subject: Bugzilla: Unsafe use of temporary files in the syncshadow script
User-Agent: Mutt/1.5.9i

[ Note: this is a different bug from the one I reported in the BTS
  since it affects the stable / oldstable release and not the testing /
  unstable branches ]

Hi there,

Bugzilla (bugzilla_2.14.2-0woody4 and bugzilla_2.16.7-7sarge1) contains
a script which is used to synchronise the bugzilla user database with
the shadow password database called syncshadowdb. This script is intented
to be run by the Bug Tracking System.

The script uses temporary files in an unsafe way since it selects a
name for the file based on PID and does not make any effort to determine
if the file exists and if it is a symlink. A local user could use this
to direct symlink attacks and overwrite files that the Bug Tracking System
has access to.

The attached (untested) patch, which uses File::Temp should fix this issue
and prevent any symlink attacks.

Regards

Javier

Comment 1

[Dave Miller [:justdave]]

Confirmed this still exists in 2.16.10.  This file no longer exists (in favor of
MySQL's native replication) in versions newer than 2.16.x.

Comment 2

[Dave Miller [:justdave]]

Attachment: The patch that was attached to the email

Comment 3

[Dave Miller [:justdave]]

Comment on attachment 193308 [details] [diff] [review]
The patch that was attached to the email

Patch applies cleanly to 2.16 branch.  Apply using -p1

Comment 4

[Dave Miller [:justdave]]

Comment on attachment 193308 [details] [diff] [review]
The patch that was attached to the email

Something about this implementation bugs me...	tempfile returns a filehandle,
but we redirect output to the file later, instead, which opens it again, which
will probably fail because tempfile already opened it for writing.

Instead of redirecting STDOUT to the filename, we should redirect it to the
filehandle that was returned from tempfile.

Also, just as a point of comparison, other places in the code where we use
File::Temp don't import any functions from File::Temp on the use line, and
explicitly spell out File::Temp::tempfile().  It'll still work the way you did
it, it's just nice to keep the code consistent sometimes. :)

Comment 5

[Teemu Mannermaa (:wicked)]

Attachment: syncshadowdb patch for 2.16.10

This should correct all problems justdave mentioned in previous comment. Tested to seemingly work.

Comment 6

[Dave Miller [:justdave]]

Comment on attachment 203870 [details] [diff] [review]
syncshadowdb patch for 2.16.10

>-    open(MYSQL, "cat $tempfile | $::mysqlpath/mysql $extra " .
>+    open (MYSQL, "/bin/cat $tempfile | $::mysqlpath/mysql $extra " .
>          Param("shadowdb") . "|") || die "Couldn't do db copy";

This assumes that the 'cat' command is in /bin, which we don't know (what OS are we running on?)

Oh, on the other hand, this is 2.16, which required hacks to run on anything other than Linux anyway.  Maybe that'll work.

I want to test this before I r+ it.  Setting up the test environment for this particular situation is going to be a pain though, so it probably won't be tonight.

Comment 7

[Dave Miller [:justdave]]

Comment on attachment 203870 [details] [diff] [review]
syncshadowdb patch for 2.16.10

(In reply to comment #6)
> I want to test this before I r+ it.  Setting up the test environment for this
> particular situation is going to be a pain though, so it probably won't be
> tonight.

On the other hand, Teemu stated he tested it already, and he has a very good prior history of accurate testing.  By inspection this looks good to me, and it compiles.

Comment 8

[Dave Miller [:justdave]]

holding approval for security advisory...

Comment 9

[timeless]

Comment on attachment 203870 [details] [diff] [review]
syncshadowdb patch for 2.16.10

you added a space between open and (). i don't think it belongs :(.

Comment 10

[Teemu Mannermaa (:wicked)]

(In reply to comment #9)
> you added a space between open and (). i don't think it belongs :(.

Committer should probably remove it then..

Comment 11

[Dave Miller [:justdave]]

The original reported (jfs) mistakenly thought we'd opened this to the public because he could see it (which in fact it was only because he was CCed on the bug, and thus could see it), and filed this in Debian's public bug system on September 21st: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=329387

Secunia just picked it up this morning: http://secunia.com/advisories/18218/

No point in leaving it hidden now.

Comment 12

[Dave Miller [:justdave]]

Go ahead and check this into cvs, no point in waiting, since it's public.

Let's see if we can get an advisory out today referencing the patch, too.

Comment 13

[Dave Miller [:justdave]]

I must make an apology to Javier, this was never intended to be sat on for so long, and he's obviously used to how Debian does things, and not how we do things.  We all let it sit expecting Javier to update the patch after it was reviewed, and then that never happened.  And I'm as much at fault because I failed to indicate in my review that he should update it.

Since Teemu doesn't have checkin privs, I'm checking this in.

Checking in syncshadowdb;
/cvsroot/mozilla/webtools/bugzilla/Attic/syncshadowdb,v  <--  syncshadowdb
new revision: 1.14.2.1; previous revision: 1.14
done

Comment 14

[Dave Miller [:justdave]]

Forgot to mark this fixed when I checked in.

FYI, It's likely going to be tomorrow some time before the advisory actually happens.  We're still too close to Christmas, and I can't get all the right people in the right place at the same time to make it happen. :(

Comment 15

[Dave Miller [:justdave]]

Comment on attachment 203870 [details] [diff] [review]
syncshadowdb patch for 2.16.10

For those coming here from the security advisory looking for the patch to apply, it's the one labelled "syncshadowdb patch for 2.16.10" in the attachment list near the top of this page.

Comment 16

[David D. Kilzer (ddk)]

Not that it matters much now, but wouldn't it be a good idea to close the $tmpfh filehandle before running "/bin/cat $tempfile", even though buffering is disabled?

open STDOUT, ">&SAVEOUT"; # redirect back to original stream
close($tmpfh);
Verbose("Restoring from tempfile into shadowdb");

Comment 17

[Dave Miller [:justdave]]

(In reply to comment #16)
> Not that it matters much now, but wouldn't it be a good idea to close the
> $tmpfh filehandle before running "/bin/cat $tempfile", even though buffering is
> disabled?

The File::Temp construct that we're using to create the temp file automatically deletes the file when we close it, so if we did that, there would be nothing there to cat.  Doing a $tmpfh->flush() might not be a bad idea.  Reading the documentation, having autoflush enabled ($|=1) is probably sufficient though.