Bug 55965

Summary: Allow shadowing of history object
Product: WebKit Reporter: Robert Hogan <robert>
Component: New BugsAssignee: Robert Hogan <robert>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarcelo, commit-queue, darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 41801, 59673    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from cr-jail-3
none
Patch none

Description Robert Hogan 2011-03-08 14:08:53 PST
Allow shadowing of history object
Comment 1 Robert Hogan 2011-03-08 14:10:44 PST
Created attachment 85088 [details]
Patch
Comment 2 Robert Hogan 2011-03-08 14:11:57 PST
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
Comment 3 Adam Barth 2011-03-08 16:48:57 PST
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?
Comment 4 Alexey Proskuryakov 2011-03-09 11:02:06 PST
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.
Comment 5 Adam Barth 2011-03-09 11:11:36 PST
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.
Comment 6 Darin Adler 2011-03-09 11:15:54 PST
(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.
Comment 7 Robert Hogan 2011-03-10 14:23:52 PST
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.
Comment 8 Adam Barth 2011-03-10 14:30:48 PST
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.
Comment 9 Adam Barth 2011-03-28 16:31:36 PDT
Comment on attachment 85088 [details]
Patch

R- for lack of a security test, but please consider the discussion above when revising your patch.
Comment 10 Robert Hogan 2011-04-05 13:19:13 PDT
Created attachment 88300 [details]
Patch
Comment 11 Robert Hogan 2011-04-05 13:29:29 PDT
(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?
Comment 12 Robert Hogan 2011-04-05 13:31:41 PDT
(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.
Comment 13 Adam Barth 2011-04-13 13:58:26 PDT
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.
Comment 14 Robert Hogan 2011-04-14 12:27:58 PDT
Created attachment 89625 [details]
Patch
Comment 15 Adam Barth 2011-04-17 01:45:20 PDT
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.
Comment 16 Adam Barth 2011-04-17 10:10:28 PDT
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?
Comment 17 Robert Hogan 2011-04-18 10:42:06 PDT
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?
Comment 18 Adam Barth 2011-04-18 11:44:43 PDT
(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?
Comment 19 Robert Hogan 2011-04-19 11:10:01 PDT
(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.
Comment 20 WebKit Commit Bot 2011-04-26 07:26:43 PDT
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
Comment 21 WebKit Commit Bot 2011-04-26 07:26:48 PDT
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
Comment 22 Robert Hogan 2011-04-27 06:40:38 PDT
Created attachment 91276 [details]
Patch
Comment 23 Robert Hogan 2011-04-27 11:22:22 PDT
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.
Comment 24 WebKit Commit Bot 2011-04-27 20:18:29 PDT
Comment on attachment 91276 [details]
Patch

Clearing flags on attachment: 91276

Committed r85142: <http://trac.webkit.org/changeset/85142>
Comment 25 WebKit Commit Bot 2011-04-27 20:18:36 PDT
All reviewed patches have been landed.  Closing bug.