Last Comment Bug 529416 - (CVE-2009-3386) [SECURITY] Dependency lists display bug aliases even for bugs the user cannot access
(CVE-2009-3386)
: [SECURITY] Dependency lists display bug aliases even for bugs the user cannot...
Status: RESOLVED FIXED
: regression, selenium
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 3.3.2
: All All
: -- normal (vote)
: Bugzilla 3.4
Assigned To: Dave Miller [:justdave] (justdave@bugzilla.org)
: default-qa
Mentors:
https://bugzilla.mozilla.org/show_bug...
Depends on: 470262
Blocks: 529481
  Show dependency treegraph
 
Reported: 2009-11-17 15:54 PST by Jesse Ruderman
Modified: 2011-12-03 07:25 PST (History)
4 users (show)
mkanat: approval+
mkanat: approval3.4+
LpSolit: blocking3.4.4+
LpSolit: testcase+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 for 3.4 (512 bytes, patch)
2009-11-17 15:59 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
LpSolit: review-
mkanat: review+
Details | Diff | Review
Patch v2 for trunk (1.18 KB, patch)
2009-11-17 22:08 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
mkanat: review+
Details | Diff | Review
Patch v2 for 3.4 branch (1.27 KB, patch)
2009-11-17 22:09 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
mkanat: review+
Details | Diff | Review
Patch v3 for trunk (1.18 KB, patch)
2009-11-17 22:14 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
mkanat: review+
LpSolit: review-
Details | Diff | Review
Patch v3 for 3.4 branch (1.27 KB, patch)
2009-11-17 22:14 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
LpSolit: review-
mkanat: review-
Details | Diff | Review
Patch v4 for trunk (492 bytes, patch)
2009-11-18 14:13 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
no flags Details | Diff | Review
Patch v4 for 3.4 branch (506 bytes, patch)
2009-11-18 14:14 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
no flags Details | Diff | Review
Patch v5 for trunk (1.07 KB, patch)
2009-11-18 14:17 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
mkanat: review+
LpSolit: review+
Details | Diff | Review
Patch v5 for 3.4 branch (1.10 KB, patch)
2009-11-18 14:17 PST, Dave Miller [:justdave] (justdave@bugzilla.org)
LpSolit: review+
Details | Diff | Review
Selenium script (2.14 KB, patch)
2011-12-03 07:25 PST, Frédéric Buclin
no flags Details | Diff | Review

Description Jesse Ruderman 2009-11-17 15:54:53 PST
https://bugzilla.mozilla.org/show_bug.cgi?id=389014 reveals the alias of the two security-sensitive bugs it blocks :(
Comment 1 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-17 15:59:33 PST
Created attachment 412965 [details] [diff] [review]
Patch v1 for 3.4

this is already deployed on b.m.o
Comment 2 Frédéric Buclin 2009-11-17 16:31:58 PST
Comment on attachment 412965 [details] [diff] [review]
Patch v1 for 3.4

