approval-mozilla-esr10 flag request mails show real subject for security bugs

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Extensions: SecureMail
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: reed, Assigned: dkl)

Tracking

Production

Details

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
khuey reported that flag mails still have the full bug summary in the subject for security bugs.
:mccr8 reported the same, although he added the intriguing detail that it looked like approval-mozilla-beta+ and approval-mozilla-aurora+ mail was secure but approval-mozilla-esr10+ mail was not. I can't think of any reason bugzilla would treat those flags differently. The only difference I see is that beta/aurora have flag comments and esr10 does not.

I can confirm myself-- for bug 724781 I got two mails sent at the same time for the same change:
  approval-mozilla-esr10 granted: [Bug 724781] Java related <rest of subject>
  [Bug 724781] (Secure bug updated)

The full content is unencrypted, it's not just the subject.
Yes, its just esr, it just happened to be the only one I looked at.
The esr flag specificity seems particularly odd.
Blocks: 731044
Summary: Flag request mails show real subject for security bugs → approval-mozilla-esr10 flag request mails show real subject for security bugs
(Assignee)

Comment 4

6 years ago
Can you attach the full text of the email (headers too) to this bug so I can track down the differences?

Thanks
dkl
Assignee: nobody → dkl
Status: NEW → ASSIGNED
Created attachment 612226 [details]
ESR10 mail with headers, sensitive text elided
Created attachment 612228 [details]
beta mail

No substantive differences between the two that I can see.
(Assignee)

Comment 8

6 years ago
Thanks I have found the problem. In the extension we grep the subject for the bug id and the regex used was finding the id incorrectly due to the esr flag having a number in the name. Also the code was making the body not secure due to not error checking the bug load was successful after getting the id. Patch ready and coming.

dkl
(Assignee)

Comment 9

6 years ago
Created attachment 612232 [details] [diff] [review]
Patch to fix encryption of flag emails with certain flag names (v1)

gerv this changes the regex used to pull the bug id from the email subject so that flags with a number in the name do not cause the wrong id to be pulled. Also it now checks for $bug->{'error'} before making the body insecure. Please review so I can get this out in the next update. Glob is out til thursday.

Thanks
dkl
Attachment #612232 - Flags: review?(gerv)
Duplicate of this bug: 742057
Comment on attachment 612232 [details] [diff] [review]
Patch to fix encryption of flag emails with certain flag names (v1)

r=gerv. Although is there a reason you go from the first digit to the closing bracket, rather than looking for a run of digits inside the brackets? 

([^\]]+)
vs
([\d]+)

With the current header, they are equivalent, but I just wondered...

Gerv
Attachment #612232 - Flags: review?(gerv) → review+
(Assignee)

Comment 12

6 years ago
(In reply to Gervase Markham [:gerv] from comment #11)
> Comment on attachment 612232 [details] [diff] [review]
> Patch to fix encryption of flag emails with certain flag names (v1)
> 
> r=gerv. Although is there a reason you go from the first digit to the
> closing bracket, rather than looking for a run of digits inside the
> brackets? 
> 
> ([^\]]+)
> vs
> ([\d]+)
> 
> With the current header, they are equivalent, but I just wondered...
> 
> Gerv

Moving to /\[\D+(\d+)\]/ which is much clearer as you pointed out. Retesting and will commit with the new regex if all goes well.

dkl
(Assignee)

Comment 13

6 years ago
Comitted.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bmo/4.0             
modified extensions/SecureMail/Extension.pm
Committed revision 8121. 

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bmo/4.2             
modified extensions/SecureMail/Extension.pm
Committed revision 8107

dkl
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.