Closed Bug 233962 Opened 20 years ago Closed 20 years ago

UserInGroup not backwards-compatible. possible security hole

Categories

(Bugzilla :: Bugzilla-General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: alin, Assigned: gerv)

References

Details

Attachments

(2 files)

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.
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.

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

Gerv
Attached patch Patch v.1Splinter Review
Is this what's wanted?

Gerv
Assignee: justdave → gerv
Status: UNCONFIRMED → ASSIGNED
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.

Gerv
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?
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)
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
Flags: documentation2.18?
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?
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?
Attachment #246032 - Flags: review? → review?(documentation)
Attachment #246032 - Flags: review?(documentation) → review+
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-
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: