Max Kanat-Alexander Reporter
	Description • 16 years ago

Currently a malicious user could upload an HTML or JavaScript attachment that could possibly harm a user. This is largely going to be handled by bug 38862.

However, a more definite solution that could possibly be more broadly applicable is to forbid viewing attachments if they are viewable in the browser, and force them to be downloaded (using the disposition argument of the content-type header would work, I believe).

Some installations (and indeed, even a certain subset of users on some installations) always need the ability to view HTML attachments, though, like Firefox developers. So this could be a user-preference.

	Frédéric Buclin Assignee
	Comment 1 • 16 years ago

The original discussion is in bug 38862 comment 212. I suggested to have a user pref "If the attachment is an HTML page..." with options:

1) display the file in the browser, unaltered,
2) display a trimmed copy of the file (with JS code removed (and iframes?))
3) display the file as plain text
4) download the file

1) is what we currently have.
2) is easy to do with HTML::Scrubber, which is already an optional Perl module
used by Bugzilla
3) is a one-liner in attachment.cgi at line 317 (on my patched copy of
attachment.cgi). I know IE sniffs the content of the file, but we all know IE
sucks, and IE users are free to choose option 4).
4) is also a one-liner at the same line as above.

And yes, I really mean a user pref, not a parameter. Another question is whether we want to give the user the possibility to override this pref on a per attachment basis, by giving some additional links in the attachments table.

	Max Kanat-Alexander Reporter
	Comment 2 • 16 years ago

I'd like to keep it simpler than that. Just "view" vs" download".

	Frédéric Buclin Assignee
	Comment 3 • 16 years ago

An all or nothing option is of no interest to me. Viewing the source code is still of interest, I think. If you see nothing wrong in the source, having a "display the HTML page" link would be interesting. Maybe we could skip the trimmed version, though.

	Frédéric Buclin Assignee
	Comment 4 • 16 years ago

jonas: about your bug 38862 comment 220:

If you trust the user who attached the HTML file, you can safely display the HTML page, else you should first have a look at its source code.

	Max Kanat-Alexander Reporter
	Comment 5 • 16 years ago

