765143 - SecureMail needs to sanitize all bug_link references in html bugmail

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[:glob ✱]

in html bugmail bugs are linkified with the bug_link filter, which provides the bugs's summary, status, etc.

securemail needs to grep the html part for any links to other bugs, and sanitize the tooltip if the referenced bug is restricted and the current email isn't encrypted.

Comment 1

[Gervase Markham [:gerv]]

Doing this post-hoc using grep sounds a bit fragile to me. Perhaps it's time for Securemail to hook in to an earlier part of the email generation process?

Gerv

Comment 2

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #1)
> Doing this post-hoc using grep sounds a bit fragile to me. Perhaps it's time
> for Securemail to hook in to an earlier part of the email generation process?

there isn't anywhere earlier we can hook -- we have to modify the content after the template has generated it.  in 4.0 most of the content is pre-formed in perl and dumped into the template, however this changed in 4.2; which means we have to post-process the template in order to determine which bugs have been bug_link'ed (and therefore have their summary included in a tooltip).

you could do it by adding a hook to the bug_link filter, however i feel this would be detrimental performance-wise and therefore not a good idea.

another more option would be to have an email-specific bug_link filter which securemail can then hook into, but this would be a reasonable invasive upstream patch.

as long as bug_link continues to use an <a> element to show_bug.cgi, with the summary in the tooltip, it shouldn't be fragile to search/replace the email after generation.

Comment 3

[Gervase Markham [:gerv]]

By hooking in earlier, perhaps I was thinking of finding a way to hack Bugzilla->user so the template generation code thought that he didn't have permission to see all of the referenced bugs, and so Did The Right Thing straight away. I'm assuming the bug_link filter does do permission checking somewhere; I'm wondering how we can get it to come up with the "right" result.

Someone who is CCed on a security bug can see that bug, but not any other security bugs to which it refers. Can we fiddle things so it "seems" to Bugzilla that a user without a public key is CCed on the current bug instead of being a member of a group which can see it?

Just random ideas, not sure if it would work...

Gerv

Comment 4

[:glob ✱]

that sort of approach is probably possible, however i feel messing about with bugzilla's security in that way would be more fragile (and risky) than a search/replace on the template's output.

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to sanitize secure bug_link HTML in emails (v1)

Using HTML::Tree to parse links in the HTML email simply and replace the class/title attributes with sanitized information if the bug is secured.

dkl

Comment 6

[Gervase Markham [:gerv]]

The architecture here seems increasingly wrong. We are creating emails only to reparse them. We are creating HTML only to reparse it. We are extracting user IDs from subject lines to recreate objects associated with them. All of these things say to me that the way SecureMail works is hooking into the process too late. :-|

That doesn't necessarily mean we shouldn't take this patch, but we should watch out for death by a thousand cuts.

Gerv

Comment 7

[:glob ✱]

Comment on attachment 638713 [details] [diff] [review]
Patch to sanitize secure bug_link HTML in emails (v1)

you don't need to perform any sanitisation if the whole email is encrypted.

can you make the check for html-tree conditional; it's only required for bugzilla > 4.

+ my ($bug_id) = $href =~ /\Qshow_bug.cgi?id=\E([0-9]+)/;
use \d+ instead of [0-9]+

when using s/mime with this patch, i received an email which my email client wasn't able to decrypt (no error, just blank content).

Comment 8

[David Lawrence [:dkl]]

Attachment: Patch to sanitize secure bug_link HTML in emails (v2)

This is a revised patch using HTML::Tree that addresses the changes you requested. I was not able to get S/MIME to fail. Can you re-verify with this patch please?

dkl

Comment 9

[David Lawrence [:dkl]]

Attachment: Patch to sanitize secure bug_link HTML in emails using template hook (v1)

Here is a different patch that solves the issue in a different way. I create a hook in bug/link.html.tmpl which in turn calls link-link.html.tmpl in SecureMail extension that filters the title, class and link text appropriately. The only caveat with this is I needed to create a new param to Bugzilla::Template::get_bug_link called in_email that is set to true when email/bugmail.html.tmpl calles bug_link or quoteUrls. So there will be some changes to be pushed upstream if possible. But this solution works without additional modules of any kind.

dkl

Comment 10

[:glob ✱]

Comment on attachment 638866 [details] [diff] [review]
Patch to sanitize secure bug_link HTML in emails (v2)

r=glob

>+        if ($make_secure == SECURE_NONE) {
>+            # Filter the bug_links in HTML email in case the bugs the links
>+            # point are "secured" bugs and the user may not be able to see 
>+            # the summaries.
>+            print STDERR "USING FILTER_BUG_LINKS" if $FILTER_BUG_LINKS;
>+            _filter_bug_links($email) if $FILTER_BUG_LINKS;
>+        }
>+
>         if ($make_secure != SECURE_NONE) {
>             _make_secure($email, $public_key, $is_bugmail && $make_secure == SECURE_ALL, $add_new);
>         }

this works, but it should be an 'else' after the existing condition, and the debugging code needs to be removed.

Comment 11

[:glob ✱]

Comment on attachment 638867 [details] [diff] [review]
Patch to sanitize secure bug_link HTML in emails using template hook (v1)

i don't think taking the perf hit of hooking a filter is a good way to fix this.


i think the only way we'd get a cleaner implementation of this is to roll this functionality into upstream bugzilla; then we wouldn't have the overhead of hook processing, and we'd be able to be smarter when identifying which templates are used to build emails.  but that's a story for another bug.

Comment 12

[Gervase Markham [:gerv]]

(In reply to Byron Jones ‹:glob› from comment #11)
> i don't think taking the perf hit of hooking a filter is a good way to fix
> this.

You may be right, but is there any way of calculating the size of the hit by profiling?

If the hit is an extra function call and if test per call to the filter, it may end up being lost in the noise.

But I'd have to sadly agree that if we can't profile, we should err on the side of caution here.

Gerv

Comment 13

[David Lawrence [:dkl]]

I will file a bug upstream about fixing quoteUrls and get_bug_link. Currently bug links in comments in the HTML email part are broken in that they contain relative URLs instead of fully qualified links. Maybe I can push for my suggested in_email param to be added.

Committing to: bzr+ssh://[e-mail]/bmo/4.2
modified extensions/SecureMail/Config.pm
modified extensions/SecureMail/Extension.pm
Committed revision 8235.