Closed
Bug 197153
Opened 22 years ago
Closed 22 years ago
potential symlink attack vulnerability in showdependencygraph.cgi and GenerateVersionTable
Categories
(Bugzilla :: Bugzilla-General, defect, P1)
Bugzilla
Bugzilla-General
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: jon, Assigned: bbaetz)
References
Details
(Whiteboard: [fixed in 2.16.3] [fixed in 2.17.4])
Attachments
(3 files, 1 obsolete file)
9.90 KB,
patch
|
gerv
:
review+
justdave
:
review+
|
Details | Diff | Splinter Review |
9.78 KB,
patch
|
gerv
:
review+
justdave
:
review+
|
Details | Diff | Splinter Review |
4.37 KB,
patch
|
justdave
:
review+
myk
:
review+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
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.
Group: webtools-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 2.18
Updated•22 years ago
|
Severity: normal → critical
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Summary: small file opening bug in showdependancygraph.cgi → potential symlink attack vulnerability in showdependancygraph.cgi
Whiteboard: [wanted for 2.16.3][wanted for 2.17.4]
Reporter | ||
Comment 2•22 years ago
|
||
Comment 3•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Summary: potential symlink attack vulnerability in showdependancygraph.cgi → potential symlink attack vulnerability in showdependancygraph.cgi and GenerateVersionTable
Comment 6•22 years ago
|
||
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.
Assignee | ||
Comment 7•22 years ago
|
||
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.
Assignee | ||
Comment 8•22 years ago
|
||
-> 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
Assignee: justdave → bbaetz
Assignee | ||
Comment 9•22 years ago
|
||
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?
Attachment #117041 -
Attachment is obsolete: true
Assignee | ||
Comment 10•22 years ago
|
||
Oh, and the final 2.16 patch will need for 5.005 testing too.
Assignee | ||
Comment 11•22 years ago
|
||
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??
Assignee | ||
Updated•22 years ago
|
Attachment #117589 -
Flags: review?(gerv)
Assignee | ||
Updated•22 years ago
|
Attachment #117706 -
Flags: review?(gerv)
Comment 12•22 years ago
|
||
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
Attachment #117589 -
Flags: review?(gerv) → review+
Assignee | ||
Comment 13•22 years ago
|
||
> 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•22 years ago
|
||
Comment on attachment 117706 [details] [diff] [review]
2.16 patch
r=gerv on the 2.16 version.
Gerv
Attachment #117706 -
Flags: review?(gerv) → review+
Assignee | ||
Comment 15•22 years ago
|
||
justdave, a=?
Anyone want to answer my question in comment 11?
Flags: approval?
Comment 16•22 years ago
|
||
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.
Reporter | ||
Comment 17•22 years ago
|
||
so will this fix go into 2.16.3 / 2.17.4?
Comment 18•22 years ago
|
||
yep. At this point, looks like we're releasing tomorrow, unless something else
comes up.
Updated•22 years ago
|
Flags: approval? → approval+
Comment 19•22 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [wanted for 2.16.3][wanted for 2.17.4] → [fixed in 2.16.3] [fixed in 2.17.4]
Comment 20•22 years ago
|
||
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;
Assignee | ||
Comment 21•22 years ago
|
||
I had that change locally, but I obviously forgot to mention it in the bug...
Assignee | ||
Comment 22•22 years ago
|
||
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
Assignee | ||
Comment 23•22 years ago
|
||
2.17 patch was broken. Oops. patch coming
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 24•22 years ago
|
||
'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)
Updated•22 years ago
|
Attachment #121597 -
Flags: review+
Updated•22 years ago
|
Attachment #121597 -
Flags: review?
Comment 25•22 years ago
|
||
Comment on attachment 121597 [details] [diff] [review]
fix the bracketing
r=myk
Attachment #121597 -
Flags: review?
Comment 26•22 years ago
|
||
a= justdave on the bracketing patch
Assignee | ||
Comment 27•22 years ago
|
||
Fixed (again!)
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl
new revision: 1.236; previous revision: 1.235
done
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 28•22 years ago
|
||
Security Advisory has been posted, removing security group
Group: webtools-security
Comment 29•21 years ago
|
||
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.
Summary: potential symlink attack vulnerability in showdependancygraph.cgi and GenerateVersionTable → potential symlink attack vulnerability in showdependencygraph.cgi and GenerateVersionTable
Comment 30•21 years ago
|
||
I thought File::Temp was supposed to be cross-platform... what did we break?
Someone needs to tell us these things ;)
Comment 31•21 years ago
|
||
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.
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•