(In reply to comment #3)
> An all or nothing option is of no interest to me. Viewing the source code is
> still of interest, I think.

  To you, maybe. In that case you could just leave the preference on. Let's keep it simple. This is just a tiny tiny part of Bugzilla.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 16 years ago

(In reply to comment #4)
> If you trust the user who attached the HTML file, you can safely display the
> HTML page, else you should first have a look at its source code.

Possibly, but then it wouldn't make sure to have a preference, but rather a way to navigate to an unfiltered version of an attachment. And only allow that after passing a very scary-looking page warning you that you're entering uncharted territory. (a'la the phising or invalid SSL cert page in firefox).

	Dave Miller [:justdave]
	Comment 7 • 16 years ago

You guys appear to be proceeding as if this is got split to a separate bug in order to allow it to be pushed off until after the security release.  In case I didn't make it clear on bug 38862, it's pretty clear from the feedback we've gotten from Bugzilla maintainers already that this issue needs to be resolved before the security release or we're going to have a lot of angry users.

Some people simply can't set up the alternate domain mechanism, and many of those who can will require time to implement it.  Getting a filtering system in place can be done just with upgrading Bugzilla and not require a time consuming site reconfiguration.  It does restrict existing functionality in Bugzilla, but that functionality can be restored by then doing the configuration needed for the alternate hostname setup.  See bug 38862 comment 222 for where I think this should be going.

	Max Kanat-Alexander Reporter
	Comment 8 • 16 years ago

Okay, in that case I need to rearrange the releases and do a 3.3.1 right now, because we freeze on Jan 29 and I need a development release well before that time.

	Frédéric Buclin Assignee
	Comment 9 • 16 years ago

(In reply to comment #7)
> site reconfiguration.  It does restrict existing functionality in Bugzilla, but
> that functionality can be restored by then doing the configuration needed for
> the alternate hostname setup.

This is not acceptable, IMO. Assuming one cannot have an alternate host at all, viewing HTML pages will not be possible, period. Even if you trust the author of the attachment. I'm against releasing 3.2.1 with the filter code hardcoded.

	Max Kanat-Alexander Reporter
	Comment 10 • 16 years ago

I agree with justdave's implementation suggestion.

	Max Kanat-Alexander Reporter
	Comment 11 • 16 years ago

Note, however, that you cannot reliably scrub or filter complex HTML, without restricting the list of allowable tags and attributes. There is pretty much no Perl module on earth that will allow this (probably no code on earth that will allow it). Since we'd be accepting all forms of HTML (and really, lots of other things that can run in a browser that we can't imagine), we'd have to force downloading--no viewing or filtering would reliably be possible.

	Max Kanat-Alexander Reporter
	Comment 12 • 16 years ago

Note that because of bugs in IE, we'd have to restrict viewing any form of attachment, even text/plain attachments.

	Frédéric Buclin Assignee
	Comment 13 • 16 years ago

As I said in the other bug, I'm fine not to implement the trimmed version, but the other ones are perfectly doable, in a very non-invasive way. IE is not a problem, as I said in the other bug, [% attachment.data FILTER html %] would be safe, even for IE users.

	Dave Miller [:justdave]
	Comment 14 • 16 years ago

If people who can't set up an alternate name are willing to accept the risk and enable the attachment viewing in the browser without it, I would have no problem with adding a one-liner somewhere that we can direct them to "go comment this linen out," or "change this 0 to a 1" or somesuch to override it.  Due to the security risk involved, I'd rather force them to edit code to get around it than have a pref, but there's no reason to make it difficult if we can make it easy to hack around.

	Dave Miller [:justdave]
	Comment 15 • 16 years ago

I do kind of like the "view source" thing LpSolit is suggesting though, running the attachment through FILTER html before displaying it to the user as text/html (so that they effectively get text).  That would be cool to have regardless of how this turns out (and maybe that ought to be another bug).  That appears to be bug 346255.

	Frédéric Buclin Assignee
	Comment 16 • 16 years ago

Here is why we shouldn't force to filter HTML pages: some groups have a Bugzilla installation on a host they don't own (a.k.a. get a free Bugzilla installation on our servers). I guess smaller groups of workers aren't technically able/allowed to have an alternate host (or can get just one for all attachments, not one per bug). This means that building alternate hosts is not under their control and you don't know what they use Bugzilla for and why they need to attach/display HTML pages all day long. Forcing them to download HTML pages is a regression to them. Maybe they know all the co-users of their (access restricted) Bugzilla installation and so they have no reason to consider the HTML files as evil. In closed environments (where login is required and/or user accounts under control, e.g. limited to @some.company.com), they have no reason to use an alternate host; all the users work in the company, the one who attaches an evil attachment to do damage would be quickly fired.

For all these groups (and again, you don't know why they need Bugzilla to display HTML attachments inline), blocking HTML files is a regression, period. And asking them to have an alternate host is not an alternative. The user pref I'm talking about (or make that a parameter if you think users shouldn't be allowed to relax the rule themselves) is non-invasive, and offers a reasonable alternative to all those who have no reason to fear evil attachments, or who are "clever" enough to decide if they trust the author of the attachment or not.

	Dave Miller [:justdave]
	Comment 17 • 16 years ago

That's exactly why I was suggesting we make it easy to disable by changing one line of code somewhere if they really needed that functionality.  It's called "secure by default".  But easy for the admin to override if they really need it and are willing to take the risk.

	Max Kanat-Alexander Reporter
	Comment 18 • 16 years ago

I am definitely in favor of secure by default. :-) Also, I'd have to imagine that the default association of text/html is your web browser, anyway, so when you download it you'll be given the option to load the page...in your web browser. But at least you'll have to click OK on it. :-)

	Frédéric Buclin Assignee
	Comment 19 • 16 years ago

It will never be *exactly* one line of code. And asking admins to uncomment e.g. lines 310-317 + 341-344 + 412-414 in some given file(s) is bad. "Oh, I did something wrong?!". I'm still strongly opposed to the "secure by default" suggestion.

	Denis Roy
	Comment 20 • 16 years ago

FWIW, I like the 'security by default' approach, and I like Jonas' idea in comment 6:

> but rather a way
> to navigate to an unfiltered version of an attachment.

If url_attach (or whatever you call it) is empty, filter by default, but warn the user that the attachment was filtered and could be harmful, but do provide a way to view unfiltered. No code hacks, no user prefs, user is safe, and no functionality is lost.

	Frédéric Buclin Assignee
	Comment 21 • 16 years ago

Per our hot discussion on IRC, I implemented a parameter to enable/disable attachment display in the browser.

	Max Kanat-Alexander Reporter
	Comment 22 • 16 years ago

Comment on attachment 355849 [details] [diff] [review]
patch, v1

Okay, wait, I was thinking about this some more, and who could there be that couldn't set up another domain that pointed to the same install? Even shared hosting people can do that, they can use VirtualHosts. That's one of the big advantages of shared hosting, in fact.

So can you give me some situation (common enough to justify this implementation) where somebody would need the allow_attachment_display parameter? (That is, some common situation where the administrator *couldn't* set up another domain.)

	Marc Schumann [:Wurblzap]
	Comment 23 • 16 years ago

It's not as much *the Bugzilla administrator couldn't*, as *the one responsible for domains wouldn't*.

	Frédéric Buclin Assignee
	Comment 24 • 16 years ago

The alternate host doesn't prevent everything, as discussed on IRC. I won't repeat my arguments here, read again my comment 16.

	Dave Miller [:justdave]
	Comment 25 • 16 years ago

What Mark said in comment 23.

(From update of attachment 355849 [details] [diff] [review])
This patch has been applied to https://bugzilla-stage-tip.mozilla.org/ and I have disabled the alternate hostname and turned this on (off actually) in its place.

I'd appreciate any testing and feedback from the usual security folks.  Just to clarify, this is NOT what we'll be using on b.m.o, this is just the fallback to prevent other bugzilla sites from getting screwed when we disclose this if they can't do the multi-domain setup.

	Daniel Veditz [:dveditz]
	Comment 26 • 16 years ago

Comment on attachment 355849 [details] [diff] [review]
patch, v1

>+            In order to view the attachment, your first have to
>+            <a href="attachment.cgi?id=[% attachment.id %]">download it</a>.

"you" first have to download it.

	Max Kanat-Alexander Reporter
	Comment 27 • 16 years ago

Okay, your patch looks great and works just fine. :-) However, I wanted to change up the Parameters page a bit in a way that was easier to do than to explain. :-) So I just changed around some of the text. It should be fine, since the attachmentbase bug probably won't change before we release (it still applies fine on the tip). You can review my text changes if you want, though. But all your code stuff is fine and works in all the browsers that I could get my hands on.

	Max Kanat-Alexander Reporter
	Comment 28 • 16 years ago

Oh, though we should be using CSS to bold the view_disabled td instead of <b> tags. That's not so important that I'd r- the patch, though.

	Max Kanat-Alexander Reporter
	Comment 29 • 16 years ago

(In reply to comment #26)
> "you" first have to download it.

  Oh, I think I also forgot to fix that typo, so that should be done in backports or on checkin.

	Max Kanat-Alexander Reporter
	Comment 30 • 16 years ago

Fixed the typo.

	Frédéric Buclin Assignee
	Comment 31 • 16 years ago

Patch v2.1 applies cleanly on tip and 3.2 and works as expected. There is some (minor?) bitrot in 3.0. Patch coming...

	Frédéric Buclin Assignee
	Comment 32 • 16 years ago

Backport for 3.0.6. The only change compared to 3.2+tip is that I moved the isurl block in attachment/edit.html.tmpl before the allow_attachment_display block to match how 3.2 and 3.3 work. Bugzilla 3.2 and 3.3 already had this change made.

	Frédéric Buclin Assignee
	Comment 33 • 16 years ago

Trivial backport for 2.22.6. Same change about the isurl block as for 3.0.6.

	Max Kanat-Alexander Reporter
	Comment 34 • 16 years ago

Comment on attachment 358707 [details] [diff] [review]
patch for 3.0, v1

r=mkanat by inspection

	Max Kanat-Alexander Reporter
	Comment 35 • 16 years ago

Comment on attachment 358709 [details] [diff] [review]
patch for 2.22, v1

This looks right to me, I'm assuming you tested it, and I can't think of any particular corner cases to test that you wouldn't have.

	Frédéric Buclin Assignee
	Comment 36 • 16 years ago

OK, all backports are ready for checkin.

	Dave Miller [:justdave]
	Comment 37 • 16 years ago

Comment on attachment 358113 [details] [diff] [review]
patch for 3.2 + tip, v2.1

This has been running on the bmo staging server with this option enabled for a week or so, I've heard positive feedback from one of our security guys, and another said he was going to look at it on Friday and I haven't heard anything, so I'll take that as a good sign.  Seems to work as designed though.

	Frédéric Buclin Assignee
	Comment 38 • 16 years ago

tip:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.152; previous revision: 1.151
done
Checking in Bugzilla/Config/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Attachment.pm,v  <--  Attachment.pm
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/admin/params/attachment.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/attachment.html.tmpl,v  <--  attachment.html.tmpl
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.56; previous revision: 1.55
done
Checking in template/en/default/attachment/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.40; previous revision: 1.39
done


3.2:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.144.2.4; previous revision: 1.144.2.3
done
Checking in Bugzilla/Config/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Attachment.pm,v  <--  Attachment.pm
new revision: 1.3.4.2; previous revision: 1.3.4.1
done
Checking in template/en/default/admin/params/attachment.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/attachment.html.tmpl,v  <--  attachment.html.tmpl
new revision: 1.4.2.2; previous revision: 1.4.2.1
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.51.2.2; previous revision: 1.51.2.1
done
Checking in template/en/default/attachment/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.38.2.1; previous revision: 1.38
done


3.0.6:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.126.2.3; previous revision: 1.126.2.2
done
Checking in Bugzilla/Config/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Attachment.pm,v  <--  Attachment.pm
new revision: 1.3.2.2; previous revision: 1.3.2.1
done
Checking in template/en/default/admin/params/attachment.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/attachment.html.tmpl,v  <--  attachment.html.tmpl
new revision: 1.3.2.2; previous revision: 1.3.2.1
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.41.2.6; previous revision: 1.41.2.5
done
Checking in template/en/default/attachment/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.37.2.1; previous revision: 1.37
done


2.22.6:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.103.2.3; previous revision: 1.103.2.2
done
Checking in Bugzilla/Config/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Attachment.pm,v  <--  Attachment.pm
new revision: 1.2.2.1; previous revision: 1.2
done
Checking in template/en/default/admin/params/attachment.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/attachment.html.tmpl,v  <--  attachment.html.tmpl
new revision: 1.2.2.1; previous revision: 1.2
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.30.2.3; previous revision: 1.30.2.2
done
Checking in template/en/default/attachment/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.25.2.3; previous revision: 1.25.2.2
done

	Max Kanat-Alexander Reporter
	Comment 39 • 16 years ago

Removing this bug from the security group, as the Security Advisory was sent (bug 468249)

	Frédéric Buclin Assignee
	Comment 40 • 13 years ago

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.2/
added t/test_security.t
Committed revision 207.

	Frédéric Buclin Assignee
	Comment 41 • 13 years ago

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.0/
added t/test_security.t
Committed revision 196.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/3.6/
added t/test_security.t
Committed revision 154.