109679 - [security] fieldx-x-x values not validated

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

I found this while enabling taint mode.

The above url will show you the summary of the test security group bug on bmo.

Not sure how to fix this - we can't sqlquote it, because its a field name. Maybe
check for a lack of spaces?

Comment 1

[Dave Miller [:justdave]]

could look them up in the fieldid table to make sure they exist first before
adding them to the query.

Comment 2

[Jacob Steenhagen]

I don't think all possible fields exist in the fielddefs table, do they?  How do
we build this list in query.cgi?  We can just validate the field name using the
same (or similar) code (moving it to globals if needed).

Of course if it's built using the fielddefs table, then that's obviously the
right answer :)   Off to look at some code...

Comment 3

[Bradley Baetz (:bbaetz)]

I tihnk it is, but that will be really really slow (although we could read them
into a hash at the beginning, I guess).

Checking for spaces doesn't work, because "Days since bug changed" uses an sql
expression. Yuck.

Comment 4

[Dave Miller [:justdave]]

the field list in the popup is generated from the fielddefs table.  Just
validate it against that when it comes back.  No, fielddefs isn't necessarily
complete, but if it's not in fielddefs (with 1 or 2 obvious exceptions) it won't
be in the field popup in the boolean chart, either.

Comment 5

[Dave Miller [:justdave]]

Attachment: Patch v1

Not the ideal fix, but it's a quick hack, and it does seal up the security
hole...

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 61015 [details] [diff] [review]
Patch v1

This is, well...

Are you going to apply to the trunk?


>+    # get a list of field names to verify against
>+    my @chartfields;
>+    push(@chartfields, "noop");
>+    SendSQL("SELECT name FROM fielddefs ORDER BY sortkey");
>+    while (MoreSQLData()) {
>+        my ($name) = FetchSQLData();
>+        push(@chartfields, $name);
>+    }

Why ORDER BY? IF you want to, you could ORDER BY name, and then do a binary
search below, but I don't
know if its worth it.

> 
>     $row = 0;
>     for ($chart=-1 ;
>          $chart < 0 || exists $F{"field$chart-0-0"} ;
>-         $chart++) {
>+         $chart++)
>+    {


(random brace changes)

I'll test later

Comment 7

[Dave Miller [:justdave]]

You're right, the ORDER BY isn't necessary.  I copied the SQL from the code that
creates that popup menu to begin with, it used it, but that's probably so the
items in the popup menu would be in the right order when displayed on the page.

The brace changes were just because I was touching a block of code, so I made
the whole block comply with the style guidelines.

Yes, it was intended for checkin both on 2.14.1 and the trunk.

Comment 8

[Dave Miller [:justdave]]

Attachment: Patch v2

Cleaner code.  Hashes the legal values instead of using an array.  This way you
can simple check if it exists instead of having to grep for it.

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 61048 [details] [diff] [review]
Patch v2

Looks good. Verified that self-made SQL cannot be inserted into query string
without generating an error.
r=dkl

Comment 10

[Dave Miller [:justdave]]

Hmm, it seems the bulk change thinks I'm not changing anything if all I do is
add names to the CC list, so I guess I have to make a comment.  Anyhow, adding
the representatives from the organizations we know of that support Bugzilla
distributions so they're aware of our upcoming security release

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 61048 [details] [diff] [review]
Patch v2

Just to mentiuon what we discussed on irc: this doesn't work with commetn
seraches, because either long_desc or longdesc is valid, and the fielddefs
table only contains longdesc. You probably need to manually push that.

Comment 12

[Dave Miller [:justdave]]

You tried everything else, so that's the only "exception" one?

Comment 13

[Bradley Baetz (:bbaetz)]

I tried most stuff, but obviously I didn't try every possible combination. I did
browse the source code, so I think so. I think we should get this (and the
others) on b.m.o asap, and see who complains, then release in a couple of days.
Add a comment to the error which says "please report your query ASAP to
#mozwebtools", or something.

About 30 seconds would have been enough for the comment one :)

Comment 14

[Dave Miller [:justdave]]

Attachment: Patch v3

Adds the long_desc field to the valid list.  Also adds a comment in the error
to mail the buglist url to us if they didn't do it on purpose.

Comment 15

[Dave Miller [:justdave]]

Attachment: Patch v4

fixes stupid syntax error....

Comment 16

[Dave Miller [:justdave]]

For the record, a review on this patch doesn't necessarily mean this is the best
way to do this....  this is a security fix that we need to get out ASAP.  A
positive review on this patch needs only indicate the following things:

1) Most importantly, that it seals the security hole
2) It doesn't break any existing functionality

If the above two are accomplished, I will check this in as-is, rather that
trying to do it in a "better" way.  We can fix it correctly for 2.16 or 2.18. 
But we need to get the 2.14.1 update out ASAP.

Comment 17

[timeless]

Comment on attachment 61700 [details] [diff] [review]
Patch v4

r=timeless, with the following caveat: branches should receive the minimalist
safe changes to solve the problem. This excludes generic style assertions (as
is the case here) unless the old style is actually risky (this is not the
case).

