If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Changing the name of a private attachment in an unhidden bug results in the name change being sent unencrypted

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Extensions: SecureMail
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mccr8, Assigned: dkl)

Tracking

Production

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
I think that if a attachment that is marked "private (only visible to core-security)" (that I can view) gets its description changed, in a non-private bug, then the old and new description are sent out in plain text in bug mail. I noticed this during the stream of bug bounty updates that happened today, and I think those are all marked private, but I could see the change made in plain text.
This seems like a Bugzilla bug, not a SecureMail bug, because it relates to the "insidergroup" feature.

Gerv
Assignee: nobody → attach-and-request
Group: bugzilla-security
Component: Extensions: SecureMail → Attachments & Requests
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: default-qa
Target Milestone: --- → Bugzilla 4.4
Version: Production → 4.2
Oh, I see. No, I'm wrong. Obviously you should see the change, as you are in the group! The issue is that the mail is not encrypted.

Gerv
Assignee: attach-and-request → nobody
Group: bugzilla-security
Component: Attachments & Requests → Extensions: SecureMail
Product: Bugzilla → bugzilla.mozilla.org
QA Contact: default-qa
Target Milestone: Bugzilla 4.4 → ---
Version: 4.2 → Production
(Reporter)

Comment 3

5 years ago
Yeah, sorry, I should have been more explicit about that.
Summary: Changing the name of a private attachment in an unhidden bug results in the name change being sent in bugmail → Changing the name of a private attachment in an unhidden bug results in the name change being sent unencrypted
(Assignee)

Comment 4

3 years ago
Sorry for sitting on this one so long. I finally see why this is failing and am working on a solution.

Basically the SecureMail code is doing the following:
                
                # Encrypt if updating a private attachment without a comment
                if ($email->header('X-Bugzilla-Changed-Fields')
                    && $email->header('X-Bugzilla-Changed-Fields') =~ /Attachment #(\d+)/)
                {
                    my $attachment = Bugzilla::Attachment->new($1);
                    if ($attachment && $attachment->isprivate) {
                        $make_secure = SECURE_BODY;
                    }
                }  

But looking at the changed field names we are actually putting in the header for attachment changes, we do not include the attachment id such as "Attachment #1234 [details] description" for a description change. We just have "Attachment description" only which will not match.

So we need to either do one of two things:

1) Convert to "Attachment #1234 [details] description" format
2) or include a new "X-Bugzilla-Attach-ID:" header and store the attachment id there to pull from in the SecureMail code. We normally never update more than one attachment at a time so this should not be an issue.

I personally think #2 is better as changing the field names in the current header may break a lot of filter rules already in place. Will work on a patch.

dkl
Assignee: nobody → dkl
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 years ago
Created attachment 8526283 [details] [diff] [review]
829358_1.patch
Attachment #8526283 - Flags: review?(glob)
Comment on attachment 8526283 [details] [diff] [review]
829358_1.patch

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

r=glob

::: extensions/SecureMail/Extension.pm
@@ +308,5 @@
>                  }
>                  # Encrypt if updating a private attachment without a comment
>                  if ($email->header('X-Bugzilla-Changed-Fields')
> +                    && $email->header('X-Bugzilla-Attach-ID')
> +                    && $email->header('X-Bugzilla-Changed-Fields') =~ /Attachment/)

searching x-bugzilla-changed-fields for attachment is redundant and can be removed
Attachment #8526283 - Flags: review?(glob) → review+
(Assignee)

Comment 7

3 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   57c9998..66da5ae  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.