Security: Credential Manager API origin confusion |
|||||||||||||||
Issue descriptionVULNERABILITY DETAILS The Credential Manager API is implemented on the browser/ side by means of one password_manager::CredentialManagerImpl instance per WebContents (the same instance for the entire life of a WebContents), because it is intended for main-frame callers only. When each API call is serviced, the origin of the caller, thus the realm of credentials to store or return, is determined by web_contents()->GetLastCommittedURL(). This is incorrect. For example, during cross-site navigations in the main frame, it can happen that there are still pending navigator.credentials.get/store() Mojo calls initiated by the old RFH when the pending RFH is committed and thus the origin returned by GetLastCommittedURL changes. After this, API calls are now serviced in the context of the new main-frame origin. However, the response callbacks of pending requests are still targeted at the old RenderFrame, and have a good chance of being delivered to the old RenderFrame, as it is not immediately destroyed at this point (and/or the non-synchronized Mojo message pipe to it not immediately disconnected). Passwords can thus be leaked cross-origin. VERSION Chrome Version: -- Reproduced in 61.0.3139.0. -- Plain text passwords are exposed to JS starting in 60.0.3108.0 (https://codereview.chromium.org/2852423002/). -- Before that, the API never exposed plain text passwords to JS, but returned an opaque Credentials object, that needed to be POST'ed. It was not tested, but is conceivable that a malicious attacker could post this to an origin controlled by that attacker. Operating System: All REPRODUCTION CASE A malicious site could start spamming navigator.credentials.get() calls shortly before it's unloaded to try to glean the zero-click-signin-enabled stored password for whatever origin is opened in the same WebContents next (either by the waiting for the user to navigate by typing into the Omnibox, or triggering the navigation itself).
,
Jun 23 2017
Sounds legit, though I'm not familiar with the code of Credential Manager API. Vaclav, would you mind taking a look and helping to triage this? I'm setting Medium security severity as per the guidelines: https://www.chromium.org/developers/severity-guidelines
,
Jun 23 2017
,
Jun 23 2017
Shouldn't we just close the Mojo connection on finish navigation? It's not clear to me why CredentialManagerImpl contains a BindingSet instead of Binding. Seems like there should be only one connection at any point of time.
,
Jun 23 2017
Yes, that combined with accepting an InterfaceRequest only if it comes from the committed RFH seems like to do the trick. It is also reasonably simple to be merged.
,
Jun 24 2017
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it. If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 26 2017
I think I heard engedy@ is working on a fix, so assigning to him. Alternatively, vasilii@ or mkwst@ seem to be better candidates for owner than I am.
,
Jun 26 2017
This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label. All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 26 2017
,
Jun 29 2017
,
Jul 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/afef048df9416f31f2d90535c3ca946ac3f43b80 commit afef048df9416f31f2d90535c3ca946ac3f43b80 Author: engedy <engedy@chromium.org> Date: Thu Jul 06 15:40:53 2017 Restrict CM API interface request and message dispatch. Add safeguards to ensure that interface requests as well as RPCs are only dispatched, and thus serviced, when coming from the document corresponding to the last committed navigation in the currently active render frame. BUG= 736357 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2947413002 Cr-Commit-Position: refs/heads/master@{#484608} [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/chrome/browser/password_manager/chrome_password_manager_client.cc [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/chrome/browser/password_manager/chrome_password_manager_client.h [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/chrome/browser/password_manager/chrome_password_manager_client_unittest.cc [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/chrome/browser/password_manager/credential_manager_browsertest.cc [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/components/password_manager/content/browser/credential_manager_impl.cc [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/components/password_manager/content/browser/credential_manager_impl.h [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/components/password_manager/content/renderer/credential_manager_client.cc [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/components/password_manager/content/renderer/credential_manager_client.h [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/components/password_manager/content/renderer/credential_manager_client_browsertest.cc [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/content/public/browser/content_browser_client.cc [modify] https://crrev.com/afef048df9416f31f2d90535c3ca946ac3f43b80/content/public/browser/content_browser_client.h
,
Jul 6 2017
I suppose we will want to merge this fix to M60. Given that M60 ships in slightly more than 2 week, what do you all think, is it worth merging to M59?
,
Jul 7 2017
,
Jul 7 2017
I very much doubt there'll be another M59 release, but yes, we should get this into M60 once it's had some time to bake.
,
Jul 8 2017
,
Jul 8 2017
This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 10 2017
Approving merge to M60.
,
Jul 10 2017
Please merge this to M60 ASAP. branch:3112
,
Jul 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/740029d9b69552069c6bec432e3f2e3756d53b1d commit 740029d9b69552069c6bec432e3f2e3756d53b1d Author: Balazs Engedy <engedy@chromium.org> Date: Wed Jul 12 14:12:44 2017 Restrict CM API interface request and message dispatch. Add safeguards to ensure that interface requests as well as RPCs are only dispatched, and thus serviced, when coming from the document corresponding to the last committed navigation in the currently active render frame. BUG= 736357 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=engedy@chromium.org (cherry picked from commit afef048df9416f31f2d90535c3ca946ac3f43b80) Review-Url: https://codereview.chromium.org/2947413002 Cr-Original-Commit-Position: refs/heads/master@{#484608} Change-Id: Ib9fe96fd60ae27f3035f31e2723e8a78bb448857 Reviewed-on: https://chromium-review.googlesource.com/568029 Reviewed-by: Balazs Engedy <engedy@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#592} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/chrome/browser/password_manager/chrome_password_manager_client.cc [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/chrome/browser/password_manager/chrome_password_manager_client.h [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/chrome/browser/password_manager/chrome_password_manager_client_unittest.cc [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/chrome/browser/password_manager/credential_manager_browsertest.cc [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/components/password_manager/content/browser/credential_manager_impl.cc [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/components/password_manager/content/browser/credential_manager_impl.h [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/components/password_manager/content/renderer/credential_manager_client.cc [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/components/password_manager/content/renderer/credential_manager_client.h [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/components/password_manager/content/renderer/credential_manager_client_browsertest.cc [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/content/public/browser/content_browser_client.cc [modify] https://crrev.com/740029d9b69552069c6bec432e3f2e3756d53b1d/content/public/browser/content_browser_client.h
,
Jul 12 2017
,
Oct 13 2017
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 |
|||||||||||||||
Comment 1 by elawrence@chromium.org
, Jun 23 2017