Open stitchng opened 5 years ago
@stitchng @mozfreddyb +@koto
Following up from https://github.com/mozilla/standards-positions/issues/20#issuecomment-513187227
We seem to agree with his (@mozfreddyb) position on a quote (from him) below:
"Automatically sanitize within the APIs that parse strings into HTML (e.g., innerHTML). One could also debate exposing a sanitizer API to the DOM"
As I see it, the value of wicg/TT (to distinguish from StitchNG's proposal) is threefold:
I'll take these ideas one at a time:
Automatically sanitize within the APIs that parse strings into HTML
This seems like a fine option for certain applications but any candidate solution to DOM XSS will have to be a good migration target for existing apps, and blanket policies are simply not good migration targets.
If one part of an application relies on getting carefully crafted, unsanitized content to a browser API, then no part of the application can run under this regime, unless you have a way of marking certain content as exempt from this default policy.
The larger the app, the more likely there is at least one case where content cannot be auto-sanitized, so you need exemptions; preferably auditable, type-safe exemptions.
That's why wicg/TT treats TrustedXYZ as exempt from the default policy for strings.
wicg/TT does enable exactly this posture via its default policy mechanism, but when an app grows to need to do something funky, you're not out of options.
One could also debate exposing a sanitizer API to the DOM
This seems like a great idea and would make writing certain wicg/TT policies much easier. I'd love to work on this too.
The developer has to write code against each DOM sink like so:
First, these examples all show developers creating a policy and then immediately using it with a sink. It's hard to come up with short examples, but that is not how we've used this internally for server-side XSS. We prefer policy code be sequestered in a small number of sensitive files, and it usually is not colocated with the code that uses browser APIs.
Back to your question: Do they really? Let me table out what happens when a developer fails to think about x
in:
document.getElementById('main-page').innerHTML = x;
Knows x is HTML | Knows x is Trustworthy | Without wicg/tt | With wicg/tt |
---|---|---|---|
False | False | May be vulnerable | Fails safe or default policy sanitizes |
False | True | May be vulnerable | Fails safe or default policy sanitizes |
True | False | May be vulnerable | Fails safe or default policy sanitizes |
True | True | ok† | ok |
† - ok, but without explicit auditing a security specialist can't double check.
Yes, if you're going to use a sink correctly you have to have some basic knowledge about the kind of input that suggests. That's true either way.
But it seems to me that with wicg/tt the developer has to think about strictly less. The developer who is using wicg/tt doesn't have to worry about whether it's trustworthy, just whether it's HTML. And via the reporting API, they can get feedback about when their assumptions are broken, which is unavailable to the developer who is not using wicg/tt.
What am I missing?
Yes, it is a proposal to do policy configuration in JavaScript .... However, we do see a fault in allowing policy configurations in JavaScript. ...
This seems unlike other issues you've raised, in that it doesn't seem to come from a different design philosophy, different priorities, or different goals.
As such, it seems like something that could be folded into either proposal if we succeed in hashing out our more fundamental differences.
@mikesamuel i'll take it from here as i'm the author of this counter proposal.
I'll now try to properly explain my proposal for TrustedTypes as i see there are certain parts of my proposal you do not completely recognize.
This seems like a fine option for certain applications but any candidate solution to DOM XSS will have to be a good migration target for existing apps, and blanket policies are simply not good migration targets.
Firstly, I would like to say that my proposal has no blanket policies
so therefore the idea of exemptions
do not arise. This proposal makes use of the policies defined and listed in the CSP response header. The key idea in this proposal is to connect the behavior of DOM sinks to policies defined and listed in CSP response headers and use the definitions from createHTML()
withing say the innerHTML
DOM API to sanitize the string which will be passed to createHTML()
eventually - but within the descriptor set()
method for innerHTML
. This also applies to createScriptURL()
and the DOM sink it applies to.
/**!
* This is a fictitious (and contrived/feigned) API that is
* meant to expose the details of the policies registered
* via the "trusted-types" directive in the CSP header to the
* DOM sinks
*/
window.CSP = {}; // contrived API (partially part of my proposal. relevant only to make a point)
/**!
* Define a trusted type policy as 'default'
*
*
*/
window.TrustedTypes.HTML.registerPolicySanitizer('default', function(){
return function(dirtyHTML){
return window.DOMPurify.sanitize(dirtyHTML)
}
});
<meta http-equiv="Content-Security-Policy" content="trusted-types default">
/**!
* This code monkey-patches of the `innerHTML` DOM API
* descriptor setter with the aim of becoming aware of trusted types
* policies and use them for sanitizing setter values (HTML strings)
*
*/
// get descriptor (may not work in chrome - luckily we have an alternative)
var originalDesc_innerHTML = Object.getOwnPropertyDescriptor(Element.prototype, 'innerHTML');
if(typeof originalDesc_innerHTML == "undefined"){
// use "__lookupSetter__" & "__defineSetter__" as Chrome/Safari doesn't let you access property descriptors
originalDesc_innerHTML = {set:Object.prototype.__lookupSetter__.call(HTMLElement.prototype, "innerHTML")}
Object.prototype.__defineSetter__.call(HTMLElement.prototype, "innerHTML", function value(value){
// { window.CSP.trustedTypesPolicies } is a conceptual API to get the trusted type policies listed in the CSP header
var trusted_types = window.CSP.trustedTypesPolicies || ['default'];
var registration = {trustedtype:{config:{},type:null},sanitizerFn:function(val){ return val; }}
if(typeof trusted_types === 'string'){
trusted_types = trusted_types.split(' ');
}
if(trusted_types.length == 1){
registration = window.TrustedTypes.htmlRegistrations[trusted_types[0]];
}
var old_value = String(value);
// the sanitizer registered to a trusted type policy is executed
var new_value = registration.sanitizerFn(old_value)
// if inclusions are configured to be blocked then do not execute the descriptor setter
if(registration.trustedType.config.blockIncludes){
return old_value;
}
return originalDesc_innerHTML.set.call(this, new_value);
});
} else {
Object.defineProperty(Element.prototype, 'innerHTML', {
configurable:originalDesc_innerHTML.configurable,
enumerable:originalDesc_innerHTML.enumerable,
get:originalDesc_innerHTML.get,
set: function innerHTML(value) {
var trusted_types = window.CSP.trustedTypesPolicies || ['default'];
var registration = {trustedtype:{config:{},type:null},sanitizerFn:function(val){ return val; }}
if(typeof trusted_types === 'string'){
trusted_types = trusted_types.split(' ');
}
if(trusted_types.length == 1){
// get the object that holds the "createHTML()` method
registration = window.TrustedTypes.htmlRegistrations[trusted_types[0]];
}
var old_value = String(value);
// the sanitizer registered to a trusted type policy is executed
var new_value = registration.sanitizerFn(old_value)
// if inclusions are configured to be blocked then do not execute the descriptor setter
if(registration.trustedType.config.blockIncludes){
return old_value;
}
// Call the original setter
return originalDesc_innerHTML.set.call(this, new_value);
}
});
}
I do hope you can now understand the core of the idea in my proposal. The code above works on all major browsers (luckily in IE8 too).
The larger the app, the more likely there is at least one case where content cannot be auto-sanitized, so you need exemptions; preferably auditable, type-safe exemptions.
As i said before, exemptions
do not arise because my proposal doesn't make room for blanket policies
. You also mentioned auto-sanitization
. This proposal doesn't also make room for that too. Again, exemptions
do not arise. From your comment on type-safe exemptions
, type safety is what i really want to discuss. My understanding of trusted types reveals these types as mostly value-objects. Being able to manipulate the values (strings mostly) passed to them. There is a sense of type-ness in these trusted types but not enough as describes the set of operations that can be performed on them (trusted types) for which other operations stand invalid. For example, can trusted types for an document.body.firstElementChild.nextSibling.innerHTML
DOM sink be used with other trusted types from say document.body.getAttributeNode('class').nodeValue
. In fact does nodevalue
present a HTML/DOM trusted type or a DOM string according to the current spec direction ? Also what are the security (XSS) ramifications of not doing this:
const TrustedTypePolicy = window.TrustedTypes.createPolicy('my-policy', {
createHTML(potentiallyUnsafeHtml){
return DOMPurify.sanitize(potentiallyUnsafeHtml)
}
});
document.getElementById("upper-card").classList.add(
// an attribute value could contain potentially malicious code so how do we sanitize ?
TrustedTypePolicy.createHTML(
document.body.getAttributeNode('class').nodeValue
)
);
as opposed to this:
document.getElementById("upper-card").classList.add(
document.body.getAttributeNode('class').nodeValue
);
for the current spec direction for TrustedTypes
when a policy is included in a CSP header ?
First, these examples all show developers creating a policy and then immediately using it with a sink. It's hard to come up with short examples, but that is not how we've used this internally for server-side XSS. We prefer policy code be sequestered in a small number of sensitive files, and it usually is not colocated with the code that uses browser APIs.
I do know that code for policies have to be hidden away is some file. @stitchng was making a quick example that didn't require the elaborateness of how policies are defined in real-life web application projects.
Yes, if you're going to use a sink correctly you have to have some basic knowledge about the kind of input that suggests. That's true either way.
This is what i believe will create developer apathy. I am a web developer and i think web developers already have a lot to think about while creating web apps. The current form of the API design for TrustedTypes
should make it more ergonomic requiring less cognitive overhead.
As such, it seems like something that could be folded into either proposal if we succeed in hashing out our more fundamental differences.
I look forward to this and will work with you and other stakeholders to make this into either proposal. I do not wish that this counter proposal be seen as a competitor to the current proposal as is. It is not. rather it is meant to evoke more discussions around the durability of the current proposal and also a means of adding ideas into the current proposal such as this policy configuration
idea.
Thanks for your interest in this and the feedback!
Could you briefly highlight the key difference between Trusted Types as specced and your proposal? I see a different API shape (it's not clear what exact shape is the proposed one, as we only get a few conflicting code examples), but I'm not sure I understand yet what the key difference is. From what I'm getting you essentially describe what our default policies do, and you have some additional behaviour that assumes existence of other native APIs (like URL sanitizers, URL types API, or CSP API) that simply don't exist, and some of them are very unlikely to exist in the future.
You can check how TT API behaves - apart from the spec that describes it, the implementation is already in Chrome, there's also the polyfill available. So, for example:
innnerHTML
setters requires a TrustedHTML
, reading from innerHTML
gives you a string.Re: Developer cognitive load, I'm not sure that I understand yet what is the advantage of your proposal of the API. From what I can tell, your example is quite similar to https://gadgets.kotowicz.net/poc/tt/demo-dompurify/?tt=1, for which the crucial functionality is simply:
TrustedTypes.createPolicy('default', {
createHTML: (s) => {
return 'Sanitizing anyway :) ' + DOMPurify.sanitize(s)
},
});
All the configuration knobs like throwErrors
, blockIncludes
are not really needed in JS, as we have existing and well-defined reporting capabilities in CSP. If you want to configure that from within JS program - you can, in the policy function body, but we default to something that already works well and is simple to grasp.
There is a alternate proposal from @isocroft on TT which can be found here as a proof of concept and his thoughts around this is to help the ongoing discussions on how TT should be rolled out in browsers and also how simple the web developer experience should be. The idea is to "invert control" in a sense to make it possible to reduce the cognitive inertia that web developers today might have with the current spec direction of TT.
So, DOM sinks like
innerHTML
for example have knowledge of using a registered policy santizer to act on any (perhaps potentially unsafe) HTML string passed to it. Its behavior can also be modified accordingly too.window.TrustedTypes.HTML.registerPolicySanitizer('alt-policy', function(TrustedType){ window.DOMPurify.addHook('afterSanitizeElements', function(currentNode, data, config){ // more code here });
});
window.TrustedTypes.URL.registerPolicySanitizer('alt-policy', function(TrustedType){ return function(url, category){ // URISanity is a ficticious URL sanitizer (doesn't exist - yet) return window.URISanity.vet(url, category); }; })
/=== configurations ===/
/ blockIncludes: blocks including potentially unsafe HTML strings into the DOM hence modifying the behavior of
innerHTML
/ /throwErrors: throws errors when the sanitizer detects unsafe HTML string content / window.TrustedTypes.HTML.config = { throwErrors:true, blockIncludes:true, reportViolation:false };/ blockNavigation: blocks navigating to potentially unsafe URLs hence modifying the behavior of
location.href
orlocation.assign()
/ /throwErrors: throws errors when the sanitizer detects unsafe URL string content / window.TrustedTypes.URL.config = { throwErrors:true, blockNavigation:true, reportViolation:false };/ In the example below,
innerHTML
throws aTrustedTypesError
because of the "ping" attribute and also does include the HTML string into the DOM too/ document.body.getElementsByName('wrapper')[0].innerHTML += 'Hello World!';/ This will also work for other DOM sinks and chokepoints too / document.body.lastElementChild.insertAdjacentTML('afterbegin', 'Hello there...');
/ Also for this too, the behviour of assign is modified by / document.location.assign("http://my.nopoints.edu.ng/_/profiling/28167/#section1")
The above actually proposes programmatic configurability over declarative as it is more cheaper and also doesn't require the web developer to keep all 70 DOM sinks in mind as he/she writes code based on TT. It also proposes that types should not be proliferated to deal with each kind on data passed around on the front-end. The use of a policy trusted ( types group ) or ( types form - a he (@isocroft) calls it) might be more efficient going forward.
For example: URIs for scripts / dynamic resources / stylesheets can come under a single types group or types form : URL
So, for stylesheets, there would also be:
We would love your take on this alternate proposal on TT. The POC implementation of the above is here. You can try it out yourselves to see how it works.