208847 - Taint issues in editgroups.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Jason Pyeron]

User-Agent:       Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0; .NET CLR 1.0.3705)
Build Identifier: 

Several SendSql calls use tainted values, this is an issue when the webserver 
calls all perl cgi with -T

see patch below


PS: Sorry for the improper filling steps here but I am on the road, and in a 
hurry. I will do all the pre file work in 8 hours.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Jason Pyeron]

Attachment: fixes taints

also fixes $::FORM{'isactive'} undef issue of submit on no value

Comment 2

[Dave Miller [:justdave]]

Although I would have rather gotten it templatized than just fixing the taint
errors outright....  setting dependency appropriately

Comment 3

[Dave Miller [:justdave]]

taintedness blocks win32, lack of templates doesn't.  we'll take what we can get
for now. :)

Comment 4

[Jouni Heikniemi]

JPyeron, what are your plans about this? Are you going to make a new patch or 
ask for review on the existing one? Some comments on it anyway:

- I wouldn't bother with so many comments explaining the issue and containing 
bug number references. That's overkill, since the changes are almost trivial 
anyway. Cvs annotate will help you dig up the change information anyway.

- Couldn't you handle the numbers with detaint_natural?

>+    # if the checkbox is unchecked, then $::FORM{'isactive'} is undef.
>+    # [e-mail] http://bugzilla.mozilla.org/show_bug.cgi?
>     # should file a seperate bug
>+    $::FORM{'isactive'}="" if (!defined($::FORM{'isactive'}));
  
Yeah, probably should. And maybe even something like $::FORM{'isactive'} 
||= "";, if that's the right fix (instead of changing the html form).

Comment 5

[Jason Pyeron]

*** Bug 223704 has been marked as a duplicate of this bug. ***

Comment 6

[Jason Pyeron]

Attachment: fixes taints

Comment 7

[Jason Pyeron]

if any one else wants to remove the comments do so,

I dont have time for that right now.

I am done on this unless there is an IMPORTANT issue.

Comment 8

[Joel Peshkin]

Comment on attachment 134151 [details] [diff] [review]
fixes taints

This could be done by putting a single call to detaint_natural($::FORM{group});
before the first statement conditional on $action.

Also, I believe that the correct change is....

my $isactive = $::FORM{isactive} || 0;

Since it is defined as "tinyint not null default 1"

Comment 9

[Jason Pyeron]

Attachment: fixes taints

re:

> Also, I believe that the correct change is....
> 
> my $isactive = $::FORM{isactive} || 0;
> 
> Since it is defined as "tinyint not null default 1"

what is in the patch is not incorrect nut your suggesstion my be more correct.
that change would need investigation time which we dont have, hence it is not
going to get done by us right now and the gross adjustment is needed to get it
WORKING.

Comment 10

[Jason Pyeron]

joel re Comment #8: I have created bug 223740 for the isactive issue

Comment 11

[chaduv]

Comment on attachment 134151 [details] [diff] [review]
fixes taints

Remove request on obsolete patch.

Comment 12

[Jouni Heikniemi]

Comment on attachment 134183 [details] [diff] [review]
fixes taints

There are several issues with this:

1) It gives out warnings like "[Tue Nov 4 22:56:16 2003] editgroups.cgi: Use of
uninitialized value in pattern match (m//) at Bugzilla/Util.pm line 56."

2) 
>+# [e-mail] see bug 223704. http://bugzilla.mozilla.org/show_bug.cgi?id=223704
>+# check the diffs, I have several taint issues. mostly with $::FORM{stuff}, and $gid's

I still think (see comment 4) that these comments are not necessary.


3) I still get taint errors after applying this patch. At least when adding new
groups.

>+    # if the checkbox is unchecked, then $::FORM{'isactive'} is undef.
>+    # [e-mail] see bug 223704. http://bugzilla.mozilla.org/show_bug.cgi?id=223704
>+    $::FORM{'isactive'}="" if (!defined($::FORM{'isactive'}));

4) This issue was filed as a separate bug, so it shouldn't be in this patch.

Comment 13

[Dave Miller [:justdave]]

*** Bug 223740 has been marked as a duplicate of this bug. ***

Comment 14

[:glob ✱]

what do you think about a "shotgun" approach?

ie.  calling trick_taint($str) in Bugzilla::DB::SendSQL ?

Comment 15

[Jouni Heikniemi]

If we wanted to discard taint mode, we could do so with a considerably less
confusing solution.

Comment 16

[:glob ✱]

Attachment: fix taint issues

ignore my previous comment, this appears to do the job.

Comment 17

[:glob ✱]

Comment on attachment 148840 [details] [diff] [review]
fix taint issues

oops, forgot about add.

Comment 18

[:glob ✱]

Attachment: fix taint issues

Comment 19

[Jouni Heikniemi]

