108982 - Need to enable taint mode

Status: RESOLVED FIXED

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

Description

[Bradley Baetz (:bbaetz)]

The last couple of days have shown that we need to get the taint stuff enabled.
I think we can do this in 3 stages:

1) Run all scripts with -T
2) Turn DBI taint on, but filter the return value from FetchSQLData with trick_taint

3) Remove the filter.

1 should be relatively easy, since processmail runs that way already. Anywhere 2
breaks is probably a security bug.
3 may be more difficult.

1 and 2 should be done for 2.16, hopefully.

Comment 1

[Matthew Tuck [:CodeMachine]]

*Sigh* I wish this was considered critical.  Hopefully we'll get several bits of
taint in for 2.16 though.

Comment 2

[Bradley Baetz (:bbaetz)]

Well, I started hacking on option 2. Then I tried using a $::FORM element
without quoting.

Theres no error. I think this is because (with the possible exception of the
last item), we run everything through a regexp in ParseUrlString. $buffer is
tainted on the way in. Is there a way to either not untaint a variable, or taint
it expicitly in perl?

Comment 3

[Dave Miller [:justdave]]

my $untaintedvar = "hello";
my $taintedvar = `$^X -e 'print "$untaintedvar";'`;

that loads a perl interpreter, so it'll decrease performance, but it's a known
path (remember we're deleting $::ENV{PATH} when we start) and we can't depend on
the location of 'echo' in a system (which would run faster).  Otherwise you
could do this:

my $taintedvar = `echo '$untaintedvar'`;

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: patch

A 0-length substring of a tainted var is tainted. Google rules :)

I've chosen not to use DBI's stuff, mainly because I want a proper stack trace.
I don't think I can override DBI's die(), can I?

This diff is just of CGI.pl and global.pl, because I have other changes in my
tree. Add -T, and use linb qw(.); on all files and enjoy!

This successfuly picks up one of the bugs recently found - haven't tested
others.

(I want to be able to check 'needs-work' when I attach the patch)...)

Comment 5

[Bradley Baetz (:bbaetz)]

This really is a 2.16 blocker. I just found another one....

I'm now doing: use re 'taint'; instead of manually tainting the stuff later.

Comment 6

[Gervase Markham [:gerv]]

+sub is_tainted {
+    return not eval { join('',@_), kill 0; 1; };
+}
+

I don't understand that. You should also add a note or URL to explain why
$taintedvar is tainted. Otherwise, looks good.

Gerv

Comment 7

[Bradley Baetz (:bbaetz)]

Comment on attachment 57344 [details] [diff] [review]
patch

I have a better patch, which I'll attach once its finished

Comment 8

[Bradley Baetz (:bbaetz)]

Attachment: sample

OK, attached are glob.pl and CGI.pl changes, plus a random changed file,
showing the sort of thing I've had to do wrt changing regexps to call teh
detainting functions.

Comment 9

[Dave Miller [:justdave]]

was just looking at this, what I see looks good, but based on comments there's
several other files that need things done to them before we can check this in,
right?

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: taint

OK, this is almost everything. I'm skipping edit*, because they're being
rewritten. The dependancy graph stuff fails because it calls unlink - someone
familiar with thta code needs to confirm that its ok, and work out how to avoid
the taint checks.

Comment 11

[Bradley Baetz (:bbaetz)]

Attachment: patch

err, um

diff _before_ attaching

Comment 12

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

Comment on attachment 59050 [details] [diff] [review]
patch

>Index: Attachment.pm

>-#!/usr/bonsaitools/bin/perl -w

Missing the replacement line.


>Index: Bug.pm

>-#!/usr/bonsaitools/bin/perl -w

Missing the replacement line.


>Index: CGI.pl

>-    my ($id) = @_;
>-
>-    # Make sure the bug number is a positive integer.
>-    # Whitespace can be ignored because the SQL server will ignore it.
>-    $id =~ /^\s*([1-9][0-9]*)\s*$/
>+    detaint_natural($_[0])
>       || DisplayError("The bug number is invalid. If you are trying to use " .
>                       "QuickSearch, you need to enable JavaScript in your " .
>                       "browser. To help us fix this limitation, look " .
>                       "<a href=\"http://bugzilla.mozilla.org/show_bug.cgi?id=70907\">here</a>.") 

This change tightens the requirements for the bug number (it no longer
can have whitespace before or after it).  Are there any scripts that rely
on the old behavior?


>Index: Token.pm

>-#!/usr/bonsaitools/bin/perl -w

Missing the replacement line.


>Index: globals.pl

>-        $::db = DBI->connect("DBI:mysql:host=$::db_host;database=$name", $::db_user, $::db_pass)
>+        $::db = DBI->connect("DBI:mysql:host=$::db_host;database=$name",
>+                             $::db_user, $::db_pass)
>             || die "Bugzilla is currently broken. Please try again later. " . 

Yuck.

>Index: process_bug.cgi

>+if (defined $::FORM{'id'}) {
>+    # Other places use it. Its already been checked in ValidateBugID, but
>+    # that was a copy, not the value itsself, which is still considered
>+    # tainted
>+    detaint_natural($::FORM{'id'})
>+      || die ("Bugzilla internal error - $::FORM{'id'} is a valid bug, but not a number??")
>+}
>+

ValidateBugID should de-taint the original then.


>Index: token.cgi

Missing the switch to taint mode.

Comment 13

[Bradley Baetz (:bbaetz)]

.pm's shouldn't have #!'s - thats not a bug

I'll fix the rest later

Comment 14

[Bradley Baetz (:bbaetz)]

Attachment: new patch

OK, fixes the problems mentioned.

Comment 15

[David D. Kilzer (ddk)]

>I don't think I can override DBI's die(), can I?

Yes, you can!  If you use an eval{}; statement around all of your
DBI calls, you trap all fatal DBI errors and can test $@ after
the statement for problems.  This is the preferred method of trapping
errors when $dbh->{RaiseError} is true.  (Haven't looked at patch
code yet.  Just answering a question I saw in the notes.)

Comment 16

[Bradley Baetz (:bbaetz)]

But RaiseError isn't set, is it?

Besides, there were other reasons not to use DBI's stuff (the output tainting we
don't want, mainly)

Comment 17

[Gervase Markham [:gerv]]

My one concern about this patch is the following:

+sub is_tainted {
+    return not eval { my $foo = join('',@_), kill 0; 1; };
+}
+
 sub SendSQL {
     my ($str, $dontshadow) = (@_);
+
+    # Don't use DBI's taint stuff yet, because:
+    # a) We don't want out vars to be tainted (yet)
+    # b) We want to know who called SendSQL...
+    # Is there a better way to do b?
+    if (is_tainted($str)) {
+        die "Attempted to send tainted string to the database";
+    }
+

What are the performance implications of executing is_tainted() on every SendSQL
call? AIUI, eval() isn't cheap. It doesn't fire off a new Perl interpreter, does
it?

Gerv

Comment 18

[Bradley Baetz (:bbaetz)]

Gerv: I doubt that the perf stuff is noticable - perl has to do it internally 
when running with -T anyway. I suspect its just a bit test. If b.m.o slows 
down, and taint mode is to blame then we can look at this again. The overhead 
is probably very very small compared with the db queries, anyway.

This isn't a feature - it is a security requirement. Turning this on found 
several security holes during my testing.

Comment 19

[Gervase Markham [:gerv]]

Comment on attachment 60556 [details] [diff] [review]
new patch

+sub is_tainted {
+    return not eval { my $foo = join('',@_), kill 0; 1; };
+}
+

Give this an explanatory comment (I still don't understand how it works ;-) and
r=gerv.

Gerv

Comment 20

[Bradley Baetz (:bbaetz)]

Attachment: better patch

OK, this patch now has tainting enabled for all non-edit* files, and modifies
the tests to check for the -T option where appropriate.

The only significant change is to showdependancygraph.cgi - can reviewers
please confirm that there is no way to force that to do something bad?

I'll put this up on landfill in a sec for people to play with.

Comment 21

[Bradley Baetz (:bbaetz)]

live at http://landfill.tequilarista.org/bz108982/

Please poke it, and see what breaks. I found another couple of things this
morning, where manually matching needed to be relaced with calls to detaint_natural.

Comment 22

[Bradley Baetz (:bbaetz)]

Attachment: oops

That was quick. Only check for a valid resolution when we use it, else you
can't reopen a bug (because that field isn't there then...)

Comment 23

[Dave Miller [:justdave]]

Comment on attachment 64704 [details] [diff] [review]
oops

oh so close!!!	I hate it when I have to mark a patch this big needs work for
something so minor...

There's a few spots where you added entire blocks of code and indented 2
characters instead of 4.  This doesn't match the style guidelines. :(

Most notably the if/then/else you added in t/002goodperl.t, but also in
globals.pl (the die line) and in Bug.pm (the "# no bug number given" comment
and the return statement at the end are not lined up with the two assignments)

Comment 24

[Bradley Baetz (:bbaetz)]

Attachment: new patch

fixed up indents; justdav says his r= carries through

/me wants to add xemacs modelines in for indent width ;)

