Closed Bug 382056 Opened 13 years ago Closed 13 years ago

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

Categories

(Bugzilla :: WebService, defect)

2.23.3
defect
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: LpSolit, Assigned: LpSolit)

References

Details

(Whiteboard: [ready for 3.0.1][ready for 3.1.1])

Attachments

(1 file, 2 obsolete files)

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.
Flags: blocking3.0.1?
Arrrrgh. Okay, yes, that's a blocker.
Flags: blocking3.0.1? → blocking3.0.1+
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.
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.
Attached patch patch, v1 (obsolete) — Splinter Review
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.
Assignee: webservice → LpSolit
Status: NEW → ASSIGNED
Attachment #268695 - Flags: review?(mkanat)
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 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.
Attachment #268695 - Flags: review?(mkanat) → review+
(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.
Flags: approval?
Whiteboard: [ready for 3.1][patch needed for 3.0?]
Blocks: 389843
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.
Flags: approval?
Whiteboard: [ready for 3.1][patch needed for 3.0?]
Attachment #268695 - Flags: review-
Attached patch patch, v2 (obsolete) — Splinter Review
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.
Attachment #268695 - Attachment is obsolete: true
Attachment #274270 - Flags: review?(mkanat)
Comment on attachment 274270 [details] [diff] [review]
patch, v2

The fields only show up in {internals}, and you didn't modify {internals}.
Attachment #274270 - Flags: review?(mkanat) → review-
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.
Attachment #274270 - Flags: review- → review+
Attached patch patch, v2.1Splinter Review
OK, we can do that too, if you prefer.
Attachment #274270 - Attachment is obsolete: true
Attachment #274273 - Flags: review?(mkanat)
Comment on attachment 274273 [details] [diff] [review]
patch, v2.1

Yeah, I like that better. :-) Thanks. :-)
Attachment #274273 - Flags: review?(mkanat) → review+
Flags: approval?
Flags: approval3.0?
Whiteboard: [ready for 3.0.1][ready for 3.1.1]
Summary: Bugzilla::Webservice::Bug->get_bugs() doesn't check if the user is in the timetracking group when returning data → [SECURITY] Bugzilla::Webservice::Bug->get_bugs() doesn't check if the user is in the timetracking group when returning data
Version: 3.0 → 2.23.3
Flags: approval?
Flags: approval3.0?
Flags: approval3.0+
Flags: approval+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Security advisory from Bug 389843 sent, unlocking bug.
Group: webtools-security
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.