[security] users with blessgroupset!= 0 can change any groupset

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
User Accounts
P1
blocker
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: bbaetz, Assigned: bbaetz)

Tracking

2.15
Bugzilla 2.16

Details

(Whiteboard: applied to 2.14.1)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

16 years ago
Simply change the value of one of the form elements in editusers.pl to something
like:

value="-$fudge+9223372036854775807"

You can find out the value to plug in for $fudge by entering a bogus value, and
looking at the sql error. Stuff may be on the right, too, so take that into account.

We don't validate the entries against the blessgroupset, and we just read in the
raw values anyway.
(Assignee)

Updated

16 years ago
Summary: users with blessgroupset!= 0 can change any groupset → [security] users with blessgroupset!= 0 can change any groupset
Blocks: 103885
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16

Comment 1

16 years ago
Created attachment 56883 [details] [diff] [review]
Patch - run the form values through detaint_natural()

Comment 2

16 years ago
Comment on attachment 56883 [details] [diff] [review]
Patch - run the form values through detaint_natural()

I just realized my patch wasn't as good as I thought it was 'cause you can still just do
the math yourself and put that in the value :(
Attachment #56883 - Flags: review-
(Assignee)

Comment 3

16 years ago
whats the % $opblessgroupset stuff for?. It looks like we're taking away
anything we have the privs to set, and then adding anything. Can that just be
redone as:

           SendSQL("UPDATE profiles
                    SET groupset =
                         groupset - (groupset & $opblessgroupset) + ($groupset &
$opblessgroupset)
                    WHERE login_name=" . SqlQuote($userold));
           SendSQL("UPDATE profiles
                    SET groupset =
                         groupset - (groupset & $opblessgroupset) + $groupset
                    WHERE login_name=" . SqlQuote($userold));

instead?

You don't have to protect blessgroupset - only people with editusers can change
that. Of course, maybe we should only allow people with editusers to change hte
blessgroupset for stuff they have the blessgroupset for. Thats a policy
decision, and another bug, though. Currently editusers lets you set teh
blessgroupset to anything you want, which then lets you set the groupset to
whatever you want. This bug won't fix that.
(Assignee)

Comment 4

16 years ago
Ok, patch coming up. I didn't use detaint_natural for the reasons Jake mentioned.

Note that this doesn't stop the fact that, at present, editusers == admin access.

Assignee: myk → bbaetz
(Assignee)

Comment 5

16 years ago
Created attachment 56938 [details] [diff] [review]
patch
(Assignee)

Comment 6

16 years ago
Actually, I'll skip filing a bug on edit users being too broad, and wait to see
what happens when the groups stuff lands. Presumably only admins will be able to
set the isadmin bit. I'd like for editusers to only allow the user to change the
blessgroupset on groups for which they have a bless groupset. This would mean
changing the admin's blessgroupset in checksetup, but is probably easier when
groups stuff lands.
Status: NEW → ASSIGNED

Comment 7

16 years ago
Comment on attachment 56938 [details] [diff] [review]
patch

I don't know groupsets all that well, but I applied this patch and couldn't get the exploit
(or variations of it) to work, but I still could use it the way it's supposed to be used...

r=jake
Attachment #56938 - Flags: review+
Comment on attachment 56938 [details] [diff] [review]
patch

>Index: editusers.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v
>retrieving revision 1.28
>diff -u -r1.28 editusers.cgi
>--- editusers.cgi	2001/10/26 18:35:04	1.28
>+++ editusers.cgi	2001/11/07 21:57:18
>@@ -767,7 +767,8 @@
>         } else {
>            SendSQL("UPDATE profiles
>                     SET groupset =
>-                         groupset - (groupset & $opblessgroupset) + $groupset
>+                         groupset - (groupset & $opblessgroupset) + 
>+                         (($groupset) & $opblessgroupset)
>                     WHERE login_name=" . SqlQuote($userold));
> 
>            # I'm paranoid that someone who I give the ability to bless people

Hacking the form so one of the bit_ fields contains the value
"9223372036854775807)) , realname = ((1" still works with the latest patch.
These patches should be combined together to make sure all are really bits.
Attachment #56938 - Flags: review-
Created attachment 56979 [details] [diff] [review]
patch v3: combines the approaches of previous patches
(Assignee)

Comment 10

16 years ago
Comment on attachment 56979 [details] [diff] [review]
patch v3: combines the approaches of previous patches

r=bbaetz.

Between us all we have two reviews on this, I think...
Attachment #56979 - Flags: review+
Comment on attachment 56979 [details] [diff] [review]
patch v3: combines the approaches of previous patches

Agreed, since my patch is just the combination of your two, and I'll second r=myk each of yours.
Attachment #56979 - Flags: review+
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.29; previous revision: 1.28
done
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Created attachment 58205 [details] [diff] [review]
Same patch, against version 2.14

The removal of tabs threw off 'patch'.	This attachment applies cleanly to
2.14.
Attachment #56883 - Attachment is obsolete: true
Attachment #56938 - Attachment is obsolete: true
Attachment 58205 [details] [diff]:

/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.23.2.1; previous revision: 1.23
Whiteboard: applied to 2.14.1
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
Opening security bugs for which fixes have appeared in official bugzilla
release.  As per justdave and his posse.
Group: security?
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.