197153 - potential symlink attack vulnerability in showdependencygraph.cgi and GenerateVersionTable

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Jonathan Schatz]

User-Agent:       Mozilla/5.0 Galeon/1.2.6 (X11; Linux i686; U;) Gecko/20020830
Build Identifier: Mozilla/5.0 Galeon/1.2.6 (X11; Linux i686; U;) Gecko/20020830

80: my $filename = "data/webdot/$$.dot";
81: my $urlbase = Param('urlbase');
82: 
83: open(DOT, ">$filename") || die "Can't create $filename";

shouldn't we check for '-e $filename' before we clobber it? something like:

my $filename;
for (my $i = $$;; $i++) {
        $filename = "$i.dot";
        last unless (-e $filename);
}

i can't figure out exactly how the webdot directories are created, but in my
setup group has rwx, so the possibility exists of someone doing something bad.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Dave Miller [:justdave]]

the $$ is replaced with the current process ID.  Under normal circumstances, if
we've gone long enough for the process IDs to get reused, the file is old enough
to replace anyway....

However, this does open us up to symlink attacks if someone drops a bunch of
symlinks named appropriately into that directory....  which under some of our
supported configurations is world-writable because the webserver has to be able
to write to it.

Comment 2

[Jonathan Schatz]

Attachment: check to see if $filename exists before writing to it

Comment 3

[Dave Miller [:justdave]]

There's nothing that says we have to use the process ID either.  We just have to
have .dot on the end of the filename.  We could make use of File::Temp to
generate a random filename, too...

Comment 4

[Jacob Steenhagen]

I'm not sure I understand what a "symlink attack" is or how it would be useful.
Are you saying that a user could create a symlink to a file that they can't
modify but the web server can and then eventaully Bugzilla will delete the
contents of that file for them?  If so, isn't that also possible with the
versioncache or the compiled templates?

Comment 5

[Dave Miller [:justdave]]

Pretty much.

data/params is not vulnerable to this because it uses a temp file generated by
File::Temp, a perl module which was created specifically for avoiding this
vulnerability.

    require File::Temp;
    my ($fh, $tmpname) = File::Temp::tempfile('params.XXXXX',
                                              DIR => 'data' );

    print $fh (Data::Dumper->Dump([ \%param ], [ '*param' ]))
      || die "Can't write param file: $!";

    close $fh;

    rename $tmpname, "data/params"
      || die "Can't rename $tmpname to data/params: $!";

versioncache uses a temp file based on the current pid, which not necessarily
being random, would make it vulnerable as well as the .dot files.

    my $tmpname = "data/versioncache.$$";
    open(FID, ">$tmpname") || die "Can't create $tmpname";
  ...
    close FID;

    rename $tmpname, "data/versioncache"
      || die "Can't rename $tmpname to versioncache";


If data/template has this problem, then it's a bug in Template Toolkit, and not
Bugzilla.

Comment 6

[Dave Miller [:justdave]]

Jon's fix is probably as good as any, at least for the .dot files, since those
get cleaned after 24 hours anyway.  since we don't clean leftover versioncache
temp files, we're probably better using File::Temp on that one.

Comment 7

[Bradley Baetz (:bbaetz)]

TT uses File::Temp too, in later versions (earlier ones would just overwrite in
place, which could lead to race conditions between multiple processes, with one
of them geting half the file)

Lets just use File::Temp instead. We already require it, and its simple, and tested.

Comment 8

[Bradley Baetz (:bbaetz)]

-> me

I have a half patch for this. The problem is that we use -o for the dot output
for local dot, and you can't generate a name for that w/o a small race
condition. (Well, you could give it a pty for the fileno, I guess, and I think
that there are perl modules to emulate that for non-unix, but....)

For 2.16, I want to use backticks to grab the output. (or the open(-|) thing)
For trunk, we can look into the GraphViz module. Problem is that that uses
IPC::Run, and brings in _massive_ ammounts of dependancies to run the external
process.

Thoughts

Comment 9

[Bradley Baetz (:bbaetz)]

