Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[security] fieldx-x-x values not validated

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
Query/Bug List
P1
blocker
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: bbaetz, Assigned: Dawn Endico)

Tracking

2.15
Bugzilla 2.16
x86
Linux

Details

(Whiteboard: included in 2.14.1, URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

16 years ago
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?
could look them up in the fieldid table to make sure they exist first before
adding them to the query.

Comment 2

16 years ago
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...
(Reporter)

Comment 3

16 years ago
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.
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
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.
Created attachment 61015 [details] [diff] [review]
Patch v1

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

Comment 6

16 years ago
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
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.
Created attachment 61048 [details] [diff] [review]
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.
Attachment #61015 - Attachment is obsolete: true

Comment 9

16 years ago
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
Attachment #61048 - Flags: review+
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
(Reporter)

Comment 11

16 years ago
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.
Attachment #61048 - Flags: review-
You tried everything else, so that's the only "exception" one?
(Reporter)

Comment 13

16 years ago
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 :)
Created attachment 61699 [details] [diff] [review]
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.
Attachment #61048 - Attachment is obsolete: true
Created attachment 61700 [details] [diff] [review]
Patch v4

fixes stupid syntax error....
Attachment #61699 - Attachment is obsolete: true
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

16 years ago
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).
Attachment #61700 - Flags: review+
Created attachment 61915 [details] [diff] [review]
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

16 years ago
Comment on attachment 61915 [details] [diff] [review]
Patch v5

:-)
r=timeless
Attachment #61915 - Flags: review+
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
&lt;109679@bugzilla.org&gt; 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
Attachment #61915 - Flags: review-
Comment on attachment 61700 [details] [diff] [review]
Patch v4

Generates error message similar to one from the v5 of this patch.
Attachment #61700 - Flags: review-
Created attachment 61925 [details] [diff] [review]
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.
Attachment #61700 - Attachment is obsolete: true
Attachment #61915 - Attachment is obsolete: true
Created attachment 61926 [details] [diff] [review]
Patch v7

Fix brainless typo
Attachment #61925 - Attachment is obsolete: true

Comment 24

16 years ago
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...
Attachment #61926 - Flags: review-
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.
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.
Look to the patches for bug 103778 to see how I reimplemented the error code in
the new buglist.cgi.

Comment 28

16 years ago
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 on attachment 61926 [details] [diff] [review]
Patch v7

Tested fine for functionality on both HEAD and the 2.14.1 branch. 
r=dkl
Attachment #61926 - Flags: review+

Comment 30

16 years ago
Comment on attachment 61926 [details] [diff] [review]
Patch v7

removing needs-work as per Jake's comments
Attachment #61926 - Flags: review-

Comment 31

16 years ago
I really did intend to check that box :)
Comment on attachment 61926 [details] [diff] [review]
Patch v7

r=gerv. This seems fine to me.

Gerv
Attachment #61926 - Flags: review+
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
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: included in 2.14.1
(Reporter)

Comment 34

16 years ago
2.14.1 is out; opening up fixed security bug
Group: security?
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.