Bug 53611 – Propagate security origin of parent document into HTML documents created with DOMImplementation

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[anton muhin]

Propagate security origin of parent document into HTML documents created with DOMImplementation

[anton muhin]

Created attachment 80945 [details]
Patch

[anton muhin]

Created attachment 80947 [details]
Patch

[Adam Barth]

Comment on attachment 80947 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80947&action=review

> Source/WebCore/dom/DOMImplementation.h:71
> -    DOMImplementation() { }
> +    DOMImplementation(const Document* parent) : m_parent(parent) { }

I'd move this into the CPP file and ASSERT(m_parent).

[anton muhin]

Created attachment 81047 [details]
Patch

[anton muhin]

(In reply to comment #3)
> (From update of attachment 80947 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80947&action=review
> 
> > Source/WebCore/dom/DOMImplementation.h:71
> > -    DOMImplementation() { }
> > +    DOMImplementation(const Document* parent) : m_parent(parent) { }
> 
> I'd move this into the CPP file and ASSERT(m_parent).

Done.  I've uploaded with r? to run though bots once again.  Once it green, I'll ask for r+ from someone around to land it actually.  Thanks a lot for review, Adam.

[anton muhin]

Comment on attachment 81047 [details]
Patch

Thanks a lot, Yury!

[Alexey Proskuryakov]

Comment on attachment 81047 [details]
Patch

Do we ever use "const Document*" (other than in misguided code)? I may be just confused, but I think that the design is such that large classes like Docuemnt or Frame are never const in WebCore.

[anton muhin]

Alexey,

WebCore::Document::implementation() is const method hence I cannot obtain not const Document unless I const cast it away or change constness of the method.   As Document::securityOrigin is const as well, the simplest solution was to store a pointer to a const Document.

Does that sound reasonable?

yours,
anton.

(In reply to comment #7)
> (From update of attachment 81047 [details])
> Do we ever use "const Document*" (other than in misguided code)? I may be just confused, but I think that the design is such that large classes like Docuemnt or Frame are never const in WebCore.

[Alexey Proskuryakov]

I think that no methods on Document are supposed to be const. Darin, is that accurate?

[anton muhin]

I am sorry, but that's apparently not directly related to the patch under consideration.  Maybe I should be allowed to proceed with it and if conclusion would be constness should be removed, my poor const Document will go away with this change as well?

(In reply to comment #9)
> I think that no methods on Document are supposed to be const. Darin, is that accurate?

[Alexey Proskuryakov]

Comment on attachment 81047 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81047&action=review

OK, I took a deeper look now, and I'm puzzled why this has r+ at all. I would encourage reviewers to pay more attention to what they approve.

> Source/WebCore/ChangeLog:10
> +        Absence of regressions is covered by the current tests.  It's difficult
> +        to check this via layout tests as objects reside in the same JS context and hence
> +        there are no security origin checks.

If this is not observable, why do this at all? And if it's just difficult to test, it still needs a test.

Note that we already have code to deal with security context of documents created via DOMImplementation in Document::initSecurityContext(). If it's wrong, or the comment there is wrong, that should be addressed.

Finally, this change directly contradicts DOM 3 Core <http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-102161490>, which says "The DOMImplementation interface provides a number of methods for performing operations that are independent of any particular instance of the document object model." There should be an explanation of why that's OK.

> Source/WebCore/dom/DOMImplementation.cpp:174
> +DOMImplementation::DOMImplementation(const Document* parent) : m_parent(parent)

Initialization should be on a separate line.

> Source/WebCore/dom/DOMImplementation.cpp:251
> +    if (!m_parent) {
> +        ec = INVALID_STATE_ERR;
> +        return 0;
> +    }

Is this how it works in other browsers? This is definitely testable, and there should be a regression test.

Garbage collection should never be observable in JavaScript, so instead of checking for document having been destroyed and raising an exception, you should make sure that it is not destroyed while observable through DOMImplementation. For JSC, this is done by overriding markChildren method (see e.g. JSDocument::markChildren() in JSDocumentCustom.cpp).

> Source/WebCore/dom/DOMImplementation.h:68
> +    void documentDestroyed()
> +    {
> +        m_parent = 0;
> +    }

This should be on one line.

> Source/WebCore/dom/DOMImplementation.h:73
> +    const Document* m_parent;

The document is not a parent of DOMImplementation, so the name is misleading.

[Darin Adler]

(In reply to comment #9)
> I think that no methods on Document are supposed to be const. Darin, is that accurate?

Yes. It’s important to remove const from a function rather than starting to use const Document* in new code.

[Adam Barth]

> If this is not observable, why do this at all? And if it's just difficult to test, it still needs a test.

This is one patch in a series of patches, which unfortunately does not have a meta bug to explain the context.  The main idea is that we are adding some defense-in-depth security checks to V8 as described in <http://www.adambarth.com/papers/2009/barth-weinberger-song.pdf>.  These checks are not directly observable because they are defense-in-depth.

Before we turn the checks on, we need to associate the correct security context with a number of objects.  There are a number of objects in WebCore that have incorrect security contexts, but that can't be observed as such because the security check that would block access isn't actually materialized (see the paper above for an explanation of why that's the case). 

Documents created via DOMImplementation are an example of one of these objects.

> Note that we already have code to deal with security context of documents created via DOMImplementation in Document::initSecurityContext(). If it's wrong, or the comment there is wrong, that should be addressed.

This code assumes that documents that don't have a frame belong to the null security context.  That's not really accurate, but it's a fine default.

> Finally, this change directly contradicts DOM 3 Core <http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-102161490>, which says "The DOMImplementation interface provides a number of methods for performing operations that are independent of any particular instance of the document object model." There should be an explanation of why that's OK.

I'm not sure how that's related to the discussion here.  That requirement is on the operation of the interface.  This patch is related to some internal bookkeeping about who should have access to which objects.

Relatedly, there are several design pressures pushing us to associate a parent or owner document with documents created by DOMImplementation.  We've seen it proposed in a number of different patches, including the patches related to document.load and loading subresources from XSLT.

> > Source/WebCore/dom/DOMImplementation.h:73
> > +    const Document* m_parent;
> 
> The document is not a parent of DOMImplementation, so the name is misleading.

Perhaps m_ownerDocument would be more accurate?

Thanks for your comments.  My apologies that the full context for these patches hasn't been included in each one.  I should have asked antonm to create a meta bug with that information.

[Alexey Proskuryakov]

> I'm not sure how that's related to the discussion here.

Well, I still don't have context, but to me, this phrase from the spec bluntly says "there should be no m_document pointer in your WebCore::DOMImplementation class".

[Adam Barth]

(In reply to comment #14)
> > I'm not sure how that's related to the discussion here.
> 
> Well, I still don't have context, but to me, this phrase from the spec bluntly says "there should be no m_document pointer in your WebCore::DOMImplementation class".

The documents created via DOMImplementation are implicitly related to other documents already.  For example, if you follow the JavaScript prototype chain backwards from window.document and from a document created via DOMImplementation, you'll eventually arrive at an object that they share.  Similarly, following the prototype chain backwards from document.implementation backwards will also lead to an object in the prototype chain of window.document.

From that perspective, it's unclear to me whether the author of that sentence understood its implications in the way you describe.  In any case, specifications are only about observable behavior, not about implementation.  Is there some experiment we should run to observe us violation this requirement of the specification?

[Alexey Proskuryakov]

> For example, if you follow the JavaScript prototype chain backwards from window.document 
> and from a document created via DOMImplementation, you'll eventually arrive at an object
> that they share.

Should that be considered a bug? Can it possibly be fixed?

> Is there some experiment we should run to observe us violation this requirement of the specification?

With the attached patch, yes. DOMImplementation fails to create a document after its owner document is deleted.

It's of course difficult to propose an experiment to detect a design mistake in something that's not supposed to ever be observable! But it is still feels very strange that we need to add new dependencies between objects just for a non-observable feature.

[Adam Barth]

(In reply to comment #16)
> > For example, if you follow the JavaScript prototype chain backwards from window.document 
> > and from a document created via DOMImplementation, you'll eventually arrive at an object
> > that they share.
> 
> Should that be considered a bug? Can it possibly be fixed?

It's not a bug and it cannot be fixed.  That's the way JavaScript works.

> > Is there some experiment we should run to observe us violation this requirement of the specification?
> 
> With the attached patch, yes. DOMImplementation fails to create a document after its owner document is deleted.

That's probably worth writing a test for.  However, most DOM methods fail to anything useful after their owner document is deleted.

> It's of course difficult to propose an experiment to detect a design mistake in something that's not supposed to ever be observable! But it is still feels very strange that we need to add new dependencies between objects just for a non-observable feature.

I don't agree that it's a design mistake.  Somehow we need to assign a security context to the newly created document.  Do you have another proposal for how to do that?

Currently, our design is to not assign it a security context and hope that its pointer doesn't leak into the wrong hands.  Historically, we've had vulnerabilities precisely because it has leaked.  We'd like to harden ourselves against this class of vulnerabilities.

[Alexey Proskuryakov]

> > > For example, if you follow the JavaScript prototype chain backwards from window.document 
> > > and from a document created via DOMImplementation, you'll eventually arrive at an object
> > > that they share.

Could you please attach a test for that? I can't get that work in Safari 5.0.3, no matter how many ".__proto__" I append.

[anton muhin]

Alexey, thank you very much for your comments.

Let me try to clarify several things.

As far as I understand currently there is the following invariant: if two JS objects belong to the same JS context (roughly, they share the same Object.prototype), they must belong to the same security domain, otherwise we have the security issue.

However, v8 for sure and as far as I can judge from the source JSC bindings both take the following shortcut: if objects are in the same context, we do not check their security origins.

Therefore, current WebKit implementation breaks the important invariant above: document and documents created via DOMImplementation live in one context, but their security origins are different.  It's impossible to prove in pure JS due to shortcut above, but quite easy in C++---just add an ASSERT.

(BTW, it looks like you'd like to see if it's indeed the case the document and documents created via DOMImplementation reside in the same context.  That's easy to prove.  Open any page in Safari and then in WebInspector do something like:

d = document.implementation.createHTMLDocument('')
Document
Object.prototype.isPrototypeOf(d)
true
Object.prototype.isPrototypeOf(document)
true

As can be seen both d and document share the same Object.prototype.

That's exactly the issue I'd like to address.  BTW, FF4 and Opera 11 both do it right, try "document.implementation.createHTMLDocument().domain" in those browsers and in Safari and Chromium.

Now to the solution.  I decided to add a pointer to parent document to the DOMImplementation.  You cite: "The DOMImplementation interface provides a number of methods for performing operations that are independent of any particular instance of the document object model.".  Note though, that it talks of instance of DOM (standard assumes that there might be different DOM implementations even residing in one browser that's why it's not guaranteed that you can move nodes around).  Please, note as well http://www.w3.org/TR/html5/dom.html#the-document-s-address : "When a Document is created by a script using the createDocument() or createHTMLDocument() APIs, the document's address is the same as the document's address of the script's document." which at the very least (imho) makes DOMImplementation dependent on script to document relationship.  Probably, we need clarifications from other people involved in HTML5 spec.

Now to ref counting.  Then I added an exception, I thought not about JS bindings, but about general situation: what if I keep a live reference to DOMImplementation with RefPtr.  Throwing an exception appeared for me to be the best way out.

But I am definitely guilty of forgetting to introduce dependency from DOMimplementation to the document in JS which I introduced in C++.  Thanks a lot for spotting this.

[Alexey Proskuryakov]

Thanks Anton, that's perfectly convincing.

The reason I was confused is that we get different prototype chains for normal documents and ones created with DOMImplementation.createDocument:

var doc = document.implementation.createDocument(null, "doc", null);
alert(document.__proto__.__proto__.__proto__.__proto__ === doc.__proto__.__proto__.__proto__.__proto__)
// prints false

So, this patch just needs:
-  JSDOMImplementation mark JSDocument,
- a test for this behavior,
- no more const Document*.

[anton muhin]

Created attachment 81664 [details]
Patch

[anton muhin]

I've uploaded new version of the patch which (I hope) should address all concerns.

I don't believe 'parent' will survive :) and will be happy to change to any better term.  Adam once suggested 'owner'.

(In reply to comment #20)
> Thanks Anton, that's perfectly convincing.
> 
> The reason I was confused is that we get different prototype chains for normal documents and ones created with DOMImplementation.createDocument:
> 
> var doc = document.implementation.createDocument(null, "doc", null);
> alert(document.__proto__.__proto__.__proto__.__proto__ === doc.__proto__.__proto__.__proto__.__proto__)
> // prints false
> 
> So, this patch just needs:
> -  JSDOMImplementation mark JSDocument,
> - a test for this behavior,
> - no more const Document*.

[anton muhin]

Sorry, forgot to add JSDOMImplementationCustom.cpp, uploading in no time.

[anton muhin]

Created attachment 81667 [details]
Patch

[anton muhin]

One thing I need to mention: alas, commenting out body of JSDOMImplementation::markChildren doesn't make added test fail.  I'll try to investigate it more.

(In reply to comment #24)
> Created an attachment (id=81667) [details]
> Patch

[Build Bot]

Attachment 81667 [details] did not build on win:
Build output: http://queues.webkit.org/results/7801160

[Alexey Proskuryakov]

I suggest ownerDocument()/m_ownerDocument.

Is gc-9.html the best place for this, or should a new test be added? You can try adding a timeout to make it more likely that the document pointer gets off stack and collected.

[Early Warning System Bot]

Attachment 81667 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7847151

[anton muhin]

Created attachment 82152 [details]
Patch

[anton muhin]

(In reply to comment #27)
> I suggest ownerDocument()/m_ownerDocument.

Done.

> Is gc-9.html the best place for this, or should a new test be added? You can try adding a timeout to 
make it more likely that the document pointer gets off stack and collected.

The test didn't work because I forgot to add it into main test runner, sorry, now everything is fine.

Regarding gc-9 vs. separate layout test.  I personally prefer to keep number of tests to the minimum and keep related things together.  But you should be ways more experienced in true WebKit ways, so if you'd prefer to have a separate test, so it'll be.

Sorry for delay, has been preempted.

[Alexey Proskuryakov]

Comment on attachment 82152 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82152&action=review

> if you'd prefer to have a separate test, so it'll be.

It's good to have tests in one file when they are clearly independent (output is clearly separated, there is no conceivable way the tests could affect each other by being on one page, and you can comment out all subtests but one for easier debugging). When that's not true, separate tests make it easier to figure out what happened when they fail.

r=me with comments addressed.

> LayoutTests/fast/dom/gc-9.html:90
> +  impl.createHTMLDocument('');  // May crash or throw an excepiton if we collect parent document of impl.

Typo: exception.

> Source/WebCore/bindings/js/JSDOMImplementationCustom.cpp:5
> + * modify it under the terms of the GNU Library General Public

Please consider using a BSD license for new code.

> Source/WebCore/bindings/js/JSDOMImplementationCustom.cpp:34
> +    Document* doc = domImplementation->ownerDocument();

Please don't abbreviate "ownerDocument".

> Source/WebCore/bindings/js/JSDOMImplementationCustom.cpp:39
> +} // namespace WebCore

I'm not a fan of these comments - they don't guarantee that they are true if you're wondering about that (you'll use your editor to find matching brace if you really need to check what's wrong with nesting), but most of the time, they just add visual noise.

> Source/WebCore/dom/DOMImplementation.h:48
> +    PassRefPtr<DocumentType> createDocumentType(const String& qualifiedName, const String& publicId, const String &systemId, ExceptionCode&);

As you're touching this code already, please move "&" to a correct position.

> Source/WebCore/dom/DOMImplementation.h:72
> +    template <class T>
> +    PassRefPtr<T> setSecurityOrigin(PassRefPtr<T> doc);

I could suggest not using a template, and spelling out "document". But it would probably be best to just inline this function in the only place it's used.

[anton muhin]

Alexey, thanks a lot for all your suggestions.  I'll address all comments next week, but right now I'd ask you just one question: may you provide me a link to the BSD license?  I copied the license from JSDOMWindowCustom.cpp.

ANd another minor question: I just kept this "// namespace WebCore" from JSDOMWindowCustom.cpp which I used as a template.  If modern trend is to remove them, I'd be happy as I find them distracting too.  Just double checking.

Thanks again for review.

And I'll upload another r? patch to run through various builder we have.

(In reply to comment #31)
> (From update of attachment 82152 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82152&action=review
> 
> > if you'd prefer to have a separate test, so it'll be.
> 
> It's good to have tests in one file when they are clearly independent (output is clearly separated, there is no conceivable way the tests could affect each other by being on one page, and you can comment out all subtests but one for easier debugging). When that's not true, separate tests make it easier to figure out what happened when they fail.
> 
> r=me with comments addressed.
> 
> > LayoutTests/fast/dom/gc-9.html:90
> > +  impl.createHTMLDocument('');  // May crash or throw an excepiton if we collect parent document of impl.
> 
> Typo: exception.
> 
> > Source/WebCore/bindings/js/JSDOMImplementationCustom.cpp:5
> > + * modify it under the terms of the GNU Library General Public
> 
> Please consider using a BSD license for new code.
> 
> > Source/WebCore/bindings/js/JSDOMImplementationCustom.cpp:34
> > +    Document* doc = domImplementation->ownerDocument();
> 
> Please don't abbreviate "ownerDocument".
> 
> > Source/WebCore/bindings/js/JSDOMImplementationCustom.cpp:39
> > +} // namespace WebCore
> 
> I'm not a fan of these comments - they don't guarantee that they are true if you're wondering about that (you'll use your editor to find matching brace if you really need to check what's wrong with nesting), but most of the time, they just add visual noise.
> 
> > Source/WebCore/dom/DOMImplementation.h:48
> > +    PassRefPtr<DocumentType> createDocumentType(const String& qualifiedName, const String& publicId, const String &systemId, ExceptionCode&);
> 
> As you're touching this code already, please move "&" to a correct position.
> 
> > Source/WebCore/dom/DOMImplementation.h:72
> > +    template <class T>
> > +    PassRefPtr<T> setSecurityOrigin(PassRefPtr<T> doc);
> 
> I could suggest not using a template, and spelling out "document". But it would probably be best to just inline this function in the only place it's used.

[Alexey Proskuryakov]

(In reply to comment #32)
> may you provide me a link to the BSD license?  I copied the license from JSDOMWindowCustom.cpp.

We have <http://webkit.org/coding/bsd-license.html> on site - Google employees probably need to use a version with "Google, Inc." in it though.

> ANd another minor question: I just kept this "// namespace WebCore" from JSDOMWindowCustom.cpp which I used as a template.  If modern trend is to remove them, I'd be happy as I find them distracting too.  Just double checking.

We don't have a style guideline on this, but there was a moderate level of support for not having comments on closing whole file namespace blocks when discussed on webkit-dev. I sometimes comment on this when there aren't bigger issues to comment on.

[anton muhin]

Created attachment 82306 [details]
Patch

[Early Warning System Bot]

Attachment 82306 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7913505

[anton muhin]

Created attachment 82308 [details]
Patch

[anton muhin]

Thanks a lot for pointers and further explanations.  Uploading new patch with the license and removed comment.

(In reply to comment #33)
> (In reply to comment #32)
> > may you provide me a link to the BSD license?  I copied the license from JSDOMWindowCustom.cpp.
> 
> We have <http://webkit.org/coding/bsd-license.html> on site - Google employees probably need to use a version with "Google, Inc." in it though.
> 
> > ANd another minor question: I just kept this "// namespace WebCore" from JSDOMWindowCustom.cpp which I used as a template.  If modern trend is to remove them, I'd be happy as I find them distracting too.  Just double checking.
> 
> We don't have a style guideline on this, but there was a moderate level of support for not having comments on closing whole file namespace blocks when discussed on webkit-dev. I sometimes comment on this when there aren't bigger issues to comment on.

[Build Bot]

Attachment 82306 [details] did not build on win:
Build output: http://queues.webkit.org/results/7913513

[Early Warning System Bot]

Attachment 82308 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7913516

[Build Bot]

Attachment 82308 [details] did not build on win:
Build output: http://queues.webkit.org/results/7912506

[anton muhin]

Created attachment 82315 [details]
Patch

[Early Warning System Bot]

Attachment 82315 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7911575

[Build Bot]

Attachment 82315 [details] did not build on win:
Build output: http://queues.webkit.org/results/7907634

[anton muhin]

Created attachment 82341 [details]
Patch

[Early Warning System Bot]

Attachment 82341 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7910688

[Build Bot]

Attachment 82341 [details] did not build on win:
Build output: http://queues.webkit.org/results/7912589

[anton muhin]

Created attachment 82451 [details]
Patch

[Early Warning System Bot]

Attachment 82451 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7911873

[anton muhin]

Created attachment 82646 [details]
Patch

[anton muhin]

Created attachment 82817 [details]
Patch

[anton muhin]

Committed r79223: <http://trac.webkit.org/changeset/79223>

[Adam Barth]

Comment on attachment 82817 [details]
Patch

Clearing review flag on landed patch.