New issue
Advanced search Search tips
Starred by 2 users
Status: Fixed
Owner:
Closed: May 15
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux, Windows, Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment
Permission Prompt not correctly dismissed on top window navigation
Reported by jm.acun...@gmail.com, Apr 11 2017 Back to list
UserAgent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce the problem:
1.Access the url: http://createcharts.esy.es/spoof-notify.html
2.Click button

What is the expected behavior?

What went wrong?
On the Google page, the notification window for another domain is displayed

Did this work before? N/A 

Chrome version: 57.0.2987.133  Channel: stable
OS Version: 6.3
Flash Version: Shockwave Flash 25.0 r0
 
ice_video_20170411-125516.webm
3.7 MB View Download
Once you press Allow, the result can be seen in the attached video.
ice_video_20170411-132054.webm
3.3 MB View Download
Tested in:

- Google Chrome Version 57.0.2987.133
- Google Chrome Version 59.0.3068.1 (Official build) canary (64 bits)
Basically, it is a process of loading times but I guarantee that in my pc, the spoof occurs 100% of the times I run the javascript code.
In Mozilla Firefox I have not been able to reproduce it.
Components: UI>Browser>Permissions>Prompts
Status: Untriaged
Summary: Permission Prompt not correctly dismissed on top window navigation (was: Spoof in google.com)
I can reproduce this with the latest 59.3068 Canary.
NotificationBubble.png
7.3 KB View Download
Comment 5 by meacer@chromium.org, Apr 11 2017
This is a variant of  bug 549724  (Security: Permission bubble runs callbacks on cross-origin pages). I WontFixed 549724 because it didn't display a modal dialog anymore, but it looks like the root cause of the issue isn't fixed yet.
Cc: dominickn@chromium.org raymes@chromium.org
Labels: -Pri-2 Security_Impact-Stable Security_Severity-Low Team-Security-UX OS-Linux Pri-1
Owner: dominickn@chromium.org
Status: Assigned
Cc: -dominickn@chromium.org timloh@chromium.org
Labels: OS-Mac
Reproduced on Mac Canary as well.

+timloh - you've been looking at permission prompt visibility / hiding things lately. Do you mind having a look at this one?
Comment 8 Deleted
If it helps, I think the problem is in the unload event
Definitive html code that exactly reproduces the case:

<script>
function notifyMe(e) {
	if(Notification.permission !== 'denied'){
		Notification.requestPermission(function(){});
	}
}
function w(){
	notifyMe();
	open('https://www.google.com', '_self');
	onunload = function(){ notifyMe(); }
}
</script>
<input type="button" onclick="w()" value="Test"/>
One last thing, 
Does this report qualify for VRP?
Cc: benwells@chromium.org
+benwells@

The key thing here is that Notification.requestPermission() is called in the unload handler (which really seems like something we might not want to allow).

The correct origin is displayed in the permission bubble and the result applies to the correct origin, so even though the prompt is displayed after the page navigates to google.com, the prompt still applies to (and saves the content setting) for createcharts.esy.es.
Also occurs from a local file
Cc: dominickn@chromium.org
Owner: timloh@chromium.org
Assigning to Tim to investigate why this is actually allowed / whether it's a race condition / etc.
Forgive me if you insist,

Because it is a bug that skips permissions between different domain origin,
Does this report qualify for VRP?
Had a brief poke around. This hits the DCHECK in ~PermissionServiceImpl (there should be no pending requests when it is destroyed). The unload event is being fired after we clear the pending requests. It doesn't seem to be a race condition but instead depending on where we navigate we go through different code-paths for unload/cancel?

When the prompt doesn't come up after the redirect (tested with navigation to https://example.com):

Cancel is from:
IPC
->.. RenderFrameHostImpl::OnDidCommitProvisionalLoad
->.. NavigationHandleImpl::~NavigationHandleImpl
->.. PermissionServiceContext::DidFinishNavigation (this is the WebContentsObserver API)

Unload is from:
IPC
-> ResourceDispatcher::OnReceivedData
->.. DocumentLoader::CommitData
->.. FrameLoader::CommitProvisionalLoad
->.. FrameLoader::DispatchUnloadEvent


When the prompt comes up on after the redirect (tested with navigation to https://chrome.google.com/webstore/):

Cancel is from:
IPC
-> RenderFrameHostManager::DidNavigateFrame
->.. RenderFrameHostManager::CommitPending
-> WebContentsImpl::NotifySwappedFromRenderManager
->.. PermissionServiceContext::RenderFrameHostChanged (this is the WebContentsObserver API)

Unload is from:
IPC
-> RenderFrameImpl::OnSwapOut
-> WebLocalFrameImpl::DispatchUnloadEvent
So to be clear, in the some cases the pending results are cleared, then the unload handler happens and another request is made?

I have no idea why the stacks are different for the two sites. That's pretty strange. Are you saying navigations to https://example.com always go one way that causes the prompts to not come up, and navigations to the webstore always go another way that does? If so do you know what makes those URLs special?

We could either:
a) try to understand all that and fix it so it always does the proper thing
b) somehow ignore requests that cause this problem, e.g. requests after pending_requests has been cleared, requests that don't match the current origin of the frame.

