Site isolation enforcement for password autofill |
||||||||||
Issue descriptionLet's use this bug to track the work needed for proper enforcement of site isolation from password autofill perspective. The goal is that once site isolation ships, a renderer hosting origin X should not be able to 1) get access to data from isolated origin Y and/or 2) spoof isolated origin Y. To achieve this, the browser should use trusted browser-side knowledge about the sender (its last committed url/origin, bounding box, etc.) rather then trust what the renderer reports. This can be achieved by a combination of 1) avoiding sending some things (like origins) over IPC and 2) verifying other things (like bounding boxes). Examples: - autofill::mojom::FormData::origin should not be sent over IPC and trusted by ContentAutofillDriver in the browser process. - Implementation of autofill::mojom::AutofillDriver should verify that |bounding_box| sent over the IPC (e.g. in QueryFormFieldAutofill) is within the bounding box of the sending frame.
,
Nov 6 2017
,
Nov 6 2017
,
Nov 18 2017
,
Jan 17 2018
,
Jan 17 2018
,
May 1 2018
,
Nov 29
vabr going hobby only -> reducing involvement. Please contact me directly in urgent matters.
,
Dec 21
vasilii@, is there more work to be done here? I think the some PasswordManager-related protections are already in place (e.g. part of the work landed in r569331) - for example I see that PasswordForm::origin and PasswordForm::signon_realm are verified via CheckChildProcessSecurityPolicyForURL. OTOH, there are other origin-like fields that are sent over IPC from the renderer process - do we need to also cover them, or are they okay? Do we need to audit //components/autofill/content/common/autofill_types.mojom for other origin-like fields: struct FormData { ... url.mojom.Url origin; ... url.mojom.Origin main_frame_origin; ... }; struct PasswordAndRealm { ... string realm; }; struct PasswordFormFillData { ... url.mojom.Url origin; // Is this protected by CheckChildProcessSecurityPolicyForURL? ... string preferred_realm; ... }; struct PasswordForm { ... string signon_realm; url.mojom.Url origin_with_path; // Is this protected by CheckChildProcessSecurityPolicyForURL? ... string affiliated_web_realm; ... url.mojom.Origin federation_origin; ... }; (note that above I focused on fields that seemed origin-like to me; there may be other fields [renderer ids? urls? that may also need Site Isolation enforcement) FWIW, for the bounding box verification I've opened a separate issue 917432.
,
Jan 7
I think - FormData::origin and main_frame_origin can be removed. - PasswordAndRealm sent from browser to renderer. Should be OK. - PasswordFormFillData sent from browser to renderer. Should be OK. - PasswordForm seems too big for passing from renderer to browser. I think we should pass a smaller struct and then |signon_realm|, |origin_with_path|, |affiliated_web_realm|, |federation_origin| are all to be erased from it.
,
Jan 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b8251dc8f8eb5b27170944b7bea8d24f8cd1d269 commit b8251dc8f8eb5b27170944b7bea8d24f8cd1d269 Author: Vadym Doroshenko <dvadym@chromium.org> Date: Tue Jan 08 18:54:51 2019 Check FormData::origin with CheckChildProcessSecurityPolicy. Bug: 467150, 781922 Change-Id: Ib7c977541bb64bc0c0b1469ac3aed7e009e64819 Reviewed-on: https://chromium-review.googlesource.com/c/1400809 Commit-Queue: Vadym Doroshenko <dvadym@chromium.org> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Cr-Commit-Position: refs/heads/master@{#620818} [modify] https://crrev.com/b8251dc8f8eb5b27170944b7bea8d24f8cd1d269/components/password_manager/content/browser/bad_message.cc
,
Jan 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/617f139e0f4e28b277e8aee4d9fe23318e30f4da commit 617f139e0f4e28b277e8aee4d9fe23318e30f4da Author: François Doray <fdoray@chromium.org> Date: Tue Jan 08 20:06:29 2019 Revert "Check FormData::origin with CheckChildProcessSecurityPolicy." This reverts commit b8251dc8f8eb5b27170944b7bea8d24f8cd1d269. Reason for revert: Suspect this for failure on "Leak Detection Linux" bot. First failed build: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Leak%20Detection%20Linux/29571 Log shows that failure is in chrome!password_manager::bad_message::(anonymous namespace)::ReceivedBadMessage: Thread 0 (crashed) 0 chrome!google_breakpad::ExceptionHandler::WriteMinidump() + 0xa1 1 chrome!breakpad::(anonymous namespace)::DumpProcess() + 0x15 2 chrome!base::debug::DumpWithoutCrashing() + 0x12 3 chrome!content::RenderProcessHostImpl::ShutdownForBadMessage(content::RenderProcessHost::CrashReportMode) + 0x231 4 chrome!password_manager::bad_message::(anonymous namespace)::ReceivedBadMessage(content::RenderProcessHost*, password_manager::BadMessageReason) + 0xf1 5 chrome!password_manager::bad_message::(anonymous namespace)::CheckChildProcessSecurityPolicyForURL(content::RenderFrameHost*, GURL const&, password_manager::BadMessageReason) + 0xe1 6 chrome!password_manager::bad_message::CheckChildProcessSecurityPolicy(content::RenderFrameHost*, std::__1::vector<autofill::PasswordForm, std::__1::allocator<autofill::PasswordForm> > const&, password_manager::BadMessageReason) + 0xa0 7 chrome!non-virtual thunk to ChromePasswordManagerClient::PasswordFormsParsed(std::__1::vector<autofill::PasswordForm, std::__1::allocator<autofill::PasswordForm> > const&) + 0x21 8 chrome!autofill::mojom::PasswordManagerDriverStubDispatch::Accept(autofill::mojom::PasswordManagerDriver*, mojo::Message*) + 0x795 9 chrome!IPC::(anonymous namespace)::ChannelAssociatedGroupController::AcceptOnProxyThread(mojo::Message) + 0x85 10 chrome!base::internal::Invoker<base::internal::BindState<void (IPC::(anonymous namespace)::ChannelAssociatedGroupController::*)(mojo::Message), scoped_refptr<IPC::(anonymous namespace)::ChannelAssociatedGroupController>, base::internal::PassedWrapper<mojo::Message> >, void ()>::Run(base::internal::BindStateBase*) + 0xa9 https://chromium-swarm.appspot.com/task?id=4247af66237c2310&refresh=10&show_raw=1 Original change's description: > Check FormData::origin with CheckChildProcessSecurityPolicy. > > Bug: 467150, 781922 > Change-Id: Ib7c977541bb64bc0c0b1469ac3aed7e009e64819 > Reviewed-on: https://chromium-review.googlesource.com/c/1400809 > Commit-Queue: Vadym Doroshenko <dvadym@chromium.org> > Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> > Cr-Commit-Position: refs/heads/master@{#620818} TBR=vasilii@chromium.org,dvadym@chromium.org Change-Id: If0c1e13141fb240071c94e665d7e8a39ab0f401a No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 467150, 781922 Reviewed-on: https://chromium-review.googlesource.com/c/1401404 Reviewed-by: François Doray <fdoray@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#620851} [modify] https://crrev.com/617f139e0f4e28b277e8aee4d9fe23318e30f4da/components/password_manager/content/browser/bad_message.cc
,
Jan 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/48aa04ef9cbf2c1f0a60a9796f1a362b2fa595d9 commit 48aa04ef9cbf2c1f0a60a9796f1a362b2fa595d9 Author: Vadym Doroshenko <dvadym@chromium.org> Date: Thu Jan 10 11:12:56 2019 Check FormData::origin with CheckChildProcessSecurityPolicy. This is a reland of https://chromium-review.googlesource.com/c/chromium/src/+/1400809 with setting checked parameter in one place where it was missing. Bug: 467150, 781922 Change-Id: Ice31008f1b26c4f0c8a08dd806d967e667087eb8 Reviewed-on: https://chromium-review.googlesource.com/c/1404163 Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Commit-Queue: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#621541} [modify] https://crrev.com/48aa04ef9cbf2c1f0a60a9796f1a362b2fa595d9/components/autofill/content/renderer/password_autofill_agent.cc [modify] https://crrev.com/48aa04ef9cbf2c1f0a60a9796f1a362b2fa595d9/components/password_manager/content/browser/bad_message.cc |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by lukasza@chromium.org
, Nov 6 2017