Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2893633002:
[Password Manager] Make filling robust against changing url by JavaScript (Closed)
|
Created:
3 years, 7 months ago by dvadym Modified:
3 years, 7 months ago Reviewers:
kolos1 CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Password Manager] Make filling robust against changing url by JavaScript.
When PasswordAutofillAgent receives filling data from the browser, it checks that origin of this data is the same of the current frame origin. If JavaScript changes origin between PasswordAutofillAgent discovers a password form and when it receives filling, filling fails.
This CL replaces checking by origin to by checking by signon_realm (signon_realm=scheme:origin:port), which is a primary key for retrieving credentials from the store, so it doesn't change any security guarantees.
BUG=723679
Review-Url: https://codereview.chromium.org/2893633002
Cr-Commit-Position: refs/heads/master@{#472839}
Committed: https://chromium.googlesource.com/chromium/src/+/0273d9bd3e2d25f1485ab7a9469318c74ab7cb17
Patch Set 1 #Patch Set 2 : Test added #
Total comments: 11
Patch Set 3 : comment fixed #Patch Set 4 : rebase #Patch Set 5 : fix merge #
Messages
Total messages: 36 (23 generated)
Description was changed from ========== [Password Manager] Make filling robust against changing url by JavaScript BUG=723679 ========== to ========== [Password Manager] Make filling robust against changing url by JavaScript. When PasswordAutofillAgent receives filling data from the browser, it checks that origin of this data is the same of the current frame origin. If JavaScript changes origin between PasswordAutofillAgent discovers a password form and when it receives filling, filling fails. This CL replaces checking by origin, by checking by signon_realm, which is primary key for retrieving credentials from the store, so it doesn't change any security limitations but make Password Manager robust in such cases. BUG=723679 ==========
Description was changed from ========== [Password Manager] Make filling robust against changing url by JavaScript. When PasswordAutofillAgent receives filling data from the browser, it checks that origin of this data is the same of the current frame origin. If JavaScript changes origin between PasswordAutofillAgent discovers a password form and when it receives filling, filling fails. This CL replaces checking by origin, by checking by signon_realm, which is primary key for retrieving credentials from the store, so it doesn't change any security limitations but make Password Manager robust in such cases. BUG=723679 ========== to ========== [Password Manager] Make filling robust against changing url by JavaScript. When PasswordAutofillAgent receives filling data from the browser, it checks that origin of this data is the same of the current frame origin. If JavaScript changes origin between PasswordAutofillAgent discovers a password form and when it receives filling, filling fails. This CL replaces checking by origin to by checking by signon_realm (signon_realm=scheme:origin:port), which is a primary key for retrieving credentials from the store, so it doesn't change any security guarantees. BUG=723679 ==========
dvadym@chromium.org changed reviewers: + kolos@chromium.org
Hi Maxim, could you please review this CL? Regards, Vadym
On 2017/05/17 17:19:36, dvadym wrote: > Hi Maxim, > > could you please review this CL? > > Regards, > Vadym LGTM with some comments.
https://codereview.chromium.org/2893633002/diff/20001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2893633002/diff/20001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2899: // when JavaScript changes url. Didn't understand the comment. https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:281: if (data.action != data.origin) Shall we do similar change here? https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:781: std::string GetSignOnRealm(const GURL& origin) { There might be confusion with GetSignonRealm from chrome/browser/ui/login/login_handler.h. This function could not remove path as I understand. And we use it here https://cs.chromium.org/chromium/src/chrome/browser/ui/login/login_handler.cc... https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:783: rep.SetPathStr(""); Shall we also clear Query, Ref, Username, Password as we do for example here: https://cs.chromium.org/chromium/src/chrome/utility/importer/nss_decryptor.cc...
The CQ bit was checked by dvadym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for review! https://codereview.chromium.org/2893633002/diff/20001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2893633002/diff/20001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2899: // when JavaScript changes url. On 2017/05/18 09:52:52, kolos1 wrote: > Didn't understand the comment. Updated comments https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:281: if (data.action != data.origin) On 2017/05/18 09:52:52, kolos1 wrote: > Shall we do similar change here? This is a different case. This condition has nothing to do with current page, it's only about |data|, and |data| contains information that was on load. https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:781: std::string GetSignOnRealm(const GURL& origin) { On 2017/05/18 09:52:53, kolos1 wrote: > There might be confusion with GetSignonRealm from > chrome/browser/ui/login/login_handler.h. This function could not remove path as > I understand. And we use it here > https://cs.chromium.org/chromium/src/chrome/browser/ui/login/login_handler.cc... GetSignOnRealm is very good name :). I don't think that there will be any confusions, this function is in autofill namespace an it's used in rederer. That's exactly why namespaces are needed :) https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:783: rep.SetPathStr(""); On 2017/05/18 09:52:53, kolos1 wrote: > Shall we also clear Query, Ref, Username, Password as we do for example here: > https://cs.chromium.org/chromium/src/chrome/utility/importer/nss_decryptor.cc... This is just extracting of logic that we had for calculating of signon_realm. As far as I understand it removes everything, only scheme:host:port are left.
See the answers. Still LGTM https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:281: if (data.action != data.origin) On 2017/05/18 11:08:52, dvadym wrote: > On 2017/05/18 09:52:52, kolos1 wrote: > > Shall we do similar change here? > > This is a different case. This condition has nothing to do with current page, > it's only about |data|, and |data| contains information that was on load. Acknowledged. https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:781: std::string GetSignOnRealm(const GURL& origin) { On 2017/05/18 11:08:52, dvadym wrote: > On 2017/05/18 09:52:53, kolos1 wrote: > > There might be confusion with GetSignonRealm from > > chrome/browser/ui/login/login_handler.h. This function could not remove path > as > > I understand. And we use it here > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/login/login_handler.cc... > > GetSignOnRealm is very good name :). I don't think that there will be any > confusions, this function is in autofill namespace an it's used in rederer. > That's exactly why namespaces are needed :) Acknowledged. https://codereview.chromium.org/2893633002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:783: rep.SetPathStr(""); On 2017/05/18 11:08:52, dvadym wrote: > On 2017/05/18 09:52:53, kolos1 wrote: > > Shall we also clear Query, Ref, Username, Password as we do for example here: > > > https://cs.chromium.org/chromium/src/chrome/utility/importer/nss_decryptor.cc... > > This is just extracting of logic that we had for calculating of signon_realm. As > far as I understand it removes everything, only scheme:host:port are left. Acknowledged.
The CQ bit was unchecked by dvadym@chromium.org
The CQ bit was checked by dvadym@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dvadym@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dvadym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dvadym@chromium.org
The CQ bit was checked by dvadym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dvadym@chromium.org
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kolos@chromium.org Link to the patchset: https://codereview.chromium.org/2893633002/#ps80001 (title: "fix merge")
The CQ bit was checked by dvadym@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dvadym@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495122395862800,
"parent_rev": "3e527f87b33591975570c2e71487ac8d6d0480f1", "commit_rev":
"0273d9bd3e2d25f1485ab7a9469318c74ab7cb17"}
Message was sent while issue was closed.
Description was changed from ========== [Password Manager] Make filling robust against changing url by JavaScript. When PasswordAutofillAgent receives filling data from the browser, it checks that origin of this data is the same of the current frame origin. If JavaScript changes origin between PasswordAutofillAgent discovers a password form and when it receives filling, filling fails. This CL replaces checking by origin to by checking by signon_realm (signon_realm=scheme:origin:port), which is a primary key for retrieving credentials from the store, so it doesn't change any security guarantees. BUG=723679 ========== to ========== [Password Manager] Make filling robust against changing url by JavaScript. When PasswordAutofillAgent receives filling data from the browser, it checks that origin of this data is the same of the current frame origin. If JavaScript changes origin between PasswordAutofillAgent discovers a password form and when it receives filling, filling fails. This CL replaces checking by origin to by checking by signon_realm (signon_realm=scheme:origin:port), which is a primary key for retrieving credentials from the store, so it doesn't change any security guarantees. BUG=723679 Review-Url: https://codereview.chromium.org/2893633002 Cr-Commit-Position: refs/heads/master@{#472839} Committed: https://chromium.googlesource.com/chromium/src/+/0273d9bd3e2d25f1485ab7a94693... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0273d9bd3e2d25f1485ab7a94693... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
