Better defaults for XSS protection

[Sam Minnée]

TL;DR - should we make the template engine escape HTML by default, and put up the the backward compatibility issues that is causes?

---

Since it was created, SilverStripe's template engine has come bundled with a content casting system built into ViewableData. This has meant that if you put "Sam & Co" into the Title field in the CMS and then put $Title into your template, it will appear as "Sam & Co".

The way it works:

- Cast each string value to an object, based on the value of the $db or $casting statics, and failing over to default_cast if necessary
- Call forTemplate() on that object.

The problem is that developers frequently create a lot of data that doesn't have a specific cast, and the default cast is used. When this default casting was introduced, it was set to "HTMLText", to minimise the change of backward compatibly errors.

However, this default option means that custom content often makes its way to templates unescaped, which is a leading cause of XSS errors.

I don't like XSS errors any more than I like SQL injections. One of the goals of the 3.0 ORM changes (and Tractorcow's subsequent work with prepared statements) was to remove SQL Injections from the path of least resistance. I would like do the same with XSS errors, with a change to the casting system.

---

The change needn't be complex. ViewableData already has a default_casting option, set to "HTMLText". We can simply change this to "Text".

There are a few things that would need to be marked as safe HTML and not double-escaped. The most straightforward way to do this is make them return HTMLText objects instead of strings. If coupled with a __toString() function that returns forTemplate(), this should be okay.

- $Layout & $Content, as handled automatically by the template engine.
- renderWith() results and/or SSViewer::process() calls.

Here's a first cut of the change:
https://gist.github.com/5053446

This will undoubtedly cause backwards-compatibility issues, but perhaps this is a pill that we must swallow in order to make SilverStripe more security against XSS attacks. Some of the problem spots:

- If you are constructing HTML in a controller function without making use of renderWith(), you will need to manually cast that as HTMLText.
- Since renderWith() returns an object and not a string, if you have code that relies on is_string() or !is_object() calls to identify generated HTML, that will need to change.

That said, the I applied the aforementioned patch to a pretty complex SilverStripe 3.0 site and the result was surprisingly functional.

Of the top of my head, this patch needs more work in a few areas:

- It doesn't provide any new APIs for specifying casting information. For example, it would be good to be able to set casting on ArrayData; it's tidier than a bunch of DBField::create_field() calls.
- As it stands, this patch also calls the Shortcode API on every single template now, which is a performance issue and also could make for some confusing logic. That's relatively easily fixed -- HTMLText::forTemplate() needs to be tweaked. We could introduce a new HTMLShortcodeText type, or have some way of setting instance-specific options on the casting objects (which would be more work).

However, my goal isn't to get the patch perfect just yet. It's to get an understanding of where it is appropriate to put this backward compatibility breakage.

---

So - what do people think? A change for 3.1? For 3.2? Or for a release beyond that?

[Marcus Nyeholt]

Yes, for 3.1. Fixing broken stuff is a better price to pay for increased overall security than trying to ask anonymous for your data back.

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to silverstripe-d...@googlegroups.com.
To post to this group, send email to silverst...@googlegroups.com.
Visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[Daniel Hensby]

Hi Sam,

Anything to increase security is a good idea from my point of view. However, with ss 3.1 in beta2, it seems a bit late to introduce it. As a dev I wouldn't expect a change like that to come in between a beta and rc/launch. However, I am biased as I'm currently about to launch a site running 3.1 beta...

However, I am interested in how you feel the system can currently be exploited as I've always been under the impression that almost all content was casted and I know as a dev, if it isn't, I need to make it safe, just like I had to with DO::get.

Lastly, if this change would require all templates / snippets that are parsed as html text to be valid mark up (matching opening and close tags), then I don't think it's a good idea - this is a current issue with 3.1. Out is my opinion that the framework should not be enforcing 'correct' html, especially not on fragments of html.

Dan

[Ingo Schommer]

I agree its a good change, but my feeling is to leave it for 3.2.

We've already stretched the definition of "beta" by introducing some API changes in beta2.

The more late stage API changes we do, the less early adopters trust in our release labelling,

which eventually will lead to a slower adoption of pre-releases (and less community validation).

This change makes an upgrade from beta2 very time intensive and error prone.

Every public method on controllers or models, every "renderWith()/customise()" or "return array(…)" invocation 

will need to be reviewed manually if its assumed to contain HTML, by looking closely at its template usage.

We should make the documentation for 3.0 and 3.1 clearer though

http://doc.silverstripe.org/framework/en/trunk/topics/security#xss-cross-site-scripting

http://doc.silverstripe.org/framework/en/trunk/reference/templates#formatting-and-casting

[Sam Minnée]

Having been working on some security audit work the last few weeks, my feeling is that, frankly, this is a security hole.  If we wanted a less invasive change, we could look at making ArrayData content typed as Varchar rather than HTMLVarchar by default.

If we were to put it into 3.1 we would need a beta3 release, although we're already considering a beta3 release for https://github.com/silverstripe/sapphire/pull/1244

On Thursday, 28 February 2013 23:04:34 UTC+13, Ingo Schommer wrote:

I agree its a good change, but my feeling is to leave it for 3.2.

We've already stretched the definition of "beta" by introducing some API changes in beta2.

The more late stage API changes we do, the less early adopters trust in our release labelling,

which eventually will lead to a slower adoption of pre-releases (and less community validation).

 

This change makes an upgrade from beta2 very time intensive and error prone.

Every public method on controllers or models, every "renderWith()/customise()" or "return array(…)" invocation 

will need to be reviewed manually if its assumed to contain HTML, by looking closely at its template usage.

We should make the documentation for 3.0 and 3.1 clearer though

http://doc.silverstripe.org/framework/en/trunk/topics/security#xss-cross-site-scripting

http://doc.silverstripe.org/framework/en/trunk/reference/templates#formatting-and-casting

Basically, the status q 

[Sam Minnée]

We should make the documentation for 3.0 and 3.1 clearer though

http://doc.silverstripe.org/framework/en/trunk/topics/security#xss-cross-site-scripting

http://doc.silverstripe.org/framework/en/trunk/reference/templates#formatting-and-casting

...press "Post" too soon.  The status quo is that most data you push into ArrayData needs to be sanitised with a construct like this:

       new DBField::create_field("Varchar", $val)

 

I've seen a lot of instances where this hasn't happened, which is why I'm inclined to say "The status quo is a security hole."  We are actively encouraging the creation of XSS errors in ArrayData constructs.