Replace Domado's editable flags with node policy objects determined by parents.

The opaque and foreign node restrictions as previously implemented are
completely broken, because they attempt to suppress ancestor methods
by overriding them, and the nodes are not even marked editable. To
achieve proper protection, all amplification-capable function objects
(that is, TameNode methods) must instead respect the intended policy.

To achieve this, several new flags are defined to capture the intended
behavior of opaque and foreign nodes. To maintain simplicity, all flags
(including the existing editability flags) are bundled into 'node
policy' objects, of which there are a small set.

Furthermore, the current taming membrane has as a premise that there is
only one tame object per feral object; but this is inconsistent with the
Domado design which determines editability flags according to the way
in which the node being tamed was reached. In order to avoid accidental
mutability of opaque nodes' children, the node policy of a node is now
almost always determined by the policy of its parent node -- with the
exceptions of opaque nodes and foreign nodes. Incidentally, an attempt
to tame a foreign node's descendant (which should never happen) is now
guaranteed to crash, as the foreign node policy refuses to specify a
child policy.

Foreign nodes' descendants are now hidden from NodeLists; this adds
the cost of scanning a prefix of the host NodeList to filter it, but the
results are cached as long as the guest does not modify the DOM.

Node editability and child-relationship editability checks have been
refactored to be implemented in one place each.

Also added a note about our slightly inaccurate implementation of
innerText.

@r5143

[kpreid2, 2012-09-12 19:04:47]

Patch Set 2 has been updated to base r5044 and slightly tweaked. Ready for
review.

[felix8a, 2012-09-13 19:53:57]

partial comments. I'm still working through the code in domado.js

https://codereview.appspot.com/6490106/diff/3001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/browser-test-case.js (right):

https://codereview.appspot.com/6490106/diff/3001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/browser-test-case.js:363: // (Note taming membrane
is in use here, so we get/return feral nodes.)
I don't understand the comment about feral nodes? 
es53-test-domado-dom-guest.html calls
  makeReadOnly(document.getElementById('testReadOnly'));
and since it's guest code, that's a tame document, so the arg received by
makeReadOnly will be a tame node, right?

the test is an expectFailure(), are you sure it's failing in the way you want it
to fail?

https://codereview.appspot.com/6490106/diff/3001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-domado-dom-guest.html (right):

https://codereview.appspot.com/6490106/diff/3001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domado-dom-guest.html:901: <p
id="testReadOnly" class="testcontainer">
I'm ok with using camelCase instead of hyphenation, but doing it piecemeal
leaves the file inconsistent with no clear indication of what direction to go. 
how about renaming all of it in another CL?

[kpreid2, 2012-09-13 20:43:59]

https://codereview.appspot.com/6490106/diff/3001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/browser-test-case.js (right):

https://codereview.appspot.com/6490106/diff/3001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/browser-test-case.js:363: // (Note taming membrane
is in use here, so we get/return feral nodes.)
On 2012/09/13 19:53:57, felix8a wrote:
> I don't understand the comment about feral nodes? 
> es53-test-domado-dom-guest.html calls
>   makeReadOnly(document.getElementById('testReadOnly'));
> and since it's guest code, that's a tame document, so the arg received by
> makeReadOnly will be a tame node, right?

The guest obtains frame.tame(makeReadOnly), so the taming membrane is in effect
here; the guest provides tame nodes and the feral side receives feral nodes.

> the test is an expectFailure(), are you sure it's failing in the way you want
it
> to fail?

Reasonably sure, but I've added a premise check to canvas-guest just in case.
dom-guest already had one (the indelible.innerHTML assert).

https://codereview.appspot.com/6490106/diff/3001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-domado-dom-guest.html (right):

https://codereview.appspot.com/6490106/diff/3001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-domado-dom-guest.html:901: <p
id="testReadOnly" class="testcontainer">
On 2012/09/13 19:53:57, felix8a wrote:
> I'm ok with using camelCase instead of hyphenation, but doing it piecemeal
> leaves the file inconsistent with no clear indication of what direction to go.

> how about renaming all of it in another CL?

This is an incremental fix for a long-standing bug, not a stylistic change. It
should be the case that the string passed to jsunitRegister === the element id
=== the string passed to pass(); the only reason this works is because our test
driver is an odd mix of jsUnit and our own DOM-twiddling extensions.

I agree we should fix all of them, and I'll add that to my todo list, but I
don't want to not-fix this one while I'm here.

[felix8a, 2012-09-13 21:01:55]

