All users were logged out of Bugzilla on October 13th, 2018

potential symlink attack vulnerability in showdependencygraph.cgi and GenerateVersionTable

RESOLVED FIXED in Bugzilla 2.18

Status

()

P1
critical
RESOLVED FIXED
16 years ago
6 years ago

People

(Reporter: jon, Assigned: bbaetz)

Tracking

unspecified
Bugzilla 2.18
Bug Flags:
approval +

Details

(Whiteboard: [fixed in 2.16.3] [fixed in 2.17.4])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
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.
Group: webtools-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 2.18
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

16 years ago
Created attachment 117041 [details] [diff] [review]
check to see if $filename exists before writing 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...

Comment 4

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

16 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

16 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

16 years ago
Created attachment 117589 [details] [diff] [review]
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?
Attachment #117041 - Attachment is obsolete: true
(Assignee)

Comment 10

16 years ago
Oh, and the final 2.16 patch will need for 5.005 testing too.
(Assignee)

Comment 11

16 years ago
Created attachment 117706 [details] [diff] [review]
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??
(Assignee)

Updated

16 years ago
Attachment #117589 - Flags: review?(gerv)
(Assignee)

Updated

16 years ago
Attachment #117706 - Flags: review?(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
Attachment #117589 - Flags: review?(gerv) → review+
(Assignee)

Comment 13

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

16 years ago
justdave, a=?

Anyone want to answer my question in comment 11?
Flags: approval?
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

16 years ago
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.
Flags: approval? → approval+
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
Last Resolved: 16 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]
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

16 years ago
I had that change locally, but I obviously forgot to mention it in the bug...
(Assignee)

Comment 22

16 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

16 years ago
2.17 patch was broken. Oops. patch coming
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 24

16 years ago
Created attachment 121597 [details] [diff] [review]
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 on attachment 121597 [details] [diff] [review]
fix the bracketing

r=myk
Attachment #121597 - Flags: review?
a= justdave on the bracketing patch
(Assignee)

Comment 27

16 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
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
Security Advisory has been posted, removing security group
Group: webtools-security

Comment 29

15 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
I thought File::Temp was supposed to be cross-platform...  what did we break? 
Someone needs to tell us these things ;)

Comment 31

15 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.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.