Any other ideas?
Looks to me like the second stack trace is from a prerender or something like that where the new frame is swapped in. Its navigation callbacks will have already happened in the background, but at the point of the renderer side swap in code running, the browser side swap in has already run (which would have wiped all the existing permission requests). So the currently loaded site fires the unload handler, which shows the permission prompt in the existing WebContents (since the swap changes the underlying RenderFrame hosted by the same WebContents.
Cc: clamy@chromium.org
+clamy@: Could you have a look at the last few comments here and see if you have any thoughts (or redirect if there's a more appropriate person on navigation)? Thanks!
Other information to consider:

- with the native navigation (browser back button) also appears the bubble for other domains visited.
ice_video_20170426-125637.webm
9.0 MB View Download
In this last example, the bubble is displayed for any domain.
Comment 24 by clamy@chromium.org, Apr 26 2017
Note: I'm not familiar with the PermissionServiceContext and don't know the rationale behind the specific calls it's listening to.

From the navigation perspective, the difference between the two example is that you have a same-process navigation in the first case and a cross-process navigation in the second case. The ordering of the commit of the navigation vs execution of the unload event is different in the two cases.

When you have a same-process navigation, the commit of the navigation happens after the provisional DocumentLoader becomes the main DocumentLoader. This has the effect of executing the unload event in the old document. So you get:
- unload event in the old document
- DidFinishNavigation (navigation commit in that case)

When you have a cross-process navigation, the commit of the navigation in the new process happens first, and then we ask the old process to execute its unload event in the background (that's the SwapOut IPC). So you get:
- RenderFrameHostChanged (navigation commit: you swap in the new RFH). You would also have a DidFinishNavigation soon after.
- unload event executes in the old document in the old process

Let me know if you have more navigation questions!
Thanks clamy@! That clears things up a lot.

The PermissionServiceContext is tied to a RenderFrameHost *OR* to a RenderProcessHost. That's kind of confusing. But I think a possible solution here is when a RFH is swapped out and RenderFrameHostChanged() is called, we should stop anything interesting from happening in PermissionServiceContext until it ends up being swapped back in (i.e. render_frame_host_ == new_host in RenderFrameHostChanged()).
Cc: reillyg@chromium.org
I think reillyg's patch at https://codereview.chromium.org/2842013002/ unintentionally fixes this. We now close the bindings when the render frame host changes (and in other cases too) so I believe we should no longer receive any messages to run code in the PermissionServiceImpl, which I think is what we want. 

reillyg: can you confirm it is true that no code in PermissionServiceImpl will run after we close the binding associated with it? It seemed not to in my test but I haven't looked at the code.

clamy: Basically we have a bunch of services tied to a particular RFH. What we want is that if that RFH ever finishes navigating in any way (except an in-page navigation) that we kill the services associated with it. So the question is: What events should we listen to in order to ensure we catch a RFH finishing its navigation? I'm guessing that just DidFinishNavigation isn't enough because we only find out about the current RFH and not any swapped out ones? Right now we're listing to:
-RenderFrameHostChanged
-FrameDeleted
-DidFinishNavigation

Is that the right set of things?

Thanks!
Yes, closing the bindings prevents any further Mojo messages from the renderer from being processed.
Thanks reillyg. 

clamy@ could you take a look at #26? Thanks!
Yes that seems the right set of observers. FYI, DevTools has a relatively similar issue in RenderFrameDevToolsAgentHost and listen to these. It also listens to ReadyToCommitNavigation since it wants to set up things in the renderer before the navigation commits (but IIUC you don't need it).
Status: Fixed
In that case I think reillyg's CL in https://codereview.chromium.org/2842013002/ is sufficient. Marking fixed. Thanks all for your help.
Project Member Comment 31 by sheriffbot@chromium.org, May 16
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Since I have not received a response to Comment 12 and Comment 16, do I understand that this bug does not qualify for VRP?
Then, It will get a CVE assigned?
Labels: M-60
Hi jm.acuna73@ - sorry, I hadn't seen your comments until now. I'm afraid low severity bugs don't currently qualify under the VRP. As the bug affected Stable, it should get a CVE assigned when the fix makes its way into a stable release, likely M60.  Thanks for your report and your updates in the bug! 
Thanks!
Labels: Release-0-M60
Labels: CVE-2017-5109
Project Member Comment 37 by sheriffbot@chromium.org, Aug 22
Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Sign in to add a comment