UserInGroup not backwards-compatible. possible security hole

RESOLVED FIXED in Bugzilla 2.18



15 years ago
6 years ago


(Reporter: A. Lin, Assigned: gerv)


Bugzilla 2.18
Bug Flags:
approval +
blocking2.18 +
documentation -
documentation2.22 +
documentation2.20 +
documentation2.18 +



(2 attachments)



15 years ago
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 like:

if ($diff->{'fieldname'} eq 'my_custom_field') {
    if (UserInGroup(Param("insidergroup"), $userid)) {

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:

Expected Results:  
New method should throw a warning or something if a second parameter is passed.
CCing the usual security folks
Group: webtools-security
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?
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.
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.
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.

Confirming; I suspect Unconfirmed security bugs aren't on Dave's radar.

Created attachment 142989 [details] [diff] [review]
Patch v.1

Is this what's wanted?

Assignee: justdave → gerv
Attachment #142989 - Flags: review?(bbaetz)
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
Attachment #142989 - Flags: review?(justdave)
Attachment #142989 - Flags: review?(justdave) → review+
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?
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.


Comment 11

14 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

Why do we even have the security flag still set on this?
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 on attachment 142989 [details] [diff] [review]
Patch v.1

I think I've been outvoted. r=bbaetz
Attachment #142989 - Flags: review?(bbaetz)

Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision: 1.262; previous revision: 1.261

Last Resolved: 14 years ago
Resolution: --- → FIXED
Flags: documentation2.18?

Comment 15

12 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

12 years ago
Created attachment 246032 [details] [diff] [review]
documentation patch for 2.20 and 2.22, v1

As I said, tip doesn't need this patch, 2.20 and 2.22 only (for 2.18, we should say instead of Bugzilla/
Attachment #246032 - Flags: review?


12 years ago
Attachment #246032 - Flags: review? → review?(documentation)


12 years ago
Attachment #246032 - Flags: review?(documentation) → review+

Comment 17

12 years ago

Checking in docs/xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision:; previous revision:


Checking in docs/xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision:; previous revision:


Checking in docs/xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision:; previous revision:
Flags: documentation2.22?
Flags: documentation2.22+
Flags: documentation2.20?
Flags: documentation2.20+
Flags: documentation2.18?
Flags: documentation2.18+
Flags: documentation-
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.