108385 - [security] Possible to add comments to a bug as someone else

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

[filed in security group, if you're not there, you won't get mail until bmo is
upgraded, and I hope that this is fixed by then, anyway. myk, if you disagree,
then open it up]

So, theres this form element, $::FORM{'who'}. And its used as the "comment added
by" user. Problem is, the only check done is that the user exists in the db (by
quietly_check_login). theres no check that I am that actual user.

Why is this there? Obsolete, or does it have some purpose? I couldn't see where
we ever add an input element with a name of 'who'. If the form element isn't
there, we use the logged in user's id to grab the name.

Its easy to check that the username matches the logged in user, but I wonder if
it may be better to get rid of this stuff entirely. I need to find out why its
there first, though...

A user can manually edit the bugs page to spoof the comment - I'll comment as
[e-mail] in a sec.

The only other thing which can be done by exploiting this is to change who
processmail thinks did the change, so if excludeSelf is set in the mail prefs,
you can make someone not get mail on a change you make. That ones minor, though.

Comment 1

[Nobody; OK to take it and work on it]

Hi, I'm nobody.

Comment 2

[Jacob Steenhagen]

My biggest question with this would be, is there any reason to ever need to call
AppendComment() to add a comment as a user other than the currently logged in
user?  If not, I think we need to just use $::userid from within the
AppendComment() routine rather than allow $who to be passed in.

We also need to get rid of the $::FORM{'who'} getting passed on the command line
of processmail.

Comment 3

[Dave Miller [:justdave]]

And note that sice b.m.o hasn't picked up the email/buglist for CC's on
confidential bugs stuff yet, most of the people on the CC list here have never
seen an email about this bug (myself included).  I only found it because it got
mentioned on the b.m.o upgrade bug.

Comment 4

[Dave Miller [:justdave]]

Attachment: Patch v1 - replace all $::FORM{'who'} with $::COOKIE{'Bugzilla_login'}

Comment 5

[Jacob Steenhagen]

Comment on attachment 56552 [details] [diff] [review]
Patch v1 - replace all $::FORM{'who'} with $::COOKIE{'Bugzilla_login'}

It's identical to the patch I was about to attach after it was decided that we did
need the ability to add a comment as a user that isn't logged in (for bug moving
amoung other things).

r=jake

Comment 6

[Jacob Steenhagen]

-> Patch author

Comment 7

[Dave Miller [:justdave]]

/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.104; previous revision: 1.103

Comment 8

[Dave Miller [:justdave]]

This applied to the 2.14.1 branch with no changes.

/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.96.2.1; previous revision: 1.96

Comment 9

[Zach Lipton [:zach]]

shouldn't this bug be closed out now?

Comment 10

[Dave Miller [:justdave]]

read the status.  it IS resolved.  I didn't reopen it, I just commented that the
patches had been checked into the 2.14.1 branch.

Comment 11

[Dave Miller [:justdave]]

Testing, ignore this.

Comment 12

[Dave Miller [:justdave]]

Hmm, it seems the bulk change thinks I'm not changing anything if all I do is
add names to the CC list, so I guess I have to make a comment.  Anyhow, adding
the representatives from the organizations we know of that support Bugzilla
distributions so they're aware of our upcoming security release

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Opening security bugs for which fixes have appeared in official bugzilla
release.  As per justdave and his posse.