>-    if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug_alias) {
>+    if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug_alias && Bugzilla->user->can_see_bug($bug_num)) {

This should go later in the code where we already call can_see_bug().
Comment 3 Frédéric Buclin 2009-11-17 16:34:17 PST
This is a regression due to bug 470262. Affects 3.4 and newer.
Comment 4 Reed Loden [:reed] (use needinfo?) 2009-11-17 17:00:06 PST
This has been assigned CVE-2009-3386.
Comment 5 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-17 18:11:06 PST
(In reply to comment #2)
> (From update of attachment 412965 [details] [diff] [review])
> >-    if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug_alias) {
> >+    if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug_alias && Bugzilla->user->can_see_bug($bug_num)) {
> 
> This should go later in the code where we already call can_see_bug().

I was thinking that, but that section of code is dependent on having a status or something, and didn't know how that would affect it, that's why I didn't move it.
Comment 6 Max Kanat-Alexander 2009-11-17 22:03:46 PST
Comment on attachment 412965 [details] [diff] [review]
Patch v1 for 3.4

No, actually, this is the right fix. The can_see_bug below is unrelated.

I'd like to create an $user var and re-use it so that we don't have to manually do Bugzilla->user->can_see_bug in both places, but this works OK.
Comment 7 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-17 22:08:09 PST
Created attachment 413030 [details] [diff] [review]
Patch v2 for trunk
Comment 8 Max Kanat-Alexander 2009-11-17 22:09:19 PST
Comment on attachment 413030 [details] [diff] [review]
Patch v2 for trunk

>-    if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug->alias) {
>+    if ($options->{use_alias} && $link_text =~ /^\d+$/
>+        && $bug->alias && $user->can_see_bug($bug)) {

  Tiny stylistic nit: brace goes on the next line aligned with the "i" in "if". (It's perlstyle.)

  I'll test this, but this looks fine.
Comment 9 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-17 22:09:46 PST
Created attachment 413031 [details] [diff] [review]
Patch v2 for 3.4 branch
Comment 10 Max Kanat-Alexander 2009-11-17 22:10:46 PST
Comment on attachment 413031 [details] [diff] [review]
Patch v2 for 3.4 branch

Same stylistic nit. Also looks fine.
Comment 11 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-17 22:14:11 PST
Created attachment 413033 [details] [diff] [review]
Patch v3 for trunk

nit addressed (might as well get it right before we commit ;)
Comment 12 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-17 22:14:41 PST
Created attachment 413034 [details] [diff] [review]
Patch v3 for 3.4 branch
Comment 13 Frédéric Buclin 2009-11-18 06:00:29 PST
Comment on attachment 413034 [details] [diff] [review]
Patch v3 for 3.4 branch

>+    if ($options->{use_alias} && $link_text =~ /^\d+$/
>+        && $bug_alias && $user->can_see_bug($bug_num))

As I said earlier, this should be moved into the IF block below. All bugs have a bug status. We use it to check if the bug ID points to an existing bug. If the bug doesn't exist, it's undefined, else it exists. No need to call can_see_bug() twice.

>+        if ($user->can_see_bug($bug_num)) {
Comment 14 Frédéric Buclin 2009-11-18 06:00:59 PST
Comment on attachment 413033 [details] [diff] [review]
Patch v3 for trunk

The same comment applies here too.
Comment 15 Max Kanat-Alexander 2009-11-18 13:27:43 PST
Actually, I think the patch as justdave wrote it is more readable, and there's no performance penalty for calling can_see_bug multiple times.
Comment 16 Max Kanat-Alexander 2009-11-18 13:58:08 PST
Comment on attachment 413034 [details] [diff] [review]
Patch v3 for 3.4 branch

LpSolit pointed out that in this version we're calling can_see_bug before verifying that the bug exists, so this one indeed should have a different patch.
Comment 17 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-18 14:13:45 PST
Created attachment 413182 [details] [diff] [review]
Patch v4 for trunk
Comment 18 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-18 14:14:32 PST
Created attachment 413183 [details] [diff] [review]
Patch v4 for 3.4 branch
Comment 19 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-18 14:17:03 PST
Created attachment 413185 [details] [diff] [review]
Patch v5 for trunk
Comment 20 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-18 14:17:31 PST
Created attachment 413186 [details] [diff] [review]
Patch v5 for 3.4 branch
Comment 21 Frédéric Buclin 2009-11-18 14:23:33 PST
Comment on attachment 413186 [details] [diff] [review]
Patch v5 for 3.4 branch

Now the logic makes sense. r=LpSolit
Comment 22 Frédéric Buclin 2009-11-18 14:24:27 PST
Comment on attachment 413185 [details] [diff] [review]
Patch v5 for trunk

Here too, I'm now fine with this patch. r=LpSolit
Comment 23 Max Kanat-Alexander 2009-11-18 18:12:09 PST
tip:

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.115; previous revision: 1.114
done

3.4:

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.99.2.4; previous revision: 1.99.2.3
done
Comment 24 Max Kanat-Alexander 2009-11-18 21:00:06 PST
Security advisory sent, unlocking bug.
Comment 25 Frédéric Buclin 2011-12-03 07:25:37 PST
Created attachment 578836 [details] [diff] [review]
Selenium script

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.2/
modified t/test_security.t
Committed revision 218.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.0/
modified t/test_security.t
Committed revision 205.

Note You need to log in before you can comment on or make changes to this bug.