Closed
Bug 252638
Opened 19 years ago
Closed 19 years ago
Form param hacking allows unprivileged users to remove bug's keywords
Categories
(Bugzilla :: Creating/Changing Bugs, defect)
Bugzilla
Creating/Changing Bugs
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)
1.75 KB,
patch
|
gerv
:
review+
|
Details | Diff | Splinter Review |
1.96 KB,
patch
|
gerv
:
review+
|
Details | Diff | Splinter Review |
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",...
Reporter | ||
Comment 1•19 years ago
|
||
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?
Comment 3•19 years ago
|
||
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
Reporter | ||
Comment 4•19 years ago
|
||
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•19 years ago
|
||
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
Comment 6•19 years ago
|
||
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.
Assignee | ||
Comment 7•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Whiteboard: patch awaiting testing and review
Assignee | ||
Comment 8•19 years ago
|
||
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
Assignee | ||
Comment 9•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Whiteboard: patch awaiting testing and review → patch awaiting review
Assignee | ||
Comment 10•19 years ago
|
||
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•19 years ago
|
||
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 12•19 years ago
|
||
Comment on attachment 159050 [details] [diff] [review] patch v2: a better and more precise fix r=gerv. Looks good. Gerv
Comment 13•19 years ago
|
||
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]
Assignee | ||
Comment 14•19 years ago
|
||
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.
Assignee | ||
Comment 15•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #159534 -
Attachment is obsolete: true
Assignee | ||
Comment 16•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Attachment #159050 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
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
Assignee | ||
Comment 17•19 years ago
|
||
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)
Assignee | ||
Comment 18•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
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]
Updated•19 years ago
|
Attachment #159536 -
Flags: review?(gerv) → review+
Updated•19 years ago
|
Attachment #159537 -
Flags: review?(gerv) → review+
Updated•19 years ago
|
Attachment #159537 -
Flags: review+ → review?(gerv)
Updated•19 years ago
|
Attachment #159537 -
Flags: review?(gerv) → review+
Assignee | ||
Updated•19 years ago
|
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]
Comment 19•19 years ago
|
||
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]
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•