926085 - Forbid single quotes to delimit URLs (no <a href='...'>)

Status: RESOLVED FIXED

(Bugzilla :: Testing Suite, enhancement)

Description

[Frédéric Buclin]

While working on bug 924932, I wondered why some URLs were truncated. The reason is that some strings have single quotes in them and are involved in code like this:

<a href='http://domain.com/script.cgi?foo=[% bar FILTER uri %]'>...</a>

But the problem is that |FILTER uri| follows RFC 2396 which considers these five characters as unreserved (and so which do not need to be escaped): !,*,',( and ). This RFC has been obsoleted by RFC 3986 which now considers these five characters as reserved and so need to be escaped. URI::Escape left the obsolete RFC 2396 in favor of RFC 3986 in version 1.53 (March 2010), but Template-Toolkit still follows the old RFC, see https://rt.cpan.org/Public/Bug/Display.html?id=57590.

When a string involved in the URL above has a single quote in it, the URL is truncated prematurely. I didn't manage to exploit this vulnerability, because spaces, < and > are correctly escaped, but to prevent truncated URLs and potential security vulnerabilities, we should forbid single quotes to be used as delimiters for URLs. The single template being affected by this problem is reports/report-table.html.tmpl and will be fixed as part of bug 924932. But we should fix our testing suite to catch such patterns and complain loudly.

I'm restricting this bug to the security group till bug 924932 is fixed and public (i.e. when Bugzilla 4.4.1 is released).

Comment 1

[Frédéric Buclin]

Another reason to forbid single quotes is that encodeURIComponent() doesn't escape them either, and so if you build a URL inside a <script> </script>, you get the same problem (also found while working on bug 924932).

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

Comment 3

[Frédéric Buclin]

Security advisory sent.

Comment 4

[Frédéric Buclin]

FYI, TT 2.26 will escape single quotes, making this bug wontfix once we require 2.26, see https://github.com/abw/Template2/issues/13.

Comment 5

[David Lawrence [:dkl]]

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

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

r=dkl

Comment 6

[Frédéric Buclin]

Changes in the two templates have already been done on trunk when migrating from HTML4 to HTML5.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified t/004template.t
Committed revision 8914.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified t/004template.t
modified template/en/default/admin/params/attachment.html.tmpl
modified template/en/default/admin/params/auth.html.tmpl
Committed revision 8657.