529416 - (CVE-2009-3386) [SECURITY] Dependency lists display bug aliases even for bugs the user cannot access

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Jesse Ruderman]

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

Attachment: Patch v1 for 3.4

this is already deployed on b.m.o

Comment 2

[Frédéric Buclin]

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]

This is a regression due to bug 470262. Affects 3.4 and newer.

Comment 4

[Reed Loden [:reed]]

This has been assigned CVE-2009-3386.

Comment 5

[Dave Miller [:justdave]]

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

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

Attachment: Patch v2 for trunk

Comment 8

[Max Kanat-Alexander]

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

Attachment: Patch v2 for 3.4 branch

Comment 10

[Max Kanat-Alexander]

Comment on attachment 413031 [details] [diff] [review]
Patch v2 for 3.4 branch

Same stylistic nit. Also looks fine.

Comment 11

[Dave Miller [:justdave]]

Attachment: Patch v3 for trunk

nit addressed (might as well get it right before we commit ;)

Comment 12

[Dave Miller [:justdave]]

Attachment: Patch v3 for 3.4 branch

Comment 13

[Frédéric Buclin]

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]

Comment on attachment 413033 [details] [diff] [review]
Patch v3 for trunk

The same comment applies here too.

Comment 15

[Max Kanat-Alexander]

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]

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

Attachment: Patch v4 for trunk

Comment 18

[Dave Miller [:justdave]]

Attachment: Patch v4 for 3.4 branch

Comment 19

[Dave Miller [:justdave]]

Attachment: Patch v5 for trunk

Comment 20

[Dave Miller [:justdave]]

Attachment: Patch v5 for 3.4 branch

Comment 21

[Frédéric Buclin]

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]

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]

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]

Security advisory sent, unlocking bug.

Comment 25

[Frédéric Buclin]

Attachment: Selenium script

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.2/
modified t/test_security.t
Committed revision 218.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.0/
modified t/test_security.t
Committed revision 205.