Comment 18

[Dave Miller [:justdave]]

Attachment: Patch v5

Revised patch, same as v4, but intended for the 2.14.1 branch only.  v4 can
still be applied to the trunk.	Only difference is the lack of brace changes. 
I agree with timeless, shouldn't make style changes on a security-only update
to an existing release.

Comment 19

[timeless]

Comment on attachment 61915 [details] [diff] [review]
Patch v5

:-)
r=timeless

Comment 20

[David Lawrence [:dkl]]

Comment on attachment 61915 [details] [diff] [review]
Patch v5

The v5 patch generates an error for me on both TRUNK and the 2.14.1 branch. 

Internal error: Can't use longdescnames_.login_name as a field name. If you
think you're getting this in error, please copy the entire URL out of the
address bar at the top of your browser window and email it to
<[e-mail]> at /var/www/bugzilla/mozilla-2.14.1/buglist.cgi
line 727.

http://bugzilla.redhat.com/mozilla-2.14.1/buglist.cgi?bug_status=NEW&bug_status
=ASSIGNED&bug_status=REOPENED&email1=dkl%40redhat.com&emailtype1=substring&emai
lassigned_to1=1&emailreporter1=1&emailcc1=1&emaillongdesc1=1&email2=&emailtype2
=substring&emailreporter2=1&bugidtype=include&bug_id=&changedin=&votes=&chfield
from=&chfieldto=Now&chfieldvalue=&product=TestProduct&short_desc=&short_desc_ty
pe=allwordssubstr&long_desc=&long_desc_type=allwordssubstr&bug_file_loc=&bug_fi
le_loc_type=allwordssubstr&field0-0-0=noop&type0-0-0=noop&value0-0-0=&cmdtype=d
oit&newqueryname=&order=Reuse+same+sort+as+last+time

Comment 21

[David Lawrence [:dkl]]

Comment on attachment 61700 [details] [diff] [review]
Patch v4

Generates error message similar to one from the v5 of this patch.

Comment 22

[Dave Miller [:justdave]]

Attachment: Patch v6

OK, change in design here...  we're finding too many conflicts with internal
values...  and we don't need to keep adding the internal values to the list of
things that are okay...  the trick is that we only need to validate the values
passed from the query form, not the internally generated values.  All of the
internally generated stuff is shoved into chart #-1 in the form hash before the
form hash is processed for chart values.  So when we're doing our validation,
we'll just blindly accept anything in chart #-1.  To guarantee the user didn't
stuff something in chart #-1 on us, anything in chart #-1 is deleted before we
stuff the internally generated stuff in it.

Comment 23

[Dave Miller [:justdave]]

Attachment: Patch v7

Fix brainless typo

Comment 24

[Jacob Steenhagen]

Comment on attachment 61926 [details] [diff] [review]
Patch v7

>Index: buglist.cgi
...
>+                    die "Internal error: $errstr" if $chart < 0;
>+                    return Error($errstr);

I'm assuming the reason we die if $chart < 0 otherwise we "return Error()" is
that the HTML header is incomplete when we're called the first time, but has
been sent by the time this loop is run the second time.  Perhaps a comment
stating this (or the real reason if I'm off base :) would be a good idea...

Comment 25

[David Lawrence [:dkl]]

The new patch checks out fine on both BUGZILLA-2_14_1-BRANCH and TRUNK 
for me. I will check back to see if there is a new one with the suggested change by 
Jake soon and wait til then to check off as reviewed.

Comment 26

[Dave Miller [:justdave]]

Jake: I have no idea why it's using 'die' there.  I copied existing
error-spewing code from elsewhere within the chart matrix, assuming anything
that errors out of the chart should error the same way.  I wasn't trying to
change that with this patch.

Comment 27

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

Look to the patches for bug 103778 to see how I reimplemented the error code in
the new buglist.cgi.

Comment 28

[Jacob Steenhagen]

Comment on attachment 61926 [details] [diff] [review]
Patch v7

Given the explination about why the error handling code that, I'll recind my
needs-work.  Still haven't tested it, so I can't give it an r=...

Comment 29

[David Lawrence [:dkl]]

Comment on attachment 61926 [details] [diff] [review]
Patch v7

Tested fine for functionality on both HEAD and the 2.14.1 branch. 
r=dkl

Comment 30

[Zach Lipton [:zach]]

Comment on attachment 61926 [details] [diff] [review]
Patch v7

removing needs-work as per Jake's comments

Comment 31

[Jacob Steenhagen]

I really did intend to check that box :)

Comment 32

[Gervase Markham [:gerv]]

Comment on attachment 61926 [details] [diff] [review]
Patch v7

r=gerv. This seems fine to me.

Gerv

Comment 33

[Dave Miller [:justdave]]

checked in on the 2.14.1 branch:

/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.139.2.2; previous revision: 1.139.2.1

and on the trunk:

/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.155; previous revision: 1.154

Comment 34

[Bradley Baetz (:bbaetz)]

2.14.1 is out; opening up fixed security bug