Closed Bug 252638 Opened 20 years ago Closed 19 years ago

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

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: clklein, Assigned: myk)

Details

(Whiteboard: [fixed in 2.16.7] [fixed in 2.18rc3] [fixed in 2.19.1])

Attachments

(2 files, 3 obsolete files)

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",...
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.
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?
Flags: blocking2.18?
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)
Flags: blocking2.18?
Flags: blocking2.18+
Flags: blocking2.16.7+
Target Milestone: --- → Bugzilla 2.16
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()). 
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

Group: webtools-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attached patch patch v1: fixes problem? (obsolete) — Splinter Review
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.
Whiteboard: patch awaiting testing and review
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.
Attachment #158934 - Attachment is obsolete: true
Comment on attachment 159050 [details] [diff] [review]
patch v2: a better and more precise fix

Gerv, how does this look?
Attachment #159050 - Flags: review?(gerv)
Whiteboard: patch awaiting testing and review → patch awaiting review
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 on attachment 159050 [details] [diff] [review]
patch v2: a better and more precise fix

r=gerv. Looks good.

Gerv
Attachment #159050 - Flags: review?(gerv) → review+
Comment on attachment 159050 [details] [diff] [review]
patch v2: a better and more precise fix

r=gerv. Looks good.

Gerv
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?
Flags: approval?
Flags: approval2.18?
Whiteboard: patch awaiting review → [wanted for 2.16.7] [ready for 2.18rc3] [ready for 2.19.1]
Attached patch patch v2 for 2.16 (obsolete) — Splinter Review
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.
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.
Attachment #159534 - Attachment is obsolete: true
Attachment #159050 - Attachment is obsolete: true
Attachment #159536 - Attachment description: patch v3: a more appropriate ordering of conditions for special check → patch v3 (2.16): a more appropriate ordering of conditions for special check
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.
Attachment #159537 - Flags: review?(gerv)
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.
Attachment #159536 - Flags: review?(gerv)
Whiteboard: [wanted for 2.16.7] [ready for 2.18rc3] [ready for 2.19.1] → [awaiting review for 2.16.7] [ready for 2.18rc3] [ready for 2.19.1]
Attachment #159536 - Flags: review?(gerv) → review+
Attachment #159537 - Flags: review?(gerv) → review+
Attachment #159537 - Flags: review+ → review?(gerv)
Attachment #159537 - Flags: review?(gerv) → review+
Flags: approval2.16?
Whiteboard: [awaiting review for 2.16.7] [ready for 2.18rc3] [ready for 2.19.1] → [ready for 2.16.7] [ready for 2.18rc3] [ready for 2.19.1]
Blocks: 261303
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
Status: NEW → RESOLVED
Closed: 19 years ago
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval2.16?
Flags: approval2.16+
Flags: approval+
Resolution: --- → FIXED
Whiteboard: [ready for 2.16.7] [ready for 2.18rc3] [ready for 2.19.1] → [fixed in 2.16.7] [fixed in 2.18rc3] [fixed in 2.19.1]
advisory has posted, clearing security flag
Group: webtools-security
No longer blocks: 261303
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.