Something really strange just happened... I was submitting a negative review,
got a login prompt and a 500 error thereafter; then resubmitted. The negative
review got recorded on the wrong attachment (attachment 148841 [details] [diff] [review] instead of
attachment 148840 [details] [diff] [review]). Also, I seem to have set obsolete flag to 0. Hmm... This
reminds me of bug 223878, but I'm not sure. I'll ask Dave to check for bmo error
log. 

Meanwhile, a negative review still stands. My bug comment got wasted once, so
here's a short recap: the idea is not to make the error messages disappear, but
rather make the potential insecurity to disappear. I'm not convinced
trick_tainting here (without validation) is ok.

Comment 20

[:glob ✱]

Attachment: fix taint issues

ok, this patch fixes rather than hides the issue.

Comment 21

[Joel Peshkin]

Comment on attachment 148848 [details] [diff] [review]
fix taint issues

Rather than SqlQuote-ing the number everywhere, calling detaint_natural on the
number right after it is pulled from the form will work.  That requires changes
in only a few places.

Comment 22

[Joel Peshkin]

Try something like ....  (natursally, making sure I didn't miss a spot :-)

Index: editgroups.cgi
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v
retrieving revision 1.34
diff -u -r1.34 editgroups.cgi
--- editgroups.cgi      27 Mar 2004 04:35:37 -0000      1.34
+++ editgroups.cgi      20 May 2004 16:24:24 -0000
@@ -1,4 +1,4 @@
-#!/usr/bin/perl -w
+#!/usr/bin/perl -wT
 # -*- Mode: perl; indent-tabs-mode: nil -*-
 #
 # The contents of this file are subject to the Mozilla Public
@@ -330,6 +330,7 @@
     # (this occurs when the inactive checkbox is not checked
     # and the browser does not send the field to the server)
     my $isactive = $::FORM{isactive} || 0;
+    detaint_natural($isactive);

     unless ($name) {
         ShowError("You must enter a name for the new group.<BR>" .
@@ -406,6 +407,7 @@
 if ($action eq 'del') {
     PutHeader("Delete group");
     my $gid = trim($::FORM{group} || '');
+    detaint_natural($gid);
     unless ($gid) {
         ShowError("No group specified.<BR>" .
                   "Click the <b>Back</b> button and try again.");
@@ -503,6 +505,7 @@
 if ($action eq 'delete') {
     PutHeader("Deleting group");
     my $gid = trim($::FORM{group} || '');
+    detaint_natural($gid);
     unless ($gid) {
         ShowError("No group specified.<BR>" .
                   "Click the <b>Back</b> button and try again.");
@@ -713,6 +716,7 @@
 # Helper sub to handle the making of changes to a group
 sub doGroupChanges {
     my $gid = trim($::FORM{group} || '');
+    detaint_natural($gid);
     unless ($gid) {
         ShowError("No group specified.<BR>" .
                   "Click the <b>Back</b> button and try again.");

Comment 23

[Jouni Heikniemi]

Comment on attachment 148848 [details] [diff] [review]
fix taint issues

Per Joel's comment 21.

Comment 24

[:glob ✱]

Attachment: fix taint issues

Comment 25

[Jouni Heikniemi]

Comment on attachment 149030 [details] [diff] [review]
fix taint issues

The Bugzilla 2.16 conversion functions don't work; they still have tainted
group id's.

Comment 26

[:glob ✱]

Attachment: detaint

Comment 27

[Jouni Heikniemi]

Comment on attachment 149033 [details] [diff] [review]
detaint

r=jouni

Comment 28

[Dave Miller [:justdave]]

Comment on attachment 149033 [details] [diff] [review]
detaint

>+++ editgroups.cgi	2004-05-21 21:34:06.109249600 +0800
>@@ -373,7 +374,7 @@
>-            $isactive . ", NOW())" );
>+            SqlQuote($isactive) . ", NOW())" );

$isactive is an integer, we shouldn't be quoting it.  Just do a detaint_natural
on it beforehand.

In fact, rather than detaint_natural, since it's a boolean, let's just do
something like this:

-    my $isactive = $::FORM{isactive} || 0;
+    my $isactive = $::FORM{isactive} ? 1 : 0;

which avoids the whole issue :)

Comment 29

[Dave Miller [:justdave]]

(In reply to comment #28)
> +    my $isactive = $::FORM{isactive} ? 1 : 0;

And if we do it this way, it makes this chunk of code unnecessary:

    if ($isactive != 0 && $isactive != 1) {
        ShowError("The active flag was improperly set.  There may be " .
                  "a problem with Bugzilla or a bug in your browser.<br>" .
                  "Please click the <b>Back</b> button and try again.");
        PutFooter();
        exit;
    }

Comment 30

[:glob ✱]

Attachment: detaint editgroups v9

Comment 31

[Jouni Heikniemi]

Comment on attachment 149051 [details] [diff] [review]
detaint editgroups v9

r=jouni

Comment 32

[Jouni Heikniemi]

Ok, checked in. Thanks for the patch, Byron!

Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.35; previous revision: 1.34
done