Comment 25

[Bradley Baetz (:bbaetz)]

Comment on attachment 65341 [details] [diff] [review]
new patch

r=justdave:

<justdave> if you guarantee me that's all you changed, go for it :-)

Comment 26

[Dave Miller [:justdave]]

Comment on attachment 65341 [details] [diff] [review]
new patch

verifying my r= justdave, it's still good. :)

Comment 27

[Dave Miller [:justdave]]

Comment on attachment 65341 [details] [diff] [review]
new patch

revoking my review...  playing with it some more, managed to break it.	(Jake
did too, so I hear)

http://pismo/~dave/bugzilla/buglist.cgi?short_desc_type=allwordssubstr&short_de
sc=&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&b
ug_file_loc=&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&emailtype1=
exact&email1=&emailtype2=exact&email2=&bugidtype=include&bug_id=&votes=&changed
in=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&newqueryname=&order=R
euse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=

The above URL gets me this: 

Software error:
Attempted to send tainted string to the database at globals.pl line 215.

Comment 28

[Bradley Baetz (:bbaetz)]

Attachment: better patch

This wasn't originally taint-tested because doing so exposed bug 109679. When I
moved countries, I forgot about that, and so didn't pick up the patch I had in
my own tree.

The only change is to detaint the column name in buglist.cgi after the test has
been done.

Comment 29

[Bradley Baetz (:bbaetz)]

Attachment: works, too!

detaint the ORDER cookie too, else this will only work the first time

Comment 30

[Bradley Baetz (:bbaetz)]

Attachment: ...and again

This time without the debugging print statement left in

Comment 31

[Dave Miller [:justdave]]

Comment on attachment 65389 [details] [diff] [review]
...and again

r= justdave

really works this time

Comment 32

[Jacob Steenhagen]

Comment on attachment 65389 [details] [diff] [review]
...and again

This still die()s for me when it tries to run the query.  I tracked it down to
the fact that I have a COLUMNLIST cookie.  If you go to the Change Columns page
on the buglist and select a new column to display, it'll also die().

Comment 33

[Bradley Baetz (:bbaetz)]

Attachment: finally?

OK, I've fixed this - just neededed to trick_taint valid field names from the
cookie. What else could there be left...?

Comment 34

[Jacob Steenhagen]

Comment on attachment 65494 [details] [diff] [review]
finally?

Yippie
r=jake

Comment 35

[Dave Miller [:justdave]]

Comment on attachment 65494 [details] [diff] [review]
finally?

>Index: buglist.cgi
>@@ -783,6 +785,9 @@
>+                # This name has been checked
>+                trick_taint($f);

How did we check it?

>@@ -1067,7 +1072,8 @@
>-        push(@fields, "$::key{$c}");
>+        trick_taint($c);
>+        push(@fields, $::key{$c});

This one doesn't even have a comment.

>Index: showdependencygraph.cgi
>@@ -168,6 +170,8 @@
>     foreach my $f (glob("data/webdot/*.dot")) {
>+        # We created these, so detainting is OK
>+        trick_taint($f);

Are you sure?  What if someone created them locally?  In any case,
we're intending to nuke anything in the directory, so if the
filesystem says it's there it's probably safe to delete it.  Should
it be explained a little better?


OK, end of story here, and reason for my need-work is that I want to
satisfy any future code auditors that we really did think it through
before using trick_taint blindy.  Prove to them that we did the
research on it and thought it through completely without making them
do the research over again in order to figure it out themselves.

For example, for the one with $::key{$c} in buglist.cgi, a comment
along these lines:

# The value we are actually using is $::key{$c}, which was created
# using the DefCol() function earlier.	We test for the existance
# of $::needsquote{$c} to find out if $c is a legitimate key in the
# hashes that were defined by DefCol().  If $::needsquote{$c} exists,
# then $c is valid and we can use it to look up our key.
# If it doesn't exist, then we know the user is screwing with us   
# and we'll just skip it.

Comment 36

[Bradley Baetz (:bbaetz)]

Attachment: + comments

Comment 37

[Bradley Baetz (:bbaetz)]

Comment on attachment 65784 [details] [diff] [review]
+ comments

r=jake:

<Jake> bbaetz: If all you've done is add extra comments, my r= will carry over
:)

Comment 38

[Dave Miller [:justdave]]

Comment on attachment 65784 [details] [diff] [review]
+ comments

r= justdave

woohoo!

Comment 39

[Dave Miller [:justdave]]

Checked in.