1064140 - (CVE-2014-1571) [SECURITY] Private comments can be shown to flagmail recipients who aren't in the insider group

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect, P1)

Description

[:glob ✱]

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.

Comment 1

[Simon Green]

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)

Comment 2

[Simon Green]

Attachment: v1 patch

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.

Comment 3

[:glob ✱]

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.

Comment 4

[Frédéric Buclin]

(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).

Comment 5

[Simon Green]

Attachment: v2 patch

Comment 6

[:glob ✱]

Comment on attachment 8486131 [details] [diff] [review]
v2 patch

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

r=glob

Comment 7

[Daniel Veditz [:dveditz]]

Assigned CVE-2014-1571 to this issue.

Comment 8

[Matt Tyson]

Attachment: Bugzilla 4.0 patch

Comment 9

[Matt Tyson]

Attachment: Bugzilla 4.2 patch

Comment 10

[Matt Tyson]

Attachment: Bugzilla 4.4 patch.

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.

Comment 11

[:glob ✱]

Comment on attachment 8495665 [details] [diff] [review]
Bugzilla 4.0 patch

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

r=glob

Comment 12

[:glob ✱]

Comment on attachment 8495679 [details] [diff] [review]
Bugzilla 4.2 patch

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

r=glob

Comment 13

[:glob ✱]

Comment on attachment 8495684 [details] [diff] [review]
Bugzilla 4.4 patch.

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

r=glob

Comment 14

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   c8f8d1e..19576a8  4.0 -> 4.0

To ssh://[e-mail]/bugzilla/bugzilla.git
   6d6b390..976dc12  4.2 -> 4.2

To ssh://[e-mail]/bugzilla/bugzilla.git
   6364067..a92eee1  4.4 -> 4.4

To ssh://[e-mail]/bugzilla/bugzilla.git
   f33b119..fa954ab  master -> master