https://codereview.appspot.com/6490106/diff/3001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/browser-test-case.js (right):

https://codereview.appspot.com/6490106/diff/3001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/browser-test-case.js:363: // (Note taming membrane
is in use here, so we get/return feral nodes.)
On 2012/09/13 20:44:00, kpreid2 wrote:
> On 2012/09/13 19:53:57, felix8a wrote:
> > I don't understand the comment about feral nodes? 
> > es53-test-domado-dom-guest.html calls
> >   makeReadOnly(document.getElementById('testReadOnly'));
> > and since it's guest code, that's a tame document, so the arg received by
> > makeReadOnly will be a tame node, right?
> 
> The guest obtains frame.tame(makeReadOnly), so the taming membrane is in
effect
> here; the guest provides tame nodes and the feral side receives feral nodes.

oh yeah, right. ok.

still looking at the domado.js changes...

[felix8a, 2012-09-14 22:19:04]

ok, done looking at domado.js.  two minor nits, but also, I think I can get
access to children of an foreign node, by using getElementsByTagName or
getElementsByClassName on a parent of the foreign node.  this does not seem to
be a new vulnerability in this change; I think the current domado allows that
too.

am I correct? and do we care?

https://codereview.appspot.com/6490106/diff/3001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6490106/diff/3001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2706: if (parent) {
how about set parentPolicy=null above and rewrite this to a single if statement?

