252638 - Form param hacking allows unprivileged users to remove bug's keywords

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Casey Klein]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040615 Firefox/0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040615 Firefox/0.8

By tweaking the form parameters to process_bug.cgi, it is possible for a user
with no permission bits (including reporter or assignee status) to remove the
keywords from a bug. process_bug.cgi diffs the current value of each field from
@::log_columns against the corresponding form parameter, and if the field is
being modified, determines if the user is allowed to change that field. 

This works in most cases but ignores the effect of the 'keywordaction' field,
which may instruct the script to delete the specified keywords. This field is
only displayed on the "change multiple bugs" form, which requires edit-bugs
privs, but the parameter is still accessible by all users. URLs such as the
following allow any logged in user to delete any bug's keywords: 
/process_bug.cgi?id_<bug_id_to_change>=on&dontchange=--do_not_change--&product=--do_not_change--&keywords=<bug's_current_keywords>&keywordaction=delete&knob=none&component=--do_not_change--&version=<bug's_current_version>

Reproducible: Always
Steps to Reproduce:
1. Log in
2. Use above URL

Actual Results:  
Keywords were deleted

Expected Results:  
ThrowUserError("illegal_change",...

Comment 1

[Casey Klein]

The following parameter set also allows any user to delete a bug's keywords:
?process_bug=id_<bug_id_to_change>=on&dontchange=--do_not_change--&product=--do_not_change--&keywordaction=makeexact&knob=none&component=--do_not_change--&version=<bug's_current_version>

Because $::FORM{'keywords'} does not exists() above, the script doesn't make
sure the user has the proper privileges.

Comment 2

[GavinS]

I haven't confirmed this problem, and apologies if it has been discussed on IRC
or elsewhere, but 2 questions:

1) Should this be made a security issue until confirmed?

2) If confirmed, should it block 2.18?

Comment 3

[Dave Miller [:justdave]]

This is worth fixing in 2.16 if it's actually happening.  Anyone had a chance to
test on landfill yet?  (I can add privs if anyone needs them for testing)

Comment 4

[Casey Klein]

For the record, we're not even attempting to enforce any permissions on the CC
or dependency fields (according to the comment header above CheckCanChangeField()). 

Comment 5

[Gervase Markham [:gerv]]

Confirmed on both 2.18rc3 and 2.16.5.

Marking security-sensitive; we normally mark permissions problems as such, don't
we? Dave: uncheck if you disagree.

Casey: would you agree that a fix would be to have an additional list of fields
to check as well as @::log_columns? It could contain keywordaction to start
with, and any others we find.

Gerv

Comment 6

[Dave Miller [:justdave]]

Probably the best thing to do in this case is to specifically make the keywords
check run regardless of whether $new matches $old if keywordaction is set to
something other than donotchange...

Testing keywordaction itself probably wouldn't give us much, and would require
an additional test for keywords in CheckCanChangeField is somewhere were to
write keywords rules.

Comment 7

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

Attachment: patch v1: fixes problem?

Here's a patch that should fix both problems.  For the problem in comment 0, it
bypasses new==old checks when the keyword action isn't "makeexact" so that
CheckCanChangeField computes permissions for the "add" and "delete" keyword
actions (of which probably only the "delete" keyword action is dangerous).

For the problem in comment 1, it conditionally processes keyword changes only
if the "keywords" form field exists instead of unconditionally processing them
no matter what.  This is consistent with CheckCanChangeField, which only checks
permissions for form fields that exist.

I haven't tested the patch yet.  Also, passing $keywordaction to
CheckCanChangeField is hacky, so suggestions for improving that are welcome.

Comment 8

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

Attachment: patch v2: a better and more precise fix

This patch avoids hacky passing of $keywordaction and does a special check for
the case in question.  I also added a simple conditional to set $keywordaction
to the default if it isn't one of the acceptable values, since otherwise the
CGI accepts any value for that field, which could cause problems.

Comment 9

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

Comment on attachment 159050 [details] [diff] [review]
patch v2: a better and more precise fix

Gerv, how does this look?

Comment 10

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

Note that a more robust solution would be to do all the adding/deleting before
we reach CheckCanChangeField so that by the time we authorize the change we have
an exact string to which the keywords are being set and don't have to make a
funky CheckCanChangeField call for this special case.

That's more robust because it uses CheckCanChangeField exactly as intended and
doesn't special case this known vulnerability while potentially overlooking some
unknown vulnerability that may be lurking out there.  It's also significantly
more and riskier work.  So I suggest we go with the immediate fix for the known
vulnerability, at least for 2.18, and plan to do the more robust fix down the road.

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 159050 [details] [diff] [review]
patch v2: a better and more precise fix

r=gerv. Looks good.

Gerv

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 159050 [details] [diff] [review]
patch v2: a better and more precise fix

r=gerv. Looks good.

Gerv

Comment 13

[Dave Miller [:justdave]]

This patch doesn't apply cleanly to the 2.16 branch, and merging it doesn't
appear to be exactly straightforward...  anyone care to cook up something like
this for 2.16?

Comment 14

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

Attachment: patch v2 for 2.16

Here's a patch that applies to 2.16.  CheckCanChangeField generates the error
message on 2.16 instead of just passing back a boolean value, so we can't just
pass it two unequal values and then generate an appropriate error message when
it returns.  Instead, we have to send it the actual values to check, so I send
back the current keywords as the old value and an empty string as the new
value.

This makes the error message harder to read (because it tells you that you
tried to change the keywords from some value to an almost invisible space), but
it plugs the security hole, doesn't open up a new one (as it could if we were
to pass "no keywords" as the new value and an installation had "no" and
"keywords" keywords), and the error message is the same as the generic field
checking code generates for all fields.

Note that I avoid running the special check if the user selected the "delete"
keyword action but didn't enter any keywords to delete.  We shouldn't error out
in that case since the user isn't actually deleting any keywords.  I'll add
this to my tip patch as well.

Comment 15

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

Attachment: patch v3 (2.16): a more appropriate ordering of conditions for special check

This patch checks for the existence of the form field before checking to see if
there's anything in the keywordlist.  I could probably drop the former check,
since the latter assumes it, but let's not assume.

Comment 16

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

Attachment: patch v3 (tip): a more appropriate ordering of conditions for special check

Comment 17

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

Comment on attachment 159537 [details] [diff] [review]
patch v3 (tip): a more appropriate ordering of conditions for special check

Gerv, could you review this one-line addition to the patch you previously
reviewed?  See comment 15 and the last paragraph in comment 14 for the details
on the change.

Comment 18

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

Comment on attachment 159536 [details] [diff] [review]
patch v3 (2.16): a more appropriate ordering of conditions for special check

Gerv, can you review this version of the patch for 2.16?  Changes from the tip
version are minor and detailed in comment 14.

Comment 19

[Dave Miller [:justdave]]

Checked in on trunk:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.215; previous revision: 1.214
done

and 2.18 branch:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.205.2.5; previous revision: 1.205.2.4
done

and 2.16 branch:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.125.2.11; previous revision: 1.125.2.10
done

Comment 20

[Dave Miller [:justdave]]

advisory has posted, clearing security flag