Bug 55965 – Allow shadowing of history object

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Robert Hogan]

Allow shadowing of history object

[Robert Hogan]

Created attachment 85088 [details]
Patch

[Robert Hogan]

This is from the same motivation as https://bugs.webkit.org/show_bug.cgi?id=41802.

See also https://trac.webkit.org/wiki/Fingerprinting#ii.Window.HistoryObject

[Adam Barth]

This seems fine.  My only concern is to make sure Replaceable and DoNotCheckDomainSecurity play nicely together.  Can you write a test that shows that when a cross-origin iframe replaces its history object, that the parent page can't see the new object?

[Alexey Proskuryakov]

Note that site compatibility concerns in bug 41802 haven't been addressed (other than by deciding to wait and see if bug reports come in). So, this patch shares the same concerns.

[Adam Barth]

I presume you mean https://bugs.webkit.org/show_bug.cgi?id=41802#c13 ?  Perhaps we should dig into the history of why our behavior is the way it is and what the history is of other implementations.  Generally, I wouldn't expect this change to cause many compat problems because we're making our behavior more flexible.

[Darin Adler]

(In reply to comment #5)
> Generally, I wouldn't expect this change to cause many compat problems because we're making our behavior more flexible.

One kind of problem we’ve seen in the past in cases like this is websites that accidentally attempts to replace a built-in property; harmless if not allowed, but can interfere with the operation of the rest of the code on the site if allowed.

[Robert Hogan]

Taking a step back, maybe the best way of permitting client manipulation/overloading of this and the other javascript items listed in https://trac.webkit.org/wiki/Fingerprinting (particularly those that belong to objects that will never be replaceable such as window and document) is to follow the method used for Navigator::userAgent() - which allows FrameLoaderClient to decide.

[Adam Barth]

That can also work.  It's mostly a matter of who's going to opt into this mechanism.  Making the option available to FrameLoaderClient means someone with access to the WebKit API will need to make that choice.  Making the JS object replaceable means that someone with access to the web-facing API will need to make that choice.

I don't have the bigger picture, so it's hard for me to give you advice as to what works better for your goals.

[Adam Barth]

Comment on attachment 85088 [details]
Patch

R- for lack of a security test, but please consider the discussion above when revising your patch.

[Robert Hogan]

Created attachment 88300 [details]
Patch

[Robert Hogan]

(In reply to comment #9)
> (From update of attachment 85088 [details])
> R- for lack of a security test, but please consider the discussion above when revising your patch.

Allowing the history object to be replaceable is still useful for my own use case. I've updated the patch and added a test to ensure a cross-origin iframe cannot affect the parent's history object. Is this what you were getting at in #3, or is there a reason a parent should not be able to see a child's history object once it has been replaced?

[Robert Hogan]

(In reply to comment #8)
> That can also work.  It's mostly a matter of who's going to opt into this mechanism.  Making the option available to FrameLoaderClient means someone with access to the WebKit API will need to make that choice.  Making the JS object replaceable means that someone with access to the web-facing API will need to make that choice.
> 
> I don't have the bigger picture, so it's hard for me to give you advice as to what works better for your goals.

My own use case is for a client-facing API, I've outlined what I'm trying to do at: https://lists.webkit.org/pipermail/webkit-qt/2011-March/001358.html
and listed the relevant bugs there:

https://bugs.webkit.org/show_bug.cgi?id=56274 (DOM)
https://bugs.webkit.org/show_bug.cgi?id=56482 (CSS)

I'm planning to post that mail to webkit-dev for feedback too, once I've received some from the QtWebKit team.

[Adam Barth]

Comment on attachment 88300 [details]
Patch

Can you also test the case where the child replaces the history object with another object and see if that change is visible to a cross-domain parent frame?  If it is, we're creating a security hole.

[Robert Hogan]

Created attachment 89625 [details]
Patch

[Adam Barth]

Comment on attachment 89625 [details]
Patch

Thanks for adding the test.  This looks good, but I need to look at this when I more awake.  Please feel free to harass me on #webkit for a review.  There's still the compat question, but I don't really know how we can address that other than to try.

[Adam Barth]

Comment on attachment 89625 [details]
Patch

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

I'm still slightly worried about this patch, but I can't think of another test to run.  Please be on the lookout for compatibility regressions!

> LayoutTests/http/tests/history/cross-origin-replace-history-object-child-expected.txt:6
> +CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://localhost:8000/history/resources/cross-origin-replaces-history-object-child-iframe.html from frame with URL http://127.0.0.1:8000/history/cross-origin-replace-history-object-child.html. Domains, protocols and ports must match.

Where does this message come from?

[Robert Hogan]

Comment on attachment 89625 [details]
Patch

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

> LayoutTests/http/tests/history/cross-origin-replace-history-object-child.html:18
> +    alert("Child window's history object after attempt to clear: " + window.frames[0].history);

It comes from here. It gets the same message on the first access with:
alert("Child window's history object before attempt to clear: " + window.frames[0].history);

Look Ok to you?

[Adam Barth]

(In reply to comment #17)
> (From update of attachment 89625 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89625&action=review
> 
> > LayoutTests/http/tests/history/cross-origin-replace-history-object-child.html:18
> > +    alert("Child window's history object after attempt to clear: " + window.frames[0].history);
> 
> It comes from here. It gets the same message on the first access with:
> alert("Child window's history object before attempt to clear: " + window.frames[0].history);
> 
> Look Ok to you?

It's strange that we get the message even though access is allowed.  Is that behavior the same before / after your patch?

[Robert Hogan]

(In reply to comment #18)
> 
> It's strange that we get the message even though access is allowed.  Is that behavior the same before / after your patch?

It is yes, here is the result without:


CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://localhost:8000/history/resources/cross-origin-replaces-history-object-child-iframe.html from frame with URL http://127.0.0.1:8000/history/cross-origin-replace-history-object-child.html. Domains, protocols and ports must match.

ALERT: Child window's history object before attempt to clear: [object History]
ALERT: About to shadow child window's history object: [object History]
ALERT: Shadowed child window's history object: [object History]
CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://localhost:8000/history/resources/cross-origin-replaces-history-object-child-iframe.html from frame with URL http://127.0.0.1:8000/history/cross-origin-replace-history-object-child.html. Domains, protocols and ports must match.

ALERT: Child window's history object after attempt to clear: [object History]

I'll commit this weekend when the bots are quiet.

[WebKit Commit Bot]

Comment on attachment 89625 [details]
Patch

Rejecting attachment 89625 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2

Last 500 characters of output:
sts/xmlhttprequest ................................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers ...........
http/tests/xmlviewer .
http/tests/xmlviewer/dumpAsText ............
725.45s total testing time

23387 test cases (99%) succeeded
2 test cases (<1%) had incorrect layout
14 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8464006

[WebKit Commit Bot]

Created attachment 91101 [details]
Archive of layout-test-results from cr-jail-3

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-3  Port: Mac  Platform: Mac OS X 10.6.6

[Robert Hogan]

Created attachment 91276 [details]
Patch

[Robert Hogan]

I revised the patch to pass a test I missed (fast/dom/Window/get-set-properties.html) and update the expected results for another test (http/tests/security/cross-frame-access-put-expected.txt) to take account of the fact that a window property with the DoNotCheckDomainSecurityOnGet attribute (which the history object now has) generates two cross-origin JS warnings rather than just one. I observed this same behaviour in the test with window.self, which also has DoNotCheckDomainSecurityOnGet - though I did not look too deeply into why the extra warning happens.

[WebKit Commit Bot]

Comment on attachment 91276 [details]
Patch

Clearing flags on attachment: 91276

Committed r85142: <http://trac.webkit.org/changeset/85142>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.