https://codereview.appspot.com/6490106/diff/3001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2707: if
(idClassPattern.test(parent.className) || idClassPattern.test(node.className)) {
long line

https://codereview.appspot.com/6490106/diff/3001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2781: // TODO(kpreid): There is no test
which caught a previous editability policy failure here
long line

[kpreid2, 2012-09-14 22:33:00]

On 2012/09/14 22:19:04, felix8a wrote:
> ok, done looking at domado.js.  two minor nits, but also, I think I can get
> access to children of an foreign node, by using getElementsByTagName or
> getElementsByClassName on a parent of the foreign node.  this does not seem to
> be a new vulnerability in this change; I think the current domado allows that
> too.

It is now impossible to defaultTameNode a child of a foreign node, because the
policy determination will walk up to the foreign node and then throw, so that is
not a vulnerability. However, this means the guest gets a node list with broken
contents, which is not good for them, and also that they can determine the
number of such children. I agree this is a bug, and I'll have to think about how
to fix it, since the existing code assumes that getting to a foreign node's
descendant is always a reason to explode.

Will reply to your inline comments separately.

[felix8a, 2012-09-14 22:43:20]

ok I'm fine with just adding a TODO for that
On Sep 14, 2012 3:33 PM, <kpreid.switchb.org@gmail.com> wrote:

> On 2012/09/14 22:19:04, felix8a wrote:
>
>> ok, done looking at domado.js.  two minor nits, but also, I think I
>>
> can get
>
>> access to children of an foreign node, by using getElementsByTagName
>>
> or
>
>> getElementsByClassName on a parent of the foreign node.  this does not
>>
> seem to
>
>> be a new vulnerability in this change; I think the current domado
>>
> allows that
>
>> too.
>>
>
> It is now impossible to defaultTameNode a child of a foreign node,
> because the policy determination will walk up to the foreign node and
> then throw, so that is not a vulnerability. However, this means the
> guest gets a node list with broken contents, which is not good for them,
> and also that they can determine the number of such children. I agree
> this is a bug, and I'll have to think about how to fix it, since the
> existing code assumes that getting to a foreign node's descendant is
> always a reason to explode.
>
> Will reply to your inline comments separately.
>
>
https://codereview.appspot.**com/6490106/<https://codereview.appspot.com/6490...
>

[felix8a, 2012-09-14 22:49:14]

no wait. tamebackednode doesn't walk up the tree. it only checks the
parent. so I can get a backed node that isn't tainted as foreign, right?
On Sep 14, 2012 3:43 PM, "felix" <felix8a@gmail.com> wrote:

> ok I'm fine with just adding a TODO for that
> On Sep 14, 2012 3:33 PM, <kpreid.switchb.org@gmail.com> wrote:
>
>> On 2012/09/14 22:19:04, felix8a wrote:
>>
>>> ok, done looking at domado.js.  two minor nits, but also, I think I
>>>
>> can get
>>
>>> access to children of an foreign node, by using getElementsByTagName
>>>
>> or
>>
>>> getElementsByClassName on a parent of the foreign node.  this does not
>>>
>> seem to
>>
>>> be a new vulnerability in this change; I think the current domado
>>>
>> allows that
>>
>>> too.
>>>
>>
>> It is now impossible to defaultTameNode a child of a foreign node,
>> because the policy determination will walk up to the foreign node and
>> then throw, so that is not a vulnerability. However, this means the
>> guest gets a node list with broken contents, which is not good for them,
>> and also that they can determine the number of such children. I agree
>> this is a bug, and I'll have to think about how to fix it, since the
>> existing code assumes that getting to a foreign node's descendant is
>> always a reason to explode.
>>
>> Will reply to your inline comments separately.
>>
>>
https://codereview.appspot.**com/6490106/<https://codereview.appspot.com/6490...
>>
>

[kpreid2, 2012-09-14 22:50:34]

(Responses now; snapshot coming when I've run a smoke test.)

https://codereview.appspot.com/6490106/diff/3001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6490106/diff/3001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2706: if (parent) {
On 2012/09/14 22:19:04, felix8a wrote:
> how about set parentPolicy=null above and rewrite this to a single if
statement?

Collapsed the if statements, but I want to emphasize that it's a "big ?:", so
the =null is still an else.

https://codereview.appspot.com/6490106/diff/3001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2707: if
(idClassPattern.test(parent.className) || idClassPattern.test(node.className)) {
On 2012/09/14 22:19:04, felix8a wrote:
> long line

Done.

https://codereview.appspot.com/6490106/diff/3001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2781: // TODO(kpreid): There is no test
which caught a previous editability policy failure here
On 2012/09/14 22:19:04, felix8a wrote:
> long line

Done.

[kpreid2, 2012-09-14 22:54:50]

On 2012/09/14 22:49:14, felix8a wrote:
> no wait. tamebackednode doesn't walk up the tree. it only checks the
> parent. so I can get a backed node that isn't tainted as foreign, 
> right?

TameBackedNode asks the tame parent for its policy. In order to bring that tame
parent into existence, its policy must be calculated from its parent, et cetera.
This is the “implicit recursion” referred to in the policy algorithm's comments.

[felix8a, 2012-09-14 23:22:31]

On 2012/09/14 22:54:50, kpreid2 wrote:
> On 2012/09/14 22:49:14, felix8a wrote:
> > no wait. tamebackednode doesn't walk up the tree. it only checks the
> > parent. so I can get a backed node that isn't tainted as foreign, 
> > right?
> 
> TameBackedNode asks the tame parent for its policy. In order to bring that
tame
> parent into existence, its policy must be calculated from its parent, et
cetera.
> This is the “implicit recursion” referred to in the policy algorithm's
comments.

ah right, ok.

lgtm

[kpreid2, 2012-10-03 15:48:33]

Patch Set 5/6 adds node-list filtering so that getElementsByTagName and friends
do not expose foreign nodes' descendants to the guest. Only affects Domado and
es53-test-domado-foreign.

The hairiest part of this change is that we now have to note whenever the vdoc
is mutated in a way which could change the contents of a NodeList. I included
all attribute mutations, even though only id= and name= are currently relevant,
because they will become relevant if we start supporting .querySelector().

Please review.

[felix8a, 2012-10-03 20:49:30]

the nodelist filtering feels to me kind of complex and fragile.  what's the harm
of letting guest code see foreign nodes in a nodelist?

[kpreid2, 2012-10-03 20:57:43]

On 2012/10/03 20:49:30, felix8a wrote:
> the nodelist filtering feels to me kind of complex and fragile.  what's the
harm
> of letting guest code see foreign nodes in a nodelist?

0. The problem is not foreign nodes — guests can see them — but foreign nodes'
*descendants*.

1. Foreign nodes' descendants are an implementation detail; allowing guests to
see their existence is an information leak. Currently a small one (count number
of nodes with a given tag name/class name), but if we ever implement
querySelector — and I expect we will eventually want to do so for the sake of
large-document performance — guests will be able to extract attribute values by
searching for prefixes.

2. Guest code may not have considered that some className criterion matches
nodes other than its own; if we expose the foreign-descendants as foreign nodes,
then stuff blows up because foreign nodes have no element-specific API, so the
alternative is to provide a union of all element interfaces which does nothing
when mutated.

[felix8a, 2012-10-03 21:20:22]

hm.  one thing that concerns me is that once someone starts admitting foreign
nodes, it's quite likely that the foreign nodes will get modified asynchronously
by caja-oblivious code, like a foreign div might be a placeholder until more
content is loaded with xhr.

another thing that concerns me is that it's hard to verify that
vdocMutatedAffectingNodeLists() is called everywhere it should be.

how about instead of vdocMutationCounter, have nodeListFiltered keep a static
snapshot of the nodes in the nodelist up to the index, and verify that the
nodelist still matches that snapshot before using the cache.  O(n) per access
instead of O(1), but more robust.

[kpreid2, 2012-10-03 21:29:56]

On 2012/10/03 21:20:22, felix8a wrote:
> hm.  one thing that concerns me is that once someone starts admitting foreign
> nodes, it's quite likely that the foreign nodes will get modified
asynchronously
> by caja-oblivious code, like a foreign div might be a placeholder until more
> content is loaded with xhr.

That is harmless, since foreign descendants don't appear in our cached
guest-visible list.
...wait, it isn't, since the foreign change invalidates lastHostIndex. Oops.
That could be fixed by validating that hostNodeList[lastHostIndex-1] is the same
as it used to be, or by always building the entire cache at once.

> another thing that concerns me is that it's hard to verify that
> vdocMutatedAffectingNodeLists() is called everywhere it should be.

That is a problem, yes. Not as hard as you might think: it only possibly needs
to be called in places that also need an editability check, so you can simply
review instances of editability checks.

> how about instead of vdocMutationCounter, have nodeListFiltered keep a static
> snapshot of the nodes in the nodelist up to the index, and verify that the
> nodelist still matches that snapshot before using the cache.  O(n) per access
> instead of O(1), but more robust.

I mostly like this idea. Will do.

[kpreid2, 2012-10-03 23:41:30]

On 2012/10/03 21:29:56, kpreid2 wrote:
> On 2012/10/03 21:20:22, felix8a wrote:
> > how about instead of vdocMutationCounter, have nodeListFiltered keep a
static
> > snapshot of the nodes in the nodelist up to the index, and verify that the
> > nodelist still matches that snapshot before using the cache.  O(n) per
access
> > instead of O(1), but more robust.
> 
> I mostly like this idea. Will do.

Done (Patch Set 8).

[kpreid2, 2012-10-05 21:41:25]

Patch Set 9 adds a bit I overlooked, namely preventing getAttribute[Node] from
exposing attributes when the policy prohibits them.

[felix8a, 2012-10-09 00:40:08]

https://codereview.appspot.com/6490106/diff/24001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6490106/diff/24001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:2337: // mutation generation counter) at
which time the cache is invalid. This

comment is out of date

https://codereview.appspot.com/6490106/diff/24001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:2356: feralIndex < expectation.length &&
feralIndex < feralLength;
I think you need to invalidate filteredCache if feralLength decreases. 
consider, I have a nodeList with 10 elements, I fetch the 10th element so it all
ends up in filteredCache, but then I modify the document so that the nodeList
only has the first element.  the cache validation succeeds, the cache doesn't
need to be extended, and then we still return filteredCache.length == 10 even
though the nodeList length == 1.

https://codereview.appspot.com/6490106/diff/24001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:2359: expectation = [];
this is perhaps over-conservative, since we know that we're still in sync up to
the first feralIndex, but it's probably fine.  (alternative would require
keeping track of filtered nodes somehow so we know how much of filteredCache to
keep.)

https://codereview.appspot.com/6490106/diff/24001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/es53-test-domado-foreign.js (right):

https://codereview.appspot.com/6490106/diff/24001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/es53-test-domado-foreign.js:26:
d.setAttribute('data-caja-testattr', 'testattrvalue');
it doesn't matter here, but test values should generally be unique, so you don't
accidentally get false positives

[kpreid2, 2012-10-09 19:53:26]

https://codereview.appspot.com/6490106/diff/24001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6490106/diff/24001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:2356: feralIndex < expectation.length &&
feralIndex < feralLength;
On 2012/10/09 00:40:08, felix8a wrote:
> I think you need to invalidate filteredCache if feralLength decreases. 

Done.

https://codereview.appspot.com/6490106/diff/24001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/es53-test-domado-foreign.js (right):

https://codereview.appspot.com/6490106/diff/24001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/es53-test-domado-foreign.js:26:
d.setAttribute('data-caja-testattr', 'testattrvalue');
On 2012/10/09 00:40:08, felix8a wrote:
> it doesn't matter here, but test values should generally be unique, so you
> don't accidentally get false positives

Done.

[felix8a, 2012-10-09 23:31:27]

lgtm

[kpreid2, 2012-10-12 22:58:55]

Updated, only trivial conflict resolution, passes tests.