New issue
Advanced search Search tips

Issue 781922 link

Starred by 3 users

Issue metadata

Status: Available
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 781923
issue 803220
issue 803235

Blocking:
issue 779444
issue 786673



Sign in to add a comment

Site isolation enforcement for password autofill

Project Member Reported by lukasza@chromium.org, Nov 6 2017

Issue description

Let'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.
 
Blockedon: 781923
Cc: engedy@chromium.org
Blocking: 779444

Comment 4 by creis@chromium.org, Nov 18 2017

Blocking: 786673
Blockedon: 803235
Blockedon: 803220

Comment 7 by ma...@chromium.org, May 1 2018

Components: UI>Browser>Autofill
Cc: -vabr@chromium.org
vabr going hobby only -> reducing involvement.
Please contact me directly in urgent matters.
Owner: vasi...@chromium.org
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.
Cc: vasi...@chromium.org
Owner: dvadym@chromium.org
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.



Project Member

Comment 11 by bugdroid1@chromium.org, 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

Project Member

Comment 12 by bugdroid1@chromium.org, 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

Project Member

Comment 13 by bugdroid1@chromium.org, 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