Sanitize Google Visualization API parameters.

The existing taming attempted to prevent the allowHtml option from being
set, but did not filter all of the means of setting options.
Additionally, there are other parameters that can be used to enable
interpretation of some fields as HTML.

Therefore, introduce sanitization for inputs to the API, based on
whitelists of allowed keys in objects. (This is not fully strict;
many parameters are allowed to be arbitrary strings where their
definition does not seem likely to later permit arbitrary HTML.)

Also added some automated tests which will check for HTML leakage and
could be extended to be general functionality tests.

Committed as 01ed526f784ecf0121f64dd3c891faaaef6cf2fc .

[kpreid_google, 2016-02-09 17:02:49]

Note: This is not yet complete; in addition to some TODOs, I just haven't
finished going through the docs to whitelist everything that should be
whitelisted. However, I want to get the general approach reviewed ASAP so it can
be deployed quickly once it's done.

[metaweta, 2016-02-09 17:08:49]

I won't have time to get to this for a few days.

On Tue, Feb 9, 2016 at 9:02 AM,  <kpreid@google.com> wrote:
> Note: This is not yet complete; in addition to some TODOs, I just
> haven't finished going through the docs to whitelist everything that
> should be whitelisted. However, I want to get the general approach
> reviewed ASAP so it can be deployed quickly once it's done.
>
> https://codereview.appspot.com/286220043/



-- 
Mike Stay - metaweta@gmail.com
http://www.cs.auckland.ac.nz/~mike
http://reperiendi.wordpress.com

[kpreid_google, 2016-02-09 17:10:22]

On 2016/02/09 17:08:49, metaweta wrote:
> I won't have time to get to this for a few days.

OK — whoever can, please take this!

[kpreid_google, 2016-02-10 01:49:05]

The existing taming attempted to prevent the allowHtml option from being
set, but did not filter all of the means of setting options.
Additionally, there are other parameters that can be used to enable
interpretation of some fields as HTML.

Therefore, introduce sanitization for inputs to the API, based on
whitelists of allowed keys in objects. (This is not fully strict;
many parameters are allowed to be arbitrary strings where their
definition does not seem likely to later permit arbitrary HTML.)

Also added some automated tests which will check for HTML leakage and
could be extended to be general functionality tests.

[MarkM, 2016-02-10 04:54:30]

Passed my eye over everything in case anything jumped out. Nothing did, but much
I did not understand. Reviewed for JS and ses issues.

LGTM

https://codereview.appspot.com/286220043/diff/20001/src/com/google/caja/apita...
File src/com/google/caja/apitaming/google.visualization.policyFactory.js
(right):

https://codereview.appspot.com/286220043/diff/20001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:23: 
All functions at the top level of a file should start with "use strict";
here and elsewhere.

https://codereview.appspot.com/286220043/diff/20001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:46: 
no trailing whitespace, here and elsewhere

https://codereview.appspot.com/286220043/diff/20001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:379: // Not
documented but legend can take on string values apparently corresponding to
legend.position.
80 columns, here and elsewhere.

[kpreid_google, 2016-02-10 19:41:48]

The existing taming attempted to prevent the allowHtml option from being
set, but did not filter all of the means of setting options.
Additionally, there are other parameters that can be used to enable
interpretation of some fields as HTML.

Therefore, introduce sanitization for inputs to the API, based on
whitelists of allowed keys in objects. (This is not fully strict;
many parameters are allowed to be arbitrary strings where their
definition does not seem likely to later permit arbitrary HTML.)

Also added some automated tests which will check for HTML leakage and
could be extended to be general functionality tests.
code style per review.

[kpreid_google, 2016-02-10 19:43:16]

This is now complete — I've eyeballed all the manual side-by-side tests for
as-good-as-they-used-to-be results.

https://codereview.appspot.com/286220043/diff/20001/src/com/google/caja/apita...
File src/com/google/caja/apitaming/google.visualization.policyFactory.js
(right):

https://codereview.appspot.com/286220043/diff/20001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:23: 
Done. Note that these are all host-side files; historically we didn't assume the
browser supported strict mode. But this is definitely an improvement here (this
file) and now (everyone supports strict mode).

https://codereview.appspot.com/286220043/diff/20001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:46: 
On 2016/02/10 04:54:30, MarkM wrote:
> no trailing whitespace, here and elsewhere

Done.

https://codereview.appspot.com/286220043/diff/20001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:379: // Not
documented but legend can take on string values apparently corresponding to
legend.position.
Done except where this would be changing the style of untouched lines. Since
this is a security patch I want to keep the diff small.

[felix8a, 2016-02-11 09:53:11]

meta: I haven't looked at the gviz taming before. This is punishingly difficult
to audit, partly because it's a huge api surface, partly because the
sanitization seems fragile. I wish it were possible to do machine checking for
things like typos.

I looked that the code does what it says it should do, but I haven't checked
that the sanitization matches the gviz api, except in a few places.

I'm wary of all the String parameters, because it seems difficult to verify what
happens to them within gviz. I've marked one that seems like it might be a
problem, and I looked at a few others, but I didn't look at all the Strings
systematically or exhaustively, because I'm guessing it would take roughly
10-ish hours of work to audit all that. (What I looked at already took 2-ish
hours of work.)

I guess this is LGTM? but I don't feel very happy about it.

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
File src/com/google/caja/apitaming/google.visualization.policyFactory.js
(right):

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:96: //
arbitrary CSS will be passed through. Docs say only "HTML color string".
it looks to me like color strings are sometimes parsed with goog.color.parse and
are sometimes kept verbatim, and then eventually become an SVG paint value or a
VML IVgColor value.
https://www.w3.org/TR/SVG/painting.html#SpecifyingPaint
https://msdn.microsoft.com/en-us/library/bb264106(v=vs.85).aspx

I don't understand what the IVgColor spec means, but conservatively the
sanitization here looks safe to me.

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:638: 'type':
String,  // TODO(kpreid): tighten up to enum
any particular reason these keys are quoted?

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:650: case
'number':
these cases fall through to default so they can be deleted, right?

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:755:
chartType: String,
it looks to me like chartType, controlType (and the setChartType, setControlType
methods) allow me to call an arbitrary constructor by name. Like if I set {
chartType: 'String' } it will successfully invoke 'new String()' but then error
out when the constructed object doesn't behave as expected.

Is that safe?

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:785: 
extra whitespace

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:793:
specOut.containerId = utils.opaqueNodeById(containerId);
shouldn't this also stringify containerId like in wrapperCtorFilter?

https://codereview.appspot.com/286220043/diff/40001/tests/com/google/caja/api...
File tests/com/google/caja/apitaming/test-security-guest.html (right):

https://codereview.appspot.com/286220043/diff/40001/tests/com/google/caja/api...
tests/com/google/caja/apitaming/test-security-guest.html:43: <div
class="testcontainer" id="testChartWrapperSpecAllowHtml"></div>
it bothers that this long name has to be repeated N times exactly or else the
test doesn't actually test the right thing, but I don't have any idea how to fix
it.

[kpreid_google, 2016-02-11 18:37:32]

The existing taming attempted to prevent the allowHtml option from being
set, but did not filter all of the means of setting options.
Additionally, there are other parameters that can be used to enable
interpretation of some fields as HTML.

Therefore, introduce sanitization for inputs to the API, based on
whitelists of allowed keys in objects. (This is not fully strict;
many parameters are allowed to be arbitrary strings where their
definition does not seem likely to later permit arbitrary HTML.)

Also added some automated tests which will check for HTML leakage and
could be extended to be general functionality tests.
code style per review.

[kpreid_google, 2016-02-11 18:38:01]

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
File src/com/google/caja/apitaming/google.visualization.policyFactory.js
(right):

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:638: 'type':
String,  // TODO(kpreid): tighten up to enum
On 2016/02/11 09:53:11, felix8a wrote:
> any particular reason these keys are quoted?

No, removed.

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:650: case
'number':
On 2016/02/11 09:53:11, felix8a wrote:
> these cases fall through to default so they can be deleted, right?

Yes, but I put them in for documentation. "I have thought about these cases
explicitly." I'll remove them if you feel this is a mistake.

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:755:
chartType: String,
On 2016/02/11 09:53:11, felix8a wrote:
> it looks to me like chartType, controlType (and the setChartType,
setControlType
> methods) allow me to call an arbitrary constructor by name. Like if I set {
> chartType: 'String' } it will successfully invoke 'new String()' but then
error
> out when the constructed object doesn't behave as expected.
> 
> Is that safe?

Not particularly. Added an enum restriction.

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:785: 
On 2016/02/11 09:53:11, felix8a wrote:
> extra whitespace

Done.

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:793:
specOut.containerId = utils.opaqueNodeById(containerId);
On 2016/02/11 09:53:11, felix8a wrote:
> shouldn't this also stringify containerId like in wrapperCtorFilter?

I left that out because it's used exactly once, but we might as well not assume
that opaqueNodeById is itself cautious. Done.

https://codereview.appspot.com/286220043/diff/40001/tests/com/google/caja/api...
File tests/com/google/caja/apitaming/test-security-guest.html (right):

https://codereview.appspot.com/286220043/diff/40001/tests/com/google/caja/api...
tests/com/google/caja/apitaming/test-security-guest.html:43: <div
class="testcontainer" id="testChartWrapperSpecAllowHtml"></div>
On 2016/02/11 09:53:11, felix8a wrote:
> it bothers that this long name has to be repeated N times exactly or else the
> test doesn't actually test the right thing, but I don't have any idea how to
fix
> it.

Changed registerUnsafeHtmlTest to pass the id value in so it can be used.

It's still repeated in HTML and JS, but if the id specified in JS is missing,
the test framework will add it — at the cost of not being in the expected
position in the document.

[felix8a, 2016-02-11 19:16:58]

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
File src/com/google/caja/apitaming/google.visualization.policyFactory.js
(right):

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:650: case
'number':
On 2016/02/11 18:38:01, kpreid_google wrote:
> On 2016/02/11 09:53:11, felix8a wrote:
> > these cases fall through to default so they can be deleted, right?
> 
> Yes, but I put them in for documentation. "I have thought about these cases
> explicitly." I'll remove them if you feel this is a mistake.

I'd prefer a comment to the case statements. My gut reading of the case
statements was that the fallthrough is a typo and there's a missing statement,
and I had to think about it to decide that the fallthrough was intentional.

https://codereview.appspot.com/286220043/diff/60001/src/com/google/caja/apita...
File src/com/google/caja/apitaming/google.visualization.policyFactory.js
(right):

https://codereview.appspot.com/286220043/diff/60001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:1028:
v.ChartWrapper.prototype.setChartType = function(chartType) {};
need to sanitize chartType

https://codereview.appspot.com/286220043/diff/60001/tests/com/google/caja/api...
File tests/com/google/caja/apitaming/test-security-guest.html (right):

https://codereview.appspot.com/286220043/diff/60001/tests/com/google/caja/api...
tests/com/google/caja/apitaming/test-security-guest.html:37: 
whitespace

[kpreid_google, 2016-02-11 19:44:16]

The existing taming attempted to prevent the allowHtml option from being
set, but did not filter all of the means of setting options.
Additionally, there are other parameters that can be used to enable
interpretation of some fields as HTML.

Therefore, introduce sanitization for inputs to the API, based on
whitelists of allowed keys in objects. (This is not fully strict;
many parameters are allowed to be arbitrary strings where their
definition does not seem likely to later permit arbitrary HTML.)

Also added some automated tests which will check for HTML leakage and
could be extended to be general functionality tests.
code style per review.

[kpreid_google, 2016-02-11 19:44:54]

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
File src/com/google/caja/apitaming/google.visualization.policyFactory.js
(right):

https://codereview.appspot.com/286220043/diff/40001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:650: case
'number':
On 2016/02/11 19:16:58, felix8a wrote:
> I'd prefer a comment to the case statements.

Done.

https://codereview.appspot.com/286220043/diff/60001/src/com/google/caja/apita...
File src/com/google/caja/apitaming/google.visualization.policyFactory.js
(right):

https://codereview.appspot.com/286220043/diff/60001/src/com/google/caja/apita...
src/com/google/caja/apitaming/google.visualization.policyFactory.js:1028:
v.ChartWrapper.prototype.setChartType = function(chartType) {};
On 2016/02/11 19:16:58, felix8a wrote:
> need to sanitize chartType

Done.

https://codereview.appspot.com/286220043/diff/60001/tests/com/google/caja/api...
File tests/com/google/caja/apitaming/test-security-guest.html (right):

https://codereview.appspot.com/286220043/diff/60001/tests/com/google/caja/api...
tests/com/google/caja/apitaming/test-security-guest.html:37: 
On 2016/02/11 19:16:58, felix8a wrote:
> whitespace

Done.

[felix8a, 2016-02-11 19:45:39]

lgtm

[kpreid_google, 2016-02-11 20:45:24]

The existing taming attempted to prevent the allowHtml option from being
set, but did not filter all of the means of setting options.
Additionally, there are other parameters that can be used to enable
interpretation of some fields as HTML.

Therefore, introduce sanitization for inputs to the API, based on
whitelists of allowed keys in objects. (This is not fully strict;
many parameters are allowed to be arbitrary strings where their
definition does not seem likely to later permit arbitrary HTML.)

Also added some automated tests which will check for HTML leakage and
could be extended to be general functionality tests.
code style per review.

[kpreid_google, 2016-02-11 20:46:33]

New snapshot 6 just removes a useless assignment to an undefined variable, in
test-apitaming.js, that started throwing due to adding 'use strict' on MarkM's
recommendation.

[felix8a, 2016-02-11 20:47:38]

lgtm