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.

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.

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...

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?

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.

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.

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.

-> 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

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?

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

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 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

> 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 on attachment 117706 [details] [diff] [review] 2.16 patch r=gerv on the 2.16 version. Gerv

justdave, a=? Anyone want to answer my question in comment 11?

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.

so will this fix go into 2.16.3 / 2.17.4?

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

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

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;

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

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

2.17 patch was broken. Oops. patch coming

'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 on attachment 121597 [details] [diff] [review] fix the bracketing r=myk

a= justdave on the bracketing patch

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

Security Advisory has been posted, removing security group

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.

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

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.