Bug 1064140 (CVE-2014-1571)

[SECURITY] Private comments can be shown to flagmail recipients who aren't in the insider group

RESOLVED FIXED in Bugzilla 4.0

Status

()

defect
P1
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glob, Assigned: mail)

Tracking

({sec-high})

2.17.1
Bugzilla 4.0
Dependency tree / graph
Bug Flags:
approval +
approval4.4 +
blocking4.4.6 +
approval4.2 +
blocking4.2.11 +
approval4.0 +
blocking4.0.15 +

Details

Attachments

(4 attachments, 1 obsolete attachment)

template/en/default/email/flagmail.txt.tmpl has:

> [% IF Bugzilla.cgi.param("comment").defined && Bugzilla.cgi.param("comment").length > 0 %]
> ------- Additional Comments from [% user.identity %]
> [%+ Bugzilla.cgi.param("comment") FILTER strip_control_chars %]

it's highly likely this will not work for API driven updates.
Setting the security flag on this because of the follow situation:

User A is in the insider group. User B can see the bug, but is not in the insider group.

UserA make a private comment and sets the needinfo? flag, with User B as the requestee. User B receives the flag mail but it includes the private comment despite them not been able to see the private comment otherwise. The bug mail is correct (shows all changes but not the private comment)
Group: bugzilla-security
Target Milestone: --- → Bugzilla 4.0
Assignee: email-notifications → sgreen
Severity: normal → critical
Status: NEW → ASSIGNED
Depends on: 179582
Keywords: sec-high
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Summary: flagmail email template shouldn't read data from $cgi → Private comments can be shown to flagmail recipients who aren't in the insider group
Posted patch v1 patch (obsolete) — Splinter Review
There were two issues that needed to worked around:

$bug->update added the flags before the comments. I solved this by moving the flag change to further down the sub routine.

The second issues was the $bug->{comments} was cached from before the change. This was solved by adding a nocache option.

Ideally, we should move flag mail generation out of Bugzilla::Flag and into send_changes, but that is for a different bug. Once this review is approved, I'll supply patches for 4.0, 4.2 and 4.4 if needed.
Attachment #8485611 - Flags: review?(glob)
Blocks: 1046280
Comment on attachment 8485611 [details] [diff] [review]
v1 patch

Review of attachment 8485611 [details] [diff] [review]:
-----------------------------------------------------------------

aside from the nocache change, this patch looks good.

::: Bugzilla/Flag.pm
@@ +1124,5 @@
>      }
>  
> +    # Get comments on the bug
> +    my $all_comments = $bug->comments({ after => $bug->lastdiffed, nocache => 1 });
> +    @$all_comments   = grep { $_->type || $_->body =~ /\S/ } @$all_comments;

to avoid re-fetching the comments, it should be easier to delete $bug->{comments} when a comment is added.
Attachment #8485611 - Flags: review?(glob) → review-
(In reply to Byron Jones ‹:glob› from comment #0)
> template/en/default/email/flagmail.txt.tmpl has:
> it's highly likely this will not work for API driven updates.

FYI, this has already been filed as bug 508541 (and a dupe already exists: bug 1054181).
Blocks: 508541
Posted patch v2 patchSplinter Review
Attachment #8485611 - Attachment is obsolete: true
Attachment #8486131 - Flags: review?(glob)
Comment on attachment 8486131 [details] [diff] [review]
v2 patch

Review of attachment 8486131 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob
Attachment #8486131 - Flags: review?(glob) → review+
Flags: approval4.4+
Flags: approval4.2+
Flags: approval4.0+
Flags: approval+
Blocks: 1072497
Assigned CVE-2014-1571 to this issue.
Alias: CVE-2014-1571
Attachment #8495665 - Flags: review?(glob)
Attachment #8495679 - Flags: review?(glob)
I've attached patches for Bugzilla 4.0, 4.2 and 4.4.

There's no functional changes, code has moved around and the trunk patch didn't apply cleanly to older branches.
Attachment #8495684 - Flags: review?(glob)
Attachment #8495679 - Attachment is patch: true
Comment on attachment 8495665 [details] [diff] [review]
Bugzilla 4.0 patch

Review of attachment 8495665 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob
Attachment #8495665 - Flags: review?(glob) → review+
Comment on attachment 8495679 [details] [diff] [review]
Bugzilla 4.2 patch

Review of attachment 8495679 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob
Attachment #8495679 - Flags: review?(glob) → review+
Comment on attachment 8495684 [details] [diff] [review]
Bugzilla 4.4 patch.

Review of attachment 8495684 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob
Attachment #8495684 - Flags: review?(glob) → review+
Summary: Private comments can be shown to flagmail recipients who aren't in the insider group → [SECURITY] Private comments can be shown to flagmail recipients who aren't in the insider group
Version: unspecified → 2.17.1
Flags: blocking4.4.6?
Flags: blocking4.2.11?
Flags: blocking4.0.15?
Flags: blocking4.4.6?
Flags: blocking4.4.6+
Flags: blocking4.2.11?
Flags: blocking4.2.11+
Flags: blocking4.0.15?
Flags: blocking4.0.15+
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   c8f8d1e..19576a8  4.0 -> 4.0

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   6d6b390..976dc12  4.2 -> 4.2

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   6364067..a92eee1  4.4 -> 4.4

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   f33b119..fa954ab  master -> master
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Group: bugzilla-security
Blocks: 1082887
You need to log in before you can comment on or make changes to this bug.