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

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

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.

Comment 1

[Jacob Steenhagen]

Attachment: Patch - run the form values through detaint_natural()

Comment 2

[Jacob Steenhagen]

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 :(

Comment 3

[Bradley Baetz (:bbaetz)]

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.

Comment 4

[Bradley Baetz (:bbaetz)]

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.

Comment 5

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 6

[Bradley Baetz (:bbaetz)]

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.

Comment 7

[Jacob Steenhagen]

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

Comment 8

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: combines the approaches of previous patches

Comment 10

[Bradley Baetz (:bbaetz)]

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

Comment 11

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.29; previous revision: 1.28
done

Comment 13

[Dave Miller [:justdave]]

Attachment: Same patch, against version 2.14

The removal of tabs threw off 'patch'.	This attachment applies cleanly to
2.14.

Comment 14

[Dave Miller [:justdave]]

Attachment 58205 [details] [diff]:

/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.23.2.1; previous revision: 1.23

Comment 15

[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 16

[Mike Shaver (:shaver emeritus)]

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