Bug 529416 (CVE-2009-3386)

[SECURITY] Dependency lists display bug aliases even for bugs the user cannot access

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
Creating/Changing Bugs
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: justdave)

Tracking

({regression, selenium})

3.3.2
Bugzilla 3.4
regression, selenium
Dependency tree / graph
Bug Flags:
approval +
approval3.4 +
blocking3.4.4 +
testcase +

Details

(URL)

Attachments

(3 attachments, 7 obsolete attachments)

1.07 KB, patch
Max Kanat-Alexander
: review+
Frédéric Buclin
: review+
Details | Diff | Splinter Review
1.10 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
2.14 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
https://bugzilla.mozilla.org/show_bug.cgi?id=389014 reveals the alias of the two security-sensitive bugs it blocks :(
Created attachment 412965 [details] [diff] [review]
Patch v1 for 3.4

this is already deployed on b.m.o
Assignee: create-and-change → justdave
Attachment #412965 - Flags: review?(mkanat)

Comment 2

8 years ago
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().
Attachment #412965 - Flags: review?(mkanat) → review-

Comment 3

8 years ago
This is a regression due to bug 470262. Affects 3.4 and newer.
Depends on: 470262
Flags: blocking3.4.4+
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Bugzilla 3.4
Version: unspecified → 3.4.3

Updated

8 years ago
Summary: Blocks-alias is shown even for bugs the user cannot access → [SECURITY] Dependency lists display bug aliases even for bugs the user cannot access
This has been assigned CVE-2009-3386.
Alias: CVE-2009-3386
(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

8 years ago
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.
Attachment #412965 - Flags: review+

Updated

8 years ago
Flags: approval?
Flags: approval3.4?
Created attachment 413030 [details] [diff] [review]
Patch v2 for trunk
Attachment #412965 - Attachment is obsolete: true
Attachment #413030 - Flags: review?(mkanat)

Comment 8

8 years ago
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.
Attachment #413030 - Flags: review?(mkanat) → review+
Created attachment 413031 [details] [diff] [review]
Patch v2 for 3.4 branch
Attachment #413031 - Flags: review?(mkanat)

Comment 10

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

Same stylistic nit. Also looks fine.
Attachment #413031 - Flags: review?(mkanat) → review+
Created attachment 413033 [details] [diff] [review]
Patch v3 for trunk

nit addressed (might as well get it right before we commit ;)
Attachment #413030 - Attachment is obsolete: true
Created attachment 413034 [details] [diff] [review]
Patch v3 for 3.4 branch
Attachment #413031 - Attachment is obsolete: true

Updated

8 years ago
Attachment #413033 - Flags: review+

Updated

8 years ago
Blocks: 529481

Comment 13

8 years ago
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)) {
Attachment #413034 - Flags: review-

Comment 14

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

The same comment applies here too.
Attachment #413033 - Flags: review-

Updated

8 years ago
Flags: approval?
Flags: approval3.4?

Comment 15

8 years ago
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

8 years ago
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.
Attachment #413034 - Flags: review-
Created attachment 413182 [details] [diff] [review]
Patch v4 for trunk
Attachment #413033 - Attachment is obsolete: true
Attachment #413182 - Flags: review?(mkanat)
Attachment #413182 - Attachment description: Patch v5 for trunk → Patch v4 for trunk
Created attachment 413183 [details] [diff] [review]
Patch v4 for 3.4 branch
Attachment #413034 - Attachment is obsolete: true
Attachment #413183 - Flags: review?(mkanat)
Created attachment 413185 [details] [diff] [review]
Patch v5 for trunk
Attachment #413182 - Attachment is obsolete: true
Attachment #413185 - Flags: review?(mkanat)
Attachment #413182 - Flags: review?(mkanat)
Created attachment 413186 [details] [diff] [review]
Patch v5 for 3.4 branch
Attachment #413183 - Attachment is obsolete: true
Attachment #413186 - Flags: review?
Attachment #413183 - Flags: review?(mkanat)

Updated

8 years ago
Version: 3.4.3 → 3.3.2

Comment 21

8 years ago
Comment on attachment 413186 [details] [diff] [review]
Patch v5 for 3.4 branch

Now the logic makes sense. r=LpSolit
Attachment #413186 - Flags: review? → review+

Comment 22

8 years ago
Comment on attachment 413185 [details] [diff] [review]
Patch v5 for trunk

Here too, I'm now fine with this patch. r=LpSolit
Attachment #413185 - Flags: review+

Updated

8 years ago
Flags: approval3.4+
Flags: approval+

Updated

8 years ago
Attachment #413185 - Flags: review?(mkanat) → review+

Comment 23

8 years ago
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
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 24

8 years ago
Security advisory sent, unlocking bug.
Group: bugzilla-security

Updated

6 years ago
Flags: testcase?

Comment 25

6 years ago
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.

Updated

6 years ago
Flags: testcase? → testcase+
Keywords: selenium
You need to log in before you can comment on or make changes to this bug.