Closed
Bug 233962
Opened 20 years ago
Closed 20 years ago
UserInGroup not backwards-compatible. possible security hole
Categories
(Bugzilla :: Bugzilla-General, defect)
Bugzilla
Bugzilla-General
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: alin, Assigned: gerv)
References
Details
Attachments
(2 files)
511 bytes,
patch
|
bbaetz
:
review+
justdave
:
review+
bugreport
:
review+
|
Details | Diff | Splinter Review |
1.25 KB,
patch
|
cso
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7 In 2.17.4, UserInGroup returned information about the "current" user if no second parameter was specified, else the specified user. In 2.17.6, UserInGroup only returns information about the "current" user and ignores any second parameter. In porting some of my custom code from 2.17.4 to 2.17.6 (but continuing to use UserInGroup), I now have security issues related to emailing. I have code in processmail which I cut & paste into BugMail.pm like: if ($diff->{'fieldname'} eq 'my_custom_field') { if (UserInGroup(Param("insidergroup"), $userid)) { $add_diff=1; } } Now, if I am an insider making the change, but there is somebody on the CC list who is NOT an insider, that non-insider is getting the email. Changing the signature of the UserInGroup method without at least throwing an warning if a second parameter is a potential security hole. Reproducible: Always Steps to Reproduce: 1. 2. 3. Expected Results: New method should throw a warning or something if a second parameter is passed.
Comment 2•20 years ago
|
||
This got changed when Bugzilla::User went in - you're meant to get a Bugzilla::User object instead. I changed all existing callers when that happened. I grepped through the current source code, and son't see any UserInGroup call with more than one param. Which line is this in?
Comment 3•20 years ago
|
||
He said in the original description that it's in customized code that he added (not necessarily what he did, but think about sub CheckCanChangeField in process_bug as an example of where people would do this). The suggestion is that we bail when the second parameter is passed in order to let people who customized that know that the syntax changed without it opening a hole on their site.
Comment 4•20 years ago
|
||
I guess. There are limits mind you; eg code ported from 2.16 won't be complient with the new groups restrictions. In that case the schema change will make it noticable, but that won't necessarily always be the case.
Assignee | ||
Comment 5•20 years ago
|
||
I think it's fair enough to have UserInGroup() die if it's passed a second parameter. But we don't want to go down the slope of documenting all the changes we make to the code to let people port customisations - they need to figure it out for themselves, or hire someone who can ;-) I don't think we need a security advisory and all that jazz for this. Gerv
Assignee | ||
Comment 6•20 years ago
|
||
Confirming; I suspect Unconfirmed security bugs aren't on Dave's radar. Gerv
Assignee | ||
Comment 7•20 years ago
|
||
Is this what's wanted? Gerv
Assignee | ||
Updated•20 years ago
|
Assignee: justdave → gerv
Status: UNCONFIRMED → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Attachment #142989 -
Flags: review?(bbaetz)
Comment 8•20 years ago
|
||
having the security flag set gets it on my radar. It wasn't confirmed yet because there was argument over whether it's really needed. But it won't hurt anything, and it could certainly help, so why not? :) I kinda like the idea.
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.18
Assignee | ||
Updated•20 years ago
|
Attachment #142989 -
Flags: review?(justdave)
Updated•20 years ago
|
Attachment #142989 -
Flags: review?(justdave) → review+
Comment 9•20 years ago
|
||
Any reason we need to hold this for a security advisory? Not sure if this even needs an advisory, as it would only affect code people have customized
Flags: blocking2.18+
Flags: approval?
Assignee | ||
Comment 10•20 years ago
|
||
I'd say no - people who hack Bugzilla around are responsible for their own security, when it comes down to it. We can mention it in the release notes. Gerv
Comment 11•20 years ago
|
||
Comment on attachment 142989 [details] [diff] [review] Patch v.1 I agree with no security advisory since there is no issue with Bugzilla as distributed. Why do we even have the security flag still set on this?
Comment 12•20 years ago
|
||
OK, lets get this checked in. Clearing security flag per last few comments. Lets make sure this gets a mention in the customizing section in the docs, too (nearby the CheckCanChangeField stuff would probably be a good spot for it, since that's where people are most likely to use it in that context on local hacks)
Group: webtools-security
Flags: documentation?
Flags: approval?
Flags: approval+
Comment 13•20 years ago
|
||
Comment on attachment 142989 [details] [diff] [review] Patch v.1 I think I've been outvoted. r=bbaetz
Attachment #142989 -
Flags: review?(bbaetz)
Updated•20 years ago
|
Blocks: bz-2.18-relnotes
Assignee | ||
Comment 14•20 years ago
|
||
Fixed. Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.262; previous revision: 1.261 done Gerv
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Flags: documentation2.18?
Comment 15•18 years ago
|
||
UserInGroup() no longer exists in 2.23.x. So documentation on tip doesn't need to be updated.
Flags: documentation?
Flags: documentation2.22?
Flags: documentation2.20?
Comment 16•18 years ago
|
||
As I said, tip doesn't need this patch, 2.20 and 2.22 only (for 2.18, we should say globals.pl instead of Bugzilla/User.pm).
Attachment #246032 -
Flags: review?
Updated•18 years ago
|
Attachment #246032 -
Flags: review? → review?(documentation)
Updated•18 years ago
|
Attachment #246032 -
Flags: review?(documentation) → review+
Comment 17•18 years ago
|
||
2.22.1: Checking in docs/xml/customization.xml; /cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v <-- customization.xml new revision: 1.21.2.6; previous revision: 1.21.2.5 done 2.20.3: Checking in docs/xml/customization.xml; /cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v <-- customization.xml new revision: 1.20.2.6; previous revision: 1.20.2.5 done 2.18.6: Checking in docs/xml/customization.xml; /cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v <-- customization.xml new revision: 1.12.2.12; previous revision: 1.12.2.11 done
Flags: documentation2.22?
Flags: documentation2.22+
Flags: documentation2.20?
Flags: documentation2.20+
Flags: documentation2.18?
Flags: documentation2.18+
Flags: documentation-
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
•