Attachment: 2.17 patch

OK, heres a 2.17 patch. I did this against 2.17 with the idea of using the same
patch gainst 2.16 (hence the non-use of autoviifying file handles, the lack of
any cleanups, and so on). too much code has moved arround for that to really
work, though.

I've used open -|, which doesn't work on windows. Thats ok for 2.16, but not
really for 2.17. I'd like to check it in anyway, though, and fix that up later.


2.16 also needs a checksetup.pl test for File::Temp, FWIW.

We could use the GraphViz module, but that brings in a fair number of
dependancies. I guess we coudl just use IPC::Run directly, or something.
Thoughts?

Comment 10

[Bradley Baetz (:bbaetz)]

Oh, and the final 2.16 patch will need for 5.005 testing too.

Comment 11

[Bradley Baetz (:bbaetz)]

Attachment: 2.16 patch

OK, heres the 2.16 patch. We now require File::Temp because of this.

Someone want to test of 5.005?

One question though - why does showdependancygraph.cgi chmod the .dot file to
0777??

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 117589 [details] [diff] [review]
2.17 patch

>Index: checksetup.pl
>===================================================================
> # Restrict access to .dot files to the public webdot server at research.att.com 
> # if research.att.com ever changed their IP, or if you use a different
> # webdot server, you'll need to edit this
>-<FilesMatch ^[0-9]+\.dot$>
>+<FilesMatch \.dot$>
>   Allow from 192.20.225.10

<cough>.

>   Deny from all
> </FilesMatch>
> 
>-# Allow access by a local copy of 'dot' to .png, .gif, .jpg, and
>-# .map files
>-<FilesMatch ^[0-9]+\.(png|gif|jpg|map)$>
>+# Allow access to .png files created by a local copy of 'dot'
>+<FilesMatch \.png$>

Do we not need to allow access to .map files? :-) I assume that dot does not,
in fact, generate GIFs or JPEGs.

>Index: defparams.pl
>===================================================================
>                 return "Dependency graph images are not accessible.\nDelete data/webdot/.htaccess and re-run checksetup.pl to rectify.\n";

If those are the instructions, you could do it yourself. How about "Assuming
you have not modified the file, delete..."

>Index: showdependencygraph.cgi
>===================================================================
>-my $filename = "data/webdot/$$.dot";
>+my ($fh, $filename) = File::Temp::tempfile("XXXXXXXXXX",
>+                                           SUFFIX => '.dot',
>+                                           DIR => "data/webdot");

Nit: in the other patch, your spacing around DIR=> is inconsistent.

Otherwise, this looks OK, but it needs testing my someone who has a working
local dot.

Gerv

Comment 13

[Bradley Baetz (:bbaetz)]

> Do we not need to allow access to .map files? :-)

No - we use the output to postprocess into an inline map file. Note that we
don't have to do that any more, using the open() thing - rather than writing to
a file, and then opening it again in the sub, we could just post precess the
scalar directly. That not going to happen for 2.16, and for 2.17 we can use teh
dot feature to generate html image maps directly.  That requires a dot upgrade
though, and is a separate bug to this.

> I assume that dot does not, in fact, generate GIFs or JPEGs.

dot does if you ask it to, but we don't.

Comment 14

[Gervase Markham [:gerv]]

Comment on attachment 117706 [details] [diff] [review]
2.16 patch

r=gerv on the 2.16 version.

Gerv

Comment 15

[Bradley Baetz (:bbaetz)]

justdave, a=?

Anyone want to answer my question in comment 11?

Comment 16

[Dave Miller [:justdave]]

about the chmod?  dunno.  is that in 2.16 or on the trunk also?

as for approval, due to the security nature this will be approved for checkin as
part of the release process so we don't have it on public display in bonsai too
long before the tarballs are available.

Comment 17

[Jonathan Schatz]

so will this fix go into 2.16.3 / 2.17.4?

Comment 18

[Dave Miller [:justdave]]

yep.  At this point, looks like we're releasing tomorrow, unless something else
comes up.

Comment 19

[Dave Miller [:justdave]]

BUGZILLA-2_16-BRANCH:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.149.2.16; previous revision: 1.149.2.15
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.73.2.3; previous revision: 1.73.2.2
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.169.2.14; previous revision: 1.169.2.13
done
Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <-- 
showdependencygraph.cgi
new revision: 1.18.2.3; previous revision: 1.18.2.2
done

HEAD:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.228; previous revision: 1.227
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.114; previous revision: 1.113
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.235; previous revision: 1.234
done
Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <-- 
showdependencygraph.cgi
new revision: 1.28; previous revision: 1.27
done

Comment 20

[Dave Miller [:justdave]]

FYI, the 2.16 branch tinderboxes all went orange when this was checked in.

#     Failed test (t/001compile.t at line 81)
not ok 12 - defparams.pl --WARNING
Bareword found where operator expected at defparams.pl line 64, near "$fh
	GenerateCode" (#1)
    (S) The Perl lexer knows whether to expect a term or an operator.  If it
    sees what it knows to be a term when it was expecting to see an
    operator, it gives you this warning.  Usually it indicates that an
    operator or delimiter was omitted, such as a semicolon.
    
	(Missing operator before GenerateCode?)
defparams.pl syntax OK


After comparing it to the patch on the trunk (which didn't fail tests), the
following was checked in to correct the test failure:

--- defparams.pl        24 Apr 2003 21:15:46 -0000      1.73.2.3
+++ defparams.pl        24 Apr 2003 21:37:06 -0000
@@ -61,7 +61,7 @@
     my $v = $::param{'version'};
     delete $::param{'version'};  # Don't write the version number out to
                                 # the params file.
-    print $fh GenerateCode('%::param');
+    print $fh (GenerateCode('%::param'));
     $::param{'version'} = $v;
     print $fh "1;\n";
     close $fh;

Comment 21

[Bradley Baetz (:bbaetz)]

I had that change locally, but I obviously forgot to mention it in the bug...

Comment 22

[Bradley Baetz (:bbaetz)]

I checked in the wording change gerv wanted, that wasn't on the patch on the bug. 

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.73.2.5; previous revision: 1.73.2.4
done

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.115; previous revision: 1.114
done

Comment 23

[Bradley Baetz (:bbaetz)]

2.17 patch was broken. Oops. patch coming

Comment 24

[Bradley Baetz (:bbaetz)]

Attachment: fix the bracketing

'oops'

2.16 is OK; the extra bracketing is only required because we're not calling a
sub which has already been declared. (Thats why 2.16 needed the extra fix for
the dependency graph stuff)

Comment 25

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 121597 [details] [diff] [review]
fix the bracketing

r=myk

Comment 26

[Dave Miller [:justdave]]

a= justdave on the bracketing patch

Comment 27

[Bradley Baetz (:bbaetz)]

Fixed (again!)

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.236; previous revision: 1.235
done

Comment 28

[Dave Miller [:justdave]]

Security Advisory has been posted, removing security group

Comment 29

[Jouni Heikniemi]

In retrospect, and not that it matters much in practice (nobody probably ever
noticed), I find it to be quite a bad solution to introduce another
Win32-breaking change in a 2.16-tree security fix and not mention it in the
release notes. While 2.16 was never promised to be Win32 compatible,
administrators who had patched their 2.16 Bugzillas to work IMO had the right to
expect a security update not to break features that did work previously. Also,
somebody should have filed a bug to fix the stuff that this patch broke on the
trunk.

All right, past is past so I'll stop whining. Fixing the Win32 incompatibility
introduced by these patches is now bug 225359.

Comment 30

[Dave Miller [:justdave]]

I thought File::Temp was supposed to be cross-platform...  what did we break? 
Someone needs to tell us these things ;)

Comment 31

[Jouni Heikniemi]

Sorry for being unclear. File::Temp probably also works on Win32 (haven't tested
really intimately, though), but using the pipe open syntax doesn't. See bbaetz'
comment 9 for details.