382056 - [SECURITY] Bugzilla::Webservice::Bug->get_bugs() doesn't check if the user is in the timetracking group when returning data

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Frédéric Buclin]

While playing with contrib/bz_webservice_demo.pl, I realized that time related data are available even if you are logged out or logged in but not in the timetracking group. So it allows me to bypass some sec checks.

Comment 1

[Max Kanat-Alexander]

Arrrrgh. Okay, yes, that's a blocker.

Comment 2

[Dave Miller [:justdave]]

since we have multiple places retrieving such data nowadays, shouldn't the perl modules (Bugzilla::Bug, etc) enforce this kind of stuff by not returning such data if the logged in user doesn't have access?  I suppose there are places that need to be able to get at it anyway, so perhaps some sort of override flag for when you really want it, but that way it makes it obvious you're overriding the privs, too.

Comment 3

[Max Kanat-Alexander]

I think that's actually a good idea. But it'd be the kind of change that can't go into a branch. So we'd have to have a basic fix for the branch, and a comprehensive fix for upstream.

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

I just did a test or two and it seems to work fine so far. I will let it applied for a few days on my local installation to see if it regresses anything or not.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 268695 [details] [diff] [review]
patch, v1

Hrm...yeah, this *probably* won't regress anything, but it's hard to tell.

Eventually we'll have to do this for UPDATE_COLUMNS as well.

I don't really like entirely removing the information from the bug object, because I'm just worried about what that will do, but I suppose if it works fine and you don't find any regressions anywhere, it's OK.

Although for the branches, I'd rather go with a more direct method of fixing the WebService call itself.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 268695 [details] [diff] [review]
patch, v1

At first I thought this would break Bugzilla::Bug->update, but then I realized that it's going to be comparing against a "new Bugzilla::Bug" that *also* doesn't have these fields, so it won't update them.

I'd rather see a more direct approach for the branches--I'm not thrilled with the idea of committing this patch to a branch.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> I'd rather see a more direct approach for the branches--I'm not thrilled with
> the idea of committing this patch to a branch.

If you have another idea for the 3.0 branch, feel free to attach a patch here.

Comment 8

[Frédéric Buclin]

OK, this patch breaks at least process_bug.cgi on tip (didn't check yet on 3.0.1):

invalid bug attribute remaining_time at Bugzilla/Bug.pm line 2944
Bugzilla::Bug::AUTOLOAD('Bugzilla::Bug=HASH(0x8ceddc4)') called at Bugzilla/Bug.pm line 2051
Bugzilla::Bug::check_status_change_triggers('Bugzilla::Bug', 'CLOSED', 'ARRAY(0x8a989b8)', 'HASH(0x8c33d40)') called at /var/www/html/bugzilla/process_bug.cgi line 847

We still need to access the remaining_time field for unprivileged users in case we have to reset the remaining time. So we have to find something else.

Comment 9

[Frédéric Buclin]

Attachment: patch, v2

This patch is by far much safer than my previous fix as it only affects WebService/Bug.pm. There is no risk to regress anything in the core Bugzilla code. I tested it on both 3.0.1 and 3.1.1.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 274270 [details] [diff] [review]
patch, v2

The fields only show up in {internals}, and you didn't modify {internals}.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 274270 [details] [diff] [review]
patch, v2

Oh, nevermind. :-) That's fine.

I'd recommend just deleting the fields from the bug, because that would make more sense to the user (and be less confusing if he was trying to figure out why he couldn't see them). But this is OK as it is, too.

Comment 12

[Frédéric Buclin]

Attachment: patch, v2.1

OK, we can do that too, if you prefer.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 274273 [details] [diff] [review]
patch, v2.1

Yeah, I like that better. :-) Thanks. :-)

Comment 14

[Frédéric Buclin]

tip:

Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.6; previous revision: 1.5
done

3.0:

Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.4.2.2; previous revision: 1.4.2.1
done

Comment 15

[Max Kanat-Alexander]

Security advisory from Bug 389843 sent, unlocking bug.