Issue 7182049: Prevent host exceptions leaking through Domado in SES mode. (Closed)
|
Created:
11 years, 4 months ago by kpreid2 Modified:
11 years, 1 month ago CC:
caja-discuss-undisclosed_googlegroups.com, MarkM, ihab.awad, Jasvir, metaweta, MikeSamuel, felix8a Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionIn SES mode, if host code throws an exception and that exception is
caught by guest code, then the guest code can traverse its prototype
chain to host globals and wreak havoc. The taming membrane defends
against this by catching and rethrowing tamed exceptions; Domado,
unfortunately, did not do the same.
To close this hole, we repurpose confidence.protectMethod to perform
the same rethrowing, and additionally have it take over the job of
confidence.p: each protected method gets an additional argument which
is the private-fields record. Thus, the host objects stored in said
record are only obtained in scopes which also protect against exception
leakage. confidence.protectMethod is renamed to 'amplifying', and
confidence.p is removed.
(Unfortunately, closure-based tamings such as those for NodeList and
CanvasRenderingContext2D are not automatically protected by this
strategy.)
@r5341
Patch Set 1 #
Total comments: 127
Patch Set 2 : Prevent host exceptions leaking through Domado in SES mode. #Patch Set 3 : Prevent host exceptions leaking through Domado in SES mode. #Patch Set 4 : Prevent host exceptions leaking through Domado in SES mode. #Patch Set 5 : Prevent host exceptions leaking through Domado in SES mode. #Patch Set 6 : Prevent host exceptions leaking through Domado in SES mode. #
MessagesTotal messages: 15
btw, could you rebase this from trunk? merge conflict in domado.js
Sign in to reply to this message.
In SES mode, if host code throws an exception and that exception is caught by guest code, then the guest code can traverse its prototype chain to host globals and wreak havoc. The taming membrane defends against this by catching and rethrowing tamed exceptions; Domado, unfortunately, did not do the same. To close this hole, we repurpose confidence.protectMethod to perform the same rethrowing, and additionally have it take over the job of confidence.p: each protected method gets an additional argument which is the private-fields record. Thus, the host objects stored in said record are only obtained in scopes which also protect against exception leakage. confidence.protectMethod is renamed to 'amplifying', and confidence.p is removed. (Unfortunately, closure-based tamings such as those for NodeList and CanvasRenderingContext2D are not automatically protected by this strategy.)
Sign in to reply to this message.
On 2013/01/23 20:56:11, felix8a wrote: > btw, could you rebase this from trunk? merge conflict in domado.js Done.
Sign in to reply to this message.
https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... File src/com/google/caja/plugin/domado.js (right): https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:362: this.confide = cajaVM.def(function (object, taming, opt_sameAs) { Can you please write some Javadocs for the 'confide' method explaining 'sup? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:362: this.confide = cajaVM.def(function (object, taming, opt_sameAs) { As I understand it, purely from the point of view of the API of Confidence, 'opt_sameAs' might as well be an *array* of objects expected to be the "same as" the primary one being confided. Is there any reason why 'opt_sameAs' is a singleton rather than a collection -- apart from a coincidence of how Confidence is used by Domado at the moment? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:362: this.confide = cajaVM.def(function (object, taming, opt_sameAs) { What is the use case for one Confidence object being used with more than one taming membrane? If not, then can we just collapse that down to one taming membrane per Confidence? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:373: var proto = Object.getPrototypeOf(object); 'proto' is never used. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:380: privates = {_obj: object, _taming: taming}; Since I'm a nasty, suspicious kind of guy, what I would put as 'privates' would be: Object.freeze({ _obj: object, _taming: taming, state: {} }) and use '_obj' for sanity checks, but only expose to the 'amplifying'-ed methods the 'state' object, and keep the '_obj' part internal to the workings of the Confidence object. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:387: 'This operation requires a ' + typename); I'm trying to figure out if it's possible for the guard behavior to be totally internal to the Confidence, so that 'this.guard' need not be exposed. In so doing, more stuff gets automated in membrane-like fashion. This may be (a) something to do immediately; (b) a to-do for a future CL; or (c) a horrible idea whichever way you look at it. :) Perhaps we should discuss / maybe you can comment? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:390: * Wrap a method or other function so as to ensure that: One of the things we might check in an automated fashion (given that we do have a JS parser) is that amplifying() is always called with a function *literal*, which ensures that the wrapped object is never leaked. It's nontrivial of course but just a thought about how we might evolve this design. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:396: this.amplifying = function (method) { To my eyes, the gerund form does not make for the most easily understandable nomenclature, but this is just a minor pov / nit. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:397: //cajaVM.def(method); // unnecessary in theory. TODO(felix8a): verify I'm not sure how you might verify that this is unnecessary except by inspecting call sites to see that 'method' is not leaked in bare fashion, and evaluating the impact of cases where it might be. As such, it's no different than any other capability leak under the sun. I think this commented-out piece is confusing and should be ripped out. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:398: function amplifierMethod(var_args) { Looks good. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:404: return method.apply(this, ampargs); If 'method' returns a function, is *that* also wrapped in an exception-trapper? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:429: // no other wrapping. Interesting. I see the use case. But it seems like it'd be safer (in a belt, suspenders and second suspenders kind of way) to do exception wrapping here as well, on the assumption that it's unlikely to hurt, and might save a missed case. I'm not sure how to call this one. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:435: domitaModules.ExpandoProxyHandler = (function () { What's the argument for not exercising the same care with the methods of EPR? What happens if it throws an un-tamed exception? Can it? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:618: domitaModules.XMLHttpRequestCtor = function (makeDOMAccessible, This is not exposed so ok to not worry about. Check. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:691: var amplifying = TameXHRConf.amplifying; Maybe call this 'xhrAmp' or something to avoid confusion? #toomanyvariables https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:871: domitaModules.CssPropertiesCollection = function(aStyleObject) { Does this need exception wrapping? The argument for why *not* is that the guest-callable functions do not call the methods of 'styleObject' and are therefore not at risk of throwing "dangerous" host frame exceptions. Is this indeed our reasoning? Should we scribble it down somewhere? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:1636: // If running with proxies, it is indicative of something going wrong wrap line https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:1649: TameNodeConf.confide(proxiedNode, taming, node); Ok, I see 'sameAs' logic going on here. I also see why you need it -- some methods generated on the basis of the expando proxy, and some generated on the basis of the underlying behavior-ful tame node. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:1667: // If guest code passes a node of its own with no feral counterpart to wrap line https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:2248: get: innocuous(function (prop) { Should this exception-wrap? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:2251: set: innocuous(function (value, prop) { Same here. ? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:2772: tamed.namedItem = cajaVM.def(function(name) { We reason that this doesn't need exception guarding because it will, at worst, throw a taming-frame exception? Same for tameGetElementsByTagName, ... below? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3063: setOwn(TameNode.prototype, "toString", cajaVM.def(function() { Why is this not marked innocuous(...)? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3129: parentPolicy = undefined; // muffle linter Could this be written as: parentPolicy = nodeAmp(f(...) { return parentPriv.policy; }).call(...); ? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3321: TameBackedNode.prototype.contains = cajaVM.def(function(other) { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3349: cajaVM.def(function(tagName) { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3354: cajaVM.def(function(className) { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3400: var ownerPolicy = undefined; // assignment to muffle linter Once again can just use return value from nodeAmp? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3431: get: cajaVM.def(function() { innocuous? other cases also. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3565: TameElement.prototype.getAttributeNode = nodeAmp(function(privates, name) { wrap line https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3689: cajaVM.def(tameAddEventListener); innocuous? also below https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3846: for (var ancestor = makeDOMAccessible(privates.feral.parentNode); long line https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4099: definePropertiesAwesomely(tameImageData, { Is it a good idea to do a dPA *inside* an amplifying() block? The functions here (e.g. the one assigned to "get") have access to feral state and are therefore not easily auditable for innocuous-ness ... and they are not exception-wrapped. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4103: // blitting (getImageData -> putImageData) rather than inspecting long lines after indentation of code https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4162: } catch (e) { throw tameException(e); } I'm scared about the _ad hoc_ exception wrapping here. Why does this not use the Confidence and exception-wrapping pattern to protect access to the feral 'gradient'? Perhaps we should discuss further why this pattern is used in this section of the code.... https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4197: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4217: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4239: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4246: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4255: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4274: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4285: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4296: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4336: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4352: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4390: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4401: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4414: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4432: } catch (e) { throw tameException(e); } Exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4583: var policy; Again, can use return value of nodeAmp? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4595: taming.tamesTo(context, tameContext2d); Related to https://groups.google.com/forum/?fromgroups=#!msg/google-caja-discuss/d-Ykk4T... ? Why the change now as opposed to some other time? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5161: get: cajaVM.def(getter), innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5162: set: cajaVM.def(function(value, prop) { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5239: TameEvent.prototype.stopPropagation = eventAmp(function(privates) { By the by -- should we extend dPA and use it for methods on prototypes as well, so we are less likely to make mistakes? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5259: setOwn(TameEvent.prototype, "toString", cajaVM.def(function() { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5291: setOwn(TameCustomHTMLEvent.prototype, "toString", cajaVM.def(function() { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5337: attributes: { enumerable: true, get: cajaVM.def(function() { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5341: body: { enumerable: true, get: cajaVM.def(function() { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5351: get: cajaVM.def(function() { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5370: // authority to access 'document' Interesting. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5389: get: cajaVM.def(function() { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5393: set: cajaVM.def(function(value) { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5524: TameHTMLDocument.prototype.createEvent = nodeAmp(function(privates, type) { long line https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5552: Array.prototype.slice.call(arguments, 1)); indent 2 more spaces https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5577: domicile.setBaseUri = cajaVM.def(function(base) { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5596: domicile.signalLoaded = cajaVM.def(function() { innocuous? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5617: domicile.tameNode = cajaVM.def(defaultTameNode); innocuous? and for a bunch of others below .... https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5620: return nodeAmp(function(privates) { return privates.feral; }) There, *now* you use the return value of nodeAmp! :) https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5637: uriFetch(naiveUriPolicy, uri, mime, callback); Should this be protected? naiveUriPolicy might be implemented in the host and may throw a host-frame exception? domicile.fetchUri is not called directly by guest code but maybe we should be on the safe side here? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5646: return uriRewrite( same re naiveUriPolicy https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5684: domicile.rewriteUriInAttribute = cajaVM.def( ... and all these that call uriRewrite that calls naiveUriPolicy ... https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5853: var propName = allCssProperties.getCssPropFromCanonical(canonName); long line https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5870: throw 'Computed styles not editable: This code should be unreachable'; long line https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5928: domicile.cssUri = cajaVM.def(function (uri, mimeType, prop) { Again, protect uriRewrite for naiveUriPolicy? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5946: domicile.emitCss = cajaVM.def(function (cssText) { protect in case stylesheet creation throws host exception? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:6040: get: cajaVM.def(function() { innocuous?
Sign in to reply to this message.
In SES mode, if host code throws an exception and that exception is caught by guest code, then the guest code can traverse its prototype chain to host globals and wreak havoc. The taming membrane defends against this by catching and rethrowing tamed exceptions; Domado, unfortunately, did not do the same. To close this hole, we repurpose confidence.protectMethod to perform the same rethrowing, and additionally have it take over the job of confidence.p: each protected method gets an additional argument which is the private-fields record. Thus, the host objects stored in said record are only obtained in scopes which also protect against exception leakage. confidence.protectMethod is renamed to 'amplifying', and confidence.p is removed. (Unfortunately, closure-based tamings such as those for NodeList and CanvasRenderingContext2D are not automatically protected by this strategy.)
Sign in to reply to this message.
New snapshot. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... File src/com/google/caja/plugin/domado.js (right): https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:362: this.confide = cajaVM.def(function (object, taming, opt_sameAs) { On 2013/01/25 00:38:33, ihab.awad wrote: > Can you please write some Javadocs for the 'confide' method explaining 'sup? Done. On 2013/01/25 00:38:33, ihab.awad wrote: > As I understand it, purely from the point of view of the API of Confidence, > 'opt_sameAs' might as well be an *array* of objects expected to be the "same as" > the primary one being confided. Is there any reason why 'opt_sameAs' is a > singleton rather than a collection -- apart from a coincidence of how Confidence > is used by Domado at the moment? You have it exactly backwards; 'opt_sameAs' is an existing object which 'object' is to be treated the same as. I hope the newly added docs clarify this. And yet, your description of your understanding is so unlike what is actually occurring that I am confused and concerned. Please expand. On 2013/01/25 00:38:33, ihab.awad wrote: > What is the use case for one Confidence object being used with more > than one taming membrane? If not, then can we just collapse that down > to one taming membrane per Confidence? The Confidences are constructed per Domado() and the taming membrane is supplied to attachDocument(), so this is unavoidable if we don't change that. The case where those are different, which does not currently occur but may in the future, is two separate DOM Documents within the same frame/set-of-globals (note that an iframe is _not_ such). As far as I know, this flexibility is not actually used. I propose that in a separate patch we refactor so that the taming membrane is passed to Domado(). https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:373: var proto = Object.getPrototypeOf(object); On 2013/01/25 00:38:33, ihab.awad wrote: > 'proto' is never used. Deleted. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:380: privates = {_obj: object, _taming: taming}; On 2013/01/25 00:38:33, ihab.awad wrote: > Since I'm a nasty, suspicious kind of guy, what I would put as 'privates' would > be: > > Object.freeze({ > _obj: object, > _taming: taming, > state: {} > }) > > and use '_obj' for sanity checks, but only expose to the 'amplifying'-ed methods > the 'state' object, and keep the '_obj' part internal to the workings of the > Confidence object. I agree that that would be a more robust design, but I'd rather not do that as part of these changes. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:390: * Wrap a method or other function so as to ensure that: On 2013/01/25 00:38:33, ihab.awad wrote: > One of the things we might check in an automated fashion (given that we do have > a JS parser) is that amplifying() is always called with a function *literal*, > which ensures that the wrapped object is never leaked. It's nontrivial of course > but just a thought about how we might evolve this design. Agree that that would be a reasonable check for future work. Disagree that it would *prevent* anything from leaking. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:396: this.amplifying = function (method) { On 2013/01/25 00:38:33, ihab.awad wrote: > To my eyes, the gerund form does not make for the most easily > understandable nomenclature, but this is just a minor pov / nit. It returns an _amplifying_ function. Open to suggestions (unless they're 'amplifier', which is wrong because this function itself is not the amplifier). https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:397: //cajaVM.def(method); // unnecessary in theory. TODO(felix8a): verify On 2013/01/25 00:38:33, ihab.awad wrote: > I'm not sure how you might verify that this is unnecessary except by inspecting > call sites to see that 'method' is not leaked in bare fashion, and evaluating > the impact of cases where it might be. As such, it's no different than any other > capability leak under the sun. I think this commented-out piece is confusing and > should be ripped out. Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:404: return method.apply(this, ampargs); On 2013/01/25 00:38:33, ihab.awad wrote: > If 'method' returns a function, is *that* also wrapped in an exception-trapper? It's 'method's job to do that, as a proper participant in the membrane. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:435: domitaModules.ExpandoProxyHandler = (function () { On 2013/01/25 00:38:33, ihab.awad wrote: > What's the argument for not exercising the same care with the methods of EPR? > What happens if it throws an un-tamed exception? Can it? ExpandoProxyHandler does not deal in amplification. The principle is that we do exception-wrapping at the points at which we start being able to invoke feral code, and that occurs with Confidence (and sometimes closures). ExpandoProxyHandler is essentially on the 'tame' side of the membrane. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:691: var amplifying = TameXHRConf.amplifying; On 2013/01/25 00:38:33, ihab.awad wrote: > Maybe call this 'xhrAmp' or something to avoid confusion? #toomanyvariables Disagree. This is tightly lexically scoped. Think of it as a module import. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:871: domitaModules.CssPropertiesCollection = function(aStyleObject) { On 2013/01/25 00:38:33, ihab.awad wrote: > Does this need exception wrapping? > > The argument for why *not* is that the guest-callable functions do not call the > methods of 'styleObject' and are therefore not at risk of throwing "dangerous" > host frame exceptions. > > Is this indeed our reasoning? Should we scribble it down somewhere? There is only one CssPropertiesCollection, 'allCssProperties', and it is only used during initialization or from amplifying() methods, but this isn't enforced. Your concern is why I don't really like this solution. Some of this concern would go away if I finished the TODO'd refactoring of Style objects to use fully statically-assembled accessors rather than doing a bunch of dynamic stuff on a closed-over style property name, which is a relic from Domita which implemented Style objects like proxies. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:1636: // If running with proxies, it is indicative of something going wrong On 2013/01/25 00:38:33, ihab.awad wrote: > wrap line Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:1649: TameNodeConf.confide(proxiedNode, taming, node); On 2013/01/25 00:38:33, ihab.awad wrote: > Ok, I see 'sameAs' logic going on here. I also see why you need it -- some > methods generated on the basis of the expando proxy, and some generated on the > basis of the underlying behavior-ful tame node. "generated"? The issue is that a method invoked on the proxy will have a 'this' value which is the proxy, and we need to be able to amplify/guard on that. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:1667: // If guest code passes a node of its own with no feral counterpart to On 2013/01/25 00:38:33, ihab.awad wrote: > wrap line Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:2248: get: innocuous(function (prop) { On 2013/01/25 00:38:33, ihab.awad wrote: > Should this exception-wrap? No, because it forwards to something else which either is responsible for doing so or does not need to. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:2772: tamed.namedItem = cajaVM.def(function(name) { On 2013/01/25 00:38:33, ihab.awad wrote: > We reason that this doesn't need exception guarding because it will, at worst, > throw a taming-frame exception? Same for tameGetElementsByTagName, ... below? Correct. Taming frame exceptions may be visibly from-a-different-frame but their prototypes are frozen. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3063: setOwn(TameNode.prototype, "toString", cajaVM.def(function() { On 2013/01/25 00:38:33, ihab.awad wrote: > Why is this not marked innocuous(...)? Because I haven't done a complete review because nothing's actually auditing for it. There's a lot more work to be done in that direction. Most of the places I've used innocuous() are ones that used to be nodeMethod() and I determined didn't need to be nodeAmp() because they used no private state nor invoked closed-over feral objects. (One of the incidental consequences of this patch is that there are far fewer redundant amplifications of the same object, which is made possible by stuff like that.) https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3129: parentPolicy = undefined; // muffle linter On 2013/01/25 00:38:33, ihab.awad wrote: > Could this be written as: > > parentPolicy = nodeAmp(f(...) { return parentPriv.policy; }).call(...); > > ? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3321: TameBackedNode.prototype.contains = cajaVM.def(function(other) { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3349: cajaVM.def(function(tagName) { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3354: cajaVM.def(function(className) { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3400: var ownerPolicy = undefined; // assignment to muffle linter On 2013/01/25 00:38:33, ihab.awad wrote: > Once again can just use return value from nodeAmp? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3431: get: cajaVM.def(function() { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? other cases also. Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3565: TameElement.prototype.getAttributeNode = nodeAmp(function(privates, name) { On 2013/01/25 00:38:33, ihab.awad wrote: > wrap line Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3689: cajaVM.def(tameAddEventListener); On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? also below tame*EventListener are amp'd themselves; if they aren't this is a problem we'd like to notice. The cajaVM.def is just ... you don't not do that. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:3846: for (var ancestor = makeDOMAccessible(privates.feral.parentNode); On 2013/01/25 00:38:33, ihab.awad wrote: > long line Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4099: definePropertiesAwesomely(tameImageData, { On 2013/01/25 00:38:33, ihab.awad wrote: > Is it a good idea to do a dPA *inside* an amplifying() block? The functions here > (e.g. the one assigned to "get") have access to feral state and are therefore > not easily auditable for innocuous-ness ... and they are not exception-wrapped. No, it's not a good idea. But the canvas taming is written in a closure-based style, and I don't want to rewrite and un-nest it as part of these changes. (Separately, on the other hand, yes.) The closure-based structure also means it gets no protection from Confidence and must do its own exception wrapping. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4103: // blitting (getImageData -> putImageData) rather than inspecting On 2013/01/25 00:38:33, ihab.awad wrote: > long lines after indentation of code Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4162: } catch (e) { throw tameException(e); } On 2013/01/25 00:38:33, ihab.awad wrote: > I'm scared about the _ad hoc_ exception wrapping here. Why does this not use the > Confidence and exception-wrapping pattern to protect access to the feral > 'gradient'? Perhaps we should discuss further why this pattern is used in this > section of the code.... See above. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4583: var policy; On 2013/01/25 00:38:33, ihab.awad wrote: > Again, can use return value of nodeAmp? Could, but that would mean the function has a side-effect and a return value. I think that would be extra confusing. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:4595: taming.tamesTo(context, tameContext2d); On 2013/01/25 00:38:33, ihab.awad wrote: > Related to > https://groups.google.com/forum/?fromgroups=#%21msg/google-caja-discuss/d-Ykk... > ? Why the change now as opposed to some other time? That writeup is obsolete; as of some time between when I wrote it and when I was rehired, Domado was hooked into the taming membrane and so the claims there no longer hold. Here, I'm just making this consistent with the rest of Domado. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5161: get: cajaVM.def(getter), On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? No, the getter is what it is and we want to pass that through. Remember when you mentioned above that perhaps we should statically check for amplifying() being used on static methods? The same reasoning applies to innocuous() and would reject your change correctly! https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5162: set: cajaVM.def(function(value, prop) { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5239: TameEvent.prototype.stopPropagation = eventAmp(function(privates) { On 2013/01/25 00:38:33, ihab.awad wrote: > By the by -- should we extend dPA and use it for methods on prototypes as well, > so we are less likely to make mistakes? I don't understand the question. dPA does not protect against anything (except, incidentally, unfrozen getters/setters). https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5259: setOwn(TameEvent.prototype, "toString", cajaVM.def(function() { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. While you're at it, how about reminding me to use single quotes? https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5291: setOwn(TameCustomHTMLEvent.prototype, "toString", cajaVM.def(function() { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5337: attributes: { enumerable: true, get: cajaVM.def(function() { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5341: body: { enumerable: true, get: cajaVM.def(function() { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5351: get: cajaVM.def(function() { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5389: get: cajaVM.def(function() { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5393: set: cajaVM.def(function(value) { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5417: TameHTMLDocument.prototype.removeChild = nodeAmp(function(privates, remove) { You missed this one! :) https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5524: TameHTMLDocument.prototype.createEvent = nodeAmp(function(privates, type) { On 2013/01/25 00:38:33, ihab.awad wrote: > long line Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5552: Array.prototype.slice.call(arguments, 1)); On 2013/01/25 00:38:33, ihab.awad wrote: > indent 2 more spaces Done. By the way, that 'return' (which is not parallelled in writeln) looks suspicious; it is returning 'documentLoaded.promise' from HtmlEmitter, but per HTML5 document.write is void. Added a TODO to look into it. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5577: domicile.setBaseUri = cajaVM.def(function(base) { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? No, this (and everything else in 'domicile') is a host-side interface. If this function were to be exposed to guest code it would be a bug which we want to notice. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5596: domicile.signalLoaded = cajaVM.def(function() { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? No, this is a host-side interface. See above. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5617: domicile.tameNode = cajaVM.def(defaultTameNode); On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? and for a bunch of others below .... No, this is a host-side interface. See above. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5637: uriFetch(naiveUriPolicy, uri, mime, callback); On 2013/01/25 00:38:33, ihab.awad wrote: > Should this be protected? naiveUriPolicy might be implemented in the host and > may throw a host-frame exception? domicile.fetchUri is not called directly by > guest code but maybe we should be on the safe side here? I agree with your paranoia, but this isn't quite the right spot. I've added enforcement that the naiveUriPolicy is a tamed/caja-side object, and made ses-frame-group wrap it as such (which was more painful than one might think; see comments there). https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5853: var propName = allCssProperties.getCssPropFromCanonical(canonName); On 2013/01/25 00:38:33, ihab.awad wrote: > long line Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5870: throw 'Computed styles not editable: This code should be unreachable'; On 2013/01/25 00:38:33, ihab.awad wrote: > long line Done. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:5946: domicile.emitCss = cajaVM.def(function (cssText) { On 2013/01/25 00:38:33, ihab.awad wrote: > protect in case stylesheet creation throws host exception? In general, domicile is a host-side interface. emitCss is called from only two places (outside of tests): - in cajoled (ES5/3) modules, at the top level to load static CSS - from HtmlEmitter, which lives on the 'outside of the membrane'. https://codereview.appspot.com/7182049/diff/1/src/com/google/caja/plugin/doma... src/com/google/caja/plugin/domado.js:6040: get: cajaVM.def(function() { On 2013/01/25 00:38:33, ihab.awad wrote: > innocuous? Hm, domicile.pseudoLocation could be turned into a host object by some unwise party. I've added protection and a note to fix that.
Sign in to reply to this message.
In SES mode, if host code throws an exception and that exception is caught by guest code, then the guest code can traverse its prototype chain to host globals and wreak havoc. The taming membrane defends against this by catching and rethrowing tamed exceptions; Domado, unfortunately, did not do the same. To close this hole, we repurpose confidence.protectMethod to perform the same rethrowing, and additionally have it take over the job of confidence.p: each protected method gets an additional argument which is the private-fields record. Thus, the host objects stored in said record are only obtained in scopes which also protect against exception leakage. confidence.protectMethod is renamed to 'amplifying', and confidence.p is removed. (Unfortunately, closure-based tamings such as those for NodeList and CanvasRenderingContext2D are not automatically protected by this strategy and must be individually protected.)
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
this has a some conflicts vs trunk in domado.js. could you please resolve and update the patch?
Sign in to reply to this message.
In SES mode, if host code throws an exception and that exception is caught by guest code, then the guest code can traverse its prototype chain to host globals and wreak havoc. The taming membrane defends against this by catching and rethrowing tamed exceptions; Domado, unfortunately, did not do the same. To close this hole, we repurpose confidence.protectMethod to perform the same rethrowing, and additionally have it take over the job of confidence.p: each protected method gets an additional argument which is the private-fields record. Thus, the host objects stored in said record are only obtained in scopes which also protect against exception leakage. confidence.protectMethod is renamed to 'amplifying', and confidence.p is removed. (Unfortunately, closure-based tamings such as those for NodeList and CanvasRenderingContext2D are not automatically protected by this strategy and must be individually protected.)
Sign in to reply to this message.
On 2013/03/07 20:13:12, felix8a wrote: > this has a some conflicts vs trunk in domado.js. could you please resolve and > update the patch? Done.
Sign in to reply to this message.
In SES mode, if host code throws an exception and that exception is caught by guest code, then the guest code can traverse its prototype chain to host globals and wreak havoc. The taming membrane defends against this by catching and rethrowing tamed exceptions; Domado, unfortunately, did not do the same. To close this hole, we repurpose confidence.protectMethod to perform the same rethrowing, and additionally have it take over the job of confidence.p: each protected method gets an additional argument which is the private-fields record. Thus, the host objects stored in said record are only obtained in scopes which also protect against exception leakage. confidence.protectMethod is renamed to 'amplifying', and confidence.p is removed. (Unfortunately, closure-based tamings such as those for NodeList and CanvasRenderingContext2D are not automatically protected by this strategy and must be individually protected.)
Sign in to reply to this message.
New snapshot (Patch Set 6) fixes a 4×-10× slowdown by avoiding using confidence.amplifying at run-time (which, according to the Chrome profiler, is slow because it needs to def() two functions); I introduced confidence.amplify which skips constructing a wrapper function. Please review if you feel it necessary.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
