Not obvious that "structure user_input" risks XSS attacks

[Matthew Paul Thomas]

In page templates:
* <foo tal:content="user_input" /> is safe
* <foo tal:content="structure user_input" /> potentially allows XSS attacks
* <foo tal:content="structure user_input/fmt:text_to_html" /> is safe.

I don't think it's immediately obvious, to a coder or a reviewer, that that the second of these is dangerous (unless user_input has been filtered beforehand) while the others are not. We have had XSS vulnerabilities before, so this is not a theoretical problem.

Possibly this could be fixed by changing the syntax in an Apps-Hungarian style: "structure" could be renamed to something that includes the word "unsafe", while "fmt:text_to_html" is renamed to something like "fmt:safe_html".

A related partial solution would be for either lint or the test suite to complain if a template contains tal:content="structure foo" or tal:replace="structure foo", unless "foo" matches some special marker signifying that it has been made safe.

[Steve Alexander (stevea)]

I've filed a bug on this elsewhere... I think it was as a Zope 3 enhancement.

Here's what should happen.

 - "structure" should always be seen as unsafe. Our code review tools and editors should be configured to highlight the word "structure" in page templates.

 - When some piece of Launchpad infrastructure is producing properly escaped HTML, it should be of a special type -- a subclass of unicode -- and the page template system should recognise this and automatically add an implicit "structure".

 - There should be a new TAL keyword "unstructured". So, we effectively get the default situation as "the code knows how to tell the template whether a string is structured or not, and we trust the code to escape things properly when necessary". And, we can manually override this in templates using "structured" and "unstructured".

 - We can do without "unstructured" if we note that we can just convert a "unicode with structure" into a "unicode", and then the template will escape it.

Doing this requires some changes to how TAL works. So this needs to be accepted upstream in Zope 3. I attempted this many months ago, but other Zope 3 developers didn't really see the point, perhaps because they use page templates a bit differently on their projects.

[Steve Alexander (stevea)]

This isn't an exploitable security vulnerability, so I'm unchecking that option.

This is a design issue that can make it easier to write and review insecure code.

[Steve Alexander (stevea)]

For Launchpad, a workaround is to normalize the XML of our page templates, then grep for "structure" that doesn't have a "text_to_html" after it, as part of our pre-merge checks.

[Michael Hudson-Doyle (mwhudson)]

I think it's worth pointing out that about half of the uses of structure in our tempaltes are to include some other view -- so while these could/should return a 'structured' type, any safety-motivated attempt to lint away uses of 'structure' will be way too annoying to be useful.

[Steve Alexander (stevea)]

We can use a naming convention to show when 'structure' is being used to include a page fragment from a view.