Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
163843002:
Fix check for user gesture on password autofill (Closed)
|
Created:
6 years, 10 months ago by vabr (Chromium) Modified:
6 years, 10 months ago CC:
chromium-reviews, benquan, jam, browser-components-watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org, abarth-chromium, Garrett Casto Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix check for user gesture on password autofill
The original security feature is:
If passwords get autofilled on page load, do not make them accessible to JavaScript code until the user interacts with the page.
Formerly, this was implemented as https://codereview.chromium.org/83023017 (Chromium) + https://codereview.chromium.org/82693006 (Blink). That implementation used the Blink user gesture support in a wrong way (https://codereview.chromium.org/82693006/#msg14), which resulted in http://crbug.com/337429.
This Cl attempts to fix the problem by going through RenderWidget::OnHandleInputEvent. At that point we can see user events and filter them using WebInputEvent::isUserGestureEventType. Through RenderViewObserver, we then notify the corresponding PasswordAutofillAgent.
Testing:
This CL adds a test replicating the problem from http://crbug.com/337429. vabr checked that the new test fails without the new fix.
All the tests put in place by https://codereview.chromium.org/83023017 are also relevant.
vabr also manually checked with the browser, that the issue reported in http://crbug.com/337429 no longer reproduces with this CL.
BUG=337429, 163072
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252352
Patch Set 1 #
Total comments: 4
Patch Set 2 : Have 'user' in the gesture variable name #Patch Set 3 : Fix PasswordManagerBrowserTest.VerifyPasswordGenerationUpload #
Total comments: 2
Patch Set 4 : Add test #Patch Set 5 : Uploading a missing test file #Patch Set 6 : New implementation using isUserGestureEventType #
Total comments: 16
Patch Set 7 : Ilya's comments addressed #Patch Set 8 : Just rebased #Patch Set 9 : Fix compilation #
Total comments: 6
Patch Set 10 : Rename ProcessingUserGestureEvent to NotifyAboutUserGesture #
Total comments: 4
Patch Set 11 : Use SimulateMouseClick & Rename NotifyAboutUserGesture -> WillProcessUserGesture #
Total comments: 2
Patch Set 12 : Correct comment #Messages
Total messages: 33 (0 generated)
Garrett: FYI. Joel, What do you say to this CL? I tested locally, and it solves the issue in http://crbug.com/337429. If we used, we can also drop the Blink support from https://codereview.chromium.org/82693006. This CL lacks the browser test to replay the issue from http://crbug.com/337429. I'm working on it, but given that tests usually take me 3 times longer than fixes, it might not be on time for the v34 branch. If we are in a rush, maybe we can provide the tests separately, as they are more important to get to trunk than to the branches. Let me know what you think (also about reverting https://codereview.chromium.org/83023017 in version 33 and 34). Thanks, Vaclav
On 2014/02/13 17:01:32, vabr (Chromium) wrote: > Garrett: FYI. > > Joel, > > What do you say to this CL? I tested locally, and it solves the issue in > http://crbug.com/337429. If we used, we can also drop the Blink support from > https://codereview.chromium.org/82693006. > > This CL lacks the browser test to replay the issue from http://crbug.com/337429. > I'm working on it, but given that tests usually take me 3 times longer than > fixes, it might not be on time for the v34 branch. If we are in a rush, maybe we > can provide the tests separately, as they are more important to get to trunk > than to the branches. > > Let me know what you think (also about reverting > https://codereview.chromium.org/83023017 in version 33 and 34). > > Thanks, > Vaclav Generally looks pretty good, modulo a few minor comments. For the tests, shouldn't most of the current unit tests for the user gestures with the password manager still apply? That is, will it really require an entire new suite a unit tests? And, yes, we should probably revert the old Blink changes, since that API will no longer be needed (assuming no dependencies have been added to it since).
Drive-by: I don't know who initially added the UserGesture API in Blink, but please get their review as well. There might be subtleties around user gestures that we're not aware of.
On 2014/02/13 21:20:52, Ilya Sherman wrote: > Drive-by: I don't know who initially added the UserGesture API in Blink, but > please get their review as well. There might be subtleties around user gestures > that we're not aware of. I added the extension to the UserGesture API that let us use it in Chromium (with consultation of abarth). The original API exists basically to tell it the renderer is currently processing a user gesture, and we tried to extend that to let us know if a user gesture has occurred at all, however it never was meant to be "stateful" in that sense, and as vabr described, we messed up and didn't consider it in the presence of multiple tabs. I'm adding abarth to this, just in case he wants to chime in.
https://codereview.chromium.org/163843002/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/163843002/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:510: void PasswordAutofillAgent::DidHandleKeyEvent() { To ask what I think is a silly question, these functions can only be called from a user generated gesture, not from a JavaScript event? That is, the following will not trigger this functionZ var event = new MouseEvent('click'); var cb = document.getElementById('checkbox'); cb.dispatchEvent(event); https://codereview.chromium.org/163843002/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/163843002/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.h:105: bool was_gesture_seen_; nit: I'd suggest renaming this to something like was_user_gesture_seen_ just so it's more explicit that it must be a user gesture, not a JavaScript-simulated gesture.
On 2014/02/13 21:33:29, jww wrote: > On 2014/02/13 21:20:52, Ilya Sherman wrote: > > Drive-by: I don't know who initially added the UserGesture API in Blink, but > > please get their review as well. There might be subtleties around user > gestures > > that we're not aware of. > > I added the extension to the UserGesture API that let us use it in Chromium > (with consultation of abarth). The original API exists basically to tell it the > renderer is currently processing a user gesture, and we tried to extend that to > let us know if a user gesture has occurred at all, however it never was meant to > be "stateful" in that sense, and as vabr described, we messed up and didn't > consider it in the presence of multiple tabs. I'm adding abarth to this, just in > case he wants to chime in. Ah, I didn't realize that you were the original author of (the extension of) that API. Sounds good then, thanks :)
Thanks Joel and Ilya. I addressed Joel's comments. As for the tests -- indeed, the tests from https://codereview.chromium.org/83023017 still apply (and are important to restore if we end up reverting that for branches). I was talking about a special test to simulate the issue from http://crbug.com/337429. It looks like it could be OK to add that in a separate CL in case we end up in a hurry. Cheers, Vaclav https://codereview.chromium.org/163843002/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/163843002/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:510: void PasswordAutofillAgent::DidHandleKeyEvent() { On 2014/02/13 21:34:00, jww wrote: > To ask what I think is a silly question, these functions can only be called from > a user generated gesture, not from a JavaScript event? That is, the following > will not trigger this functionZ > var event = new MouseEvent('click'); > var cb = document.getElementById('checkbox'); > cb.dispatchEvent(event); Thanks for bringing this up! I double-checked, and such dispatchEvent won't trigger DidHandleMouseEvent (while it would toggle the checkbox). I believe this is also tested by your PasswordAutofillAgentTest.NoDOMActivationTest, right? https://codereview.chromium.org/163843002/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/163843002/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.h:105: bool was_gesture_seen_; On 2014/02/13 21:34:00, jww wrote: > nit: I'd suggest renaming this to something like was_user_gesture_seen_ just so > it's more explicit that it must be a user gesture, not a JavaScript-simulated > gesture. Done.
https://codereview.chromium.org/163843002/diff/210001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/163843002/diff/210001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:123: virtual void DidHandleKeyEvent() OVERRIDE; did you verify that these callbacks aren't triggered if the javascript dispatches an event?
https://codereview.chromium.org/163843002/diff/210001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/163843002/diff/210001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:123: virtual void DidHandleKeyEvent() OVERRIDE; On 2014/02/14 12:00:41, jochen wrote: > did you verify that these callbacks aren't triggered if the javascript > dispatches an event? Yes, please see my response to Joel's comment in patch set 1.
ok, the callbacks are all invoked from RenderWidget::OnHandleInputEvent. However, if the page handles the events, they won't be invoked (see the prevent_default block at the end of OnHandleInputEvent), no?
On 2014/02/14 12:42:15, jochen wrote: > ok, the callbacks are all invoked from RenderWidget::OnHandleInputEvent. > > However, if the page handles the events, they won't be invoked (see the > prevent_default block at the end of OnHandleInputEvent), no? That's a very good question. Especially clicks on submit buttons look like they might have to end up with prevent_default true, if I understand it correctly. According to the code, keyboard events are always causing prevent_default == false, so at least there will be a workaround. I'm not sure what to do about the prevent_default==true cases yet, but my suggestion is: Since we are ending up reverting the original feature on stable, and probably on beta as well (?), we can try to see how much the prevent_default proves to be a problem on dev&canary and try to patch that later. It should not be worse than it is now.
Welcome new reviewers on this CL :). The existing reviewers, please note that patch set 6 changed the implementation a bit. Below is what I need from whom, the CL summary should give you enough context. James and Jói, please review adding the new ProcessingUserGestureEvent() method (see also the note at the end of the e-mail): joi@chromium.org content/public/renderer/render_view_observer.h jamesr@chromium.org content/renderer/render_view_impl.h content/renderer/render_view_impl.cc content/renderer/render_widget.h content/renderer/render_widget.cc Ilya, could you please review my changes to to the autofill agent? isherman@chromium.org components/autofill/content/renderer/password_autofill_agent.h components/autofill/content/renderer/password_autofill_agent.cc Joel, please review the following files, and ideally the CL as a whole: jww@chromium.org chrome/browser/password_manager/password_manager_browsertest.cc chrome/test/data/password/form_and_link.html Jochen is obviously welcome to comment on anything as well. (And thanks for your help so far!) James, Jói -- in case you deem patch set 6 inappropriately intrusive, check out patch set 5. Patch set 5 touches less files, but has a problem of not catching some events (see https://codereview.chromium.org/163843002/#msg10). That's why I prefer p.s. 6, but I'm open to discussions. Thanks all, Vaclav
Thanks! //components/autofill/content/renderer/ LGTM with nits: https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:231: for (std::vector<blink::WebInputElement>::iterator iter = elements_.begin(); Optional nit: "iter" -> "it" (for consistency with most of the surrounding code) https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:243: } nit: Please order the implementation code within the .cc file to match the order of the declarations within the .h file. https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:571: // New navigation means the need to wait for a new user gesture before nit: Suggested rephrasing: "This is a new navigation, so require a new user gesture before filling in passwords." https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:808: // We wait with filling in the password until a user gesture occurred. This is nit: "We wait with filling in" -> "Wait to fill in" https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:808: // We wait with filling in the password until a user gesture occurred. This is nit: "occurred" -> "occurs" https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:92: void RegisterElement(blink::WebInputElement& element); nit: Please pass by const-reference, pointer, or copy. (Since this is a reference-counting wrapper, by copy is probably the best choice; but all are fine.) https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:96: void OnGesture(); nit: Perhaps "OnUserInteraction()" or "OnUserGesture()"? https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:103: void ShowValue(blink::WebInputElement& element); nit: Please pass by const-reference, pointer, or copy. (Since this is a reference-counting wrapper, by copy is probably the best choice; but all are fine.)
It seems very odd to fire a notification to indicating that we're about to process a user gesture without any sort of indication that we are no longer doing so. The code you have up in chrome/ appears to keep the in_user_gesture bit around until the next navigation, which I'm having trouble understanding. In Blink, I believe we have some more complex code to decide if at any point in time we are currently associated with a user gesture or not. Can we reuse the same logic we have for "is this considered associated with a user gesture" instead of inventing a new thingy? If we are forced to invent a new thingy, wouldn't it make more sense for content embedders to ask content:: if it considers itself to be processing a user gesture at that point in time?
On 2014/02/14 23:22:19, jamesr wrote: > It seems very odd to fire a notification to indicating that we're about to > process a user gesture without any sort of indication that we are no longer > doing so. The code you have up in chrome/ appears to keep the in_user_gesture > bit around until the next navigation, which I'm having trouble understanding. > In Blink, I believe we have some more complex code to decide if at any point in > time we are currently associated with a user gesture or not. > > Can we reuse the same logic we have for "is this considered associated with a > user gesture" instead of inventing a new thingy? If we are forced to invent a > new thingy, wouldn't it make more sense for content embedders to ask content:: > if it considers itself to be processing a user gesture at that point in time? So far, we can ask "is this event associated with a user gesture" for a given event, "user gesture" is a global state of the browser, and once you have an event, you just check this state. The problem here is that we don't have an event given. The questions Vaclav tries to answer is "has any event occurred that was associated with an user gesture", so the code tries to catch "any event".
Thanks, Ilya, your comments are addressed. James, (sorry if I'm repeating what Jochen said in the meantime,) The new code from this CL wants to detect, for each tab separately, the moment when the user first interacts with a page, and remember it until the page changes (new navigation). What I found in Blink (WebUserGestureIndicator & friends), is a way to tell if Blink is currently processing a user gesture. The emphasis is on "currently" (indication kept on stack during the processing, there are timers to check for outdated events). There is also no good way to tell what tab this interaction happens in. These Blink features were used by the old implementation of the "user gesture for autofill" feature, which was deemed faulty (https://codereview.chromium.org/82693006/#msg14) and should be fixed by this CL. As for your second comment about content:: -- do you mean moving the PasswordValueGatekeeper logic into RenderWidget? That sounds fine by me, but I'm not sure if I understand you correctly. Thanks, Vaclav https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:231: for (std::vector<blink::WebInputElement>::iterator iter = elements_.begin(); On 2014/02/14 23:07:33, Ilya Sherman wrote: > Optional nit: "iter" -> "it" (for consistency with most of the surrounding code) Done. https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:243: } On 2014/02/14 23:07:33, Ilya Sherman wrote: > nit: Please order the implementation code within the .cc file to match the order > of the declarations within the .h file. Done for the definitions of the added methods (the existing ones of PasswordAutofillAgent don't seem to match the declaration order. https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:571: // New navigation means the need to wait for a new user gesture before On 2014/02/14 23:07:33, Ilya Sherman wrote: > nit: Suggested rephrasing: "This is a new navigation, so require a new user > gesture before filling in passwords." Done. https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:808: // We wait with filling in the password until a user gesture occurred. This is On 2014/02/14 23:07:33, Ilya Sherman wrote: > nit: "We wait with filling in" -> "Wait to fill in" Done. https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:808: // We wait with filling in the password until a user gesture occurred. This is On 2014/02/14 23:07:33, Ilya Sherman wrote: > nit: "occurred" -> "occurs" Done. https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:92: void RegisterElement(blink::WebInputElement& element); On 2014/02/14 23:07:33, Ilya Sherman wrote: > nit: Please pass by const-reference, pointer, or copy. (Since this is a > reference-counting wrapper, by copy is probably the best choice; but all are > fine.) Thanks for catching this! I went with the pointer, as I don't think it's worse than a copy, and it makes it clearer that the original element is modified. Done. https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:96: void OnGesture(); On 2014/02/14 23:07:33, Ilya Sherman wrote: > nit: Perhaps "OnUserInteraction()" or "OnUserGesture()"? Done. I chose OnUserGesture to match the naming from blink (isUserGestureEventType). https://codereview.chromium.org/163843002/diff/520001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:103: void ShowValue(blink::WebInputElement& element); On 2014/02/14 23:07:33, Ilya Sherman wrote: > nit: Please pass by const-reference, pointer, or copy. (Since this is a > reference-counting wrapper, by copy is probably the best choice; but all are > fine.) Thanks! Done, the same as RegisterElement.
https://codereview.chromium.org/163843002/diff/710001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/163843002/diff/710001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:183: void SimulateClick(); There's already a SimulateElementClick() function that's used in these browser_tests. You might consider reusing that rather than rolling your own here. No super important, though, and this has the advantage of not being tied to a particular element. https://codereview.chromium.org/163843002/diff/710001/chrome/test/data/passwo... File chrome/test/data/password/form_and_link.html (left): https://codereview.chromium.org/163843002/diff/710001/chrome/test/data/passwo... chrome/test/data/password/form_and_link.html:4: <iframe src="password_form.html" id="first_frame" name="first_frame"> I'm pretty confused about why you're deleting these frames. It seems like you're renaming multi_frames.html, in which case these frames are still being used by several browser tests. Am I misunderstanding what's going on here? https://codereview.chromium.org/163843002/diff/710001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/163843002/diff/710001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:573: gatekeeper_.Reset(); Perhaps it's too late to ask this question anyway, but in the old WebUserGesture system, couldn't we have just allocated a new handler at this point and fixed the problem? That is, without this new refactor, if on every navigation we created a new gesture_handler_, and set the old one to be ref counted, couldn't we have solved this issue without the new implementation? There's probably good reason to do the refactor anyway, so feel free to blow this question off if you think it's not worth thinking about.
On 2014/02/17 14:16:59, jochen wrote: > On 2014/02/14 23:22:19, jamesr wrote: > > It seems very odd to fire a notification to indicating that we're about to > > process a user gesture without any sort of indication that we are no longer > > doing so. The code you have up in chrome/ appears to keep the in_user_gesture > > bit around until the next navigation, which I'm having trouble understanding. > > In Blink, I believe we have some more complex code to decide if at any point > in > > time we are currently associated with a user gesture or not. > > > > Can we reuse the same logic we have for "is this considered associated with a > > user gesture" instead of inventing a new thingy? If we are forced to invent a > > new thingy, wouldn't it make more sense for content embedders to ask content:: > > if it considers itself to be processing a user gesture at that point in time? > > So far, we can ask "is this event associated with a user gesture" for a given > event, "user gesture" is a global state of the browser, and once you have an > event, you just check this state. > > The problem here is that we don't have an event given. > > The questions Vaclav tries to answer is "has any event occurred that was > associated with an user gesture", so the code tries to catch "any event". OK - if the intent here is to ask "has this view ever handled any user event" then the function name should reflect that. I still think a polling thing makes more sense than a notification that the embedder just has to cache somewhere. I also really doubt this is ever a useful bit - what are the situations where a widget is loaded and *never* has any sort of events fired at it?
Joel, Thanks for the comments, please check my responses below. James, I'm afraid there is still a misunderstanding, sorry for that (and thanks for your time spent on this). The goal, citing the description of this CL is: "If passwords get autofilled on page load, do not make them accessible to JavaScript code until the user interacts with the page." > OK - if the intent here is to ask "has this view ever handled any user event" This is not exactly the intention here. It's rather: "tell me immediately, when the user first interacts with the view". > then the function name should reflect that. My bad naming probably caused most of the misunderstanding. I'll try to think about better names, and update the CL. > I still think a polling thing makes more sense than a notification that the embedder just has to cache somewhere. I would be fine with caching it in the RenderWidget instead, as long as the embedder is notified immediately on the first user interaction. Do you want me to change the place of caching, or does it no more make sense given the other things I wrote here? > I also really doubt this is ever a useful bit - what are the situations where a widget is loaded and *never* has any sort of events fired at it? This is the misunderstanding: the embedder needs to take an action (making password element values accessible to JavaScript) as soon as the first user interaction with the view occurs. It does not matter whether it occurs, as much as when. Cheers, Vaclav https://codereview.chromium.org/163843002/diff/710001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/163843002/diff/710001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:183: void SimulateClick(); On 2014/02/18 20:19:56, jww wrote: > There's already a SimulateElementClick() function that's used in these > browser_tests. You might consider reusing that rather than rolling your own > here. No super important, though, and this has the advantage of not being tied > to a particular element. The SimulateElementClick is a method of RenderViewTest (and it's children). The PasswordAutofillAgentTest is a RenderViewTest, but PasswordManagerBrowserTest is not. That's why I had to write it myself here. https://codereview.chromium.org/163843002/diff/710001/chrome/test/data/passwo... File chrome/test/data/password/form_and_link.html (left): https://codereview.chromium.org/163843002/diff/710001/chrome/test/data/passwo... chrome/test/data/password/form_and_link.html:4: <iframe src="password_form.html" id="first_frame" name="first_frame"> On 2014/02/18 20:19:56, jww wrote: > I'm pretty confused about why you're deleting these frames. It seems like you're > renaming multi_frames.html, in which case these frames are still being used by > several browser tests. Am I misunderstanding what's going on here? I'm actually not touching multi_frames.html at all (you would see it marked as deleted here), git just knows that I created form_and_link.html as a copy of multi_frames.html, and then edited it. I could try to force git into not thinking so, on the other hand git is precisely right about how I created the file, so I'm not sure if this is a problem. https://codereview.chromium.org/163843002/diff/710001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/163843002/diff/710001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:573: gatekeeper_.Reset(); On 2014/02/18 20:19:56, jww wrote: > Perhaps it's too late to ask this question anyway, but in the old WebUserGesture > system, couldn't we have just allocated a new handler at this point and fixed > the problem? > > That is, without this new refactor, if on every navigation we created a new > gesture_handler_, and set the old one to be ref counted, couldn't we have solved > this issue without the new implementation? > > There's probably good reason to do the refactor anyway, so feel free to blow > this question off if you think it's not worth thinking about. Joel, I don't think what you suggest would help. Consider the following scenario: 1. User loads a.com in tab 1. 2. User middle-clicks a link to b.com, which opens in tab 2 in background. 3. User re-loads tab 1. Now, remember that tab 1 and tab 2 shared the handler. After reloading in step 3, the handler set by tab 2 would be replaced by the new handler from tab 1. After that, tab 2 would never get notified about user interaction any more. And because there was no user interaction with tab 2 yet, no password fields would become accessible to JavaScript in tab 2.
On 2014/02/19 07:56:39, vabr (Chromium) wrote: > My bad naming probably caused most of the misunderstanding. I'll try to think > about better names, and update the CL. Naming updated in patch set 10.
The security benefits of this seem extremely dubious to me. Did this pass any sort of review? If you want to stick with these semantics, call the notification WillProcessUserGesture() to be consistent with the rest of the interface. https://codereview.chromium.org/163843002/diff/1110001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/163843002/diff/1110001/content/renderer/rende... content/renderer/render_widget.cc:1091: if (WebInputEvent::isUserGestureEventType(input_event->type)) TouchStart is considered a user gesture, so if a user touches their devices screen in any way during load this will fire and autofill will kick in. I really don't see how this provides any useful indication of user intent. Have you never tried to scroll a page before it's fully loaded?
On 2014/02/19 20:00:06, jamesr wrote: > The security benefits of this seem extremely dubious to me. Did this pass any > sort of review? > > If you want to stick with these semantics, call the notification > WillProcessUserGesture() to be consistent with the rest of the interface. > > https://codereview.chromium.org/163843002/diff/1110001/content/renderer/rende... > File content/renderer/render_widget.cc (right): > > https://codereview.chromium.org/163843002/diff/1110001/content/renderer/rende... > content/renderer/render_widget.cc:1091: if > (WebInputEvent::isUserGestureEventType(input_event->type)) > TouchStart is considered a user gesture, so if a user touches their devices > screen in any way during load this will fire and autofill will kick in. I > really don't see how this provides any useful indication of user intent. Have > you never tried to scroll a page before it's fully loaded? Yes, I'm from the Chrome security team. This is a very specific security threat, namely page redirects. We want to help prevent attacks where an attacker has an XSS on a bunch of pages, and redirects through all of them, along the way collecting autofilled credentials. We have a similar mitigation for the related iframe version of the attack.
On 2014/02/19 21:58:52, jww wrote: > On 2014/02/19 20:00:06, jamesr wrote: > > The security benefits of this seem extremely dubious to me. Did this pass any > > sort of review? > > > > If you want to stick with these semantics, call the notification > > WillProcessUserGesture() to be consistent with the rest of the interface. > > > > > https://codereview.chromium.org/163843002/diff/1110001/content/renderer/rende... > > File content/renderer/render_widget.cc (right): > > > > > https://codereview.chromium.org/163843002/diff/1110001/content/renderer/rende... > > content/renderer/render_widget.cc:1091: if > > (WebInputEvent::isUserGestureEventType(input_event->type)) > > TouchStart is considered a user gesture, so if a user touches their devices > > screen in any way during load this will fire and autofill will kick in. I > > really don't see how this provides any useful indication of user intent. Have > > you never tried to scroll a page before it's fully loaded? > > Yes, I'm from the Chrome security team. This is a very specific security threat, > namely page redirects. We want to help prevent attacks where an attacker has an > XSS on a bunch of pages, and redirects through all of them, along the way > collecting autofilled credentials. We have a similar mitigation for the related > iframe version of the attack. All an attacker would have to do to work around this mitigation is have the user touch the page *in any way* during the redirects. That seems like a pretty easy thing to do, but if you say it's still worthwhile then OK.
lgtm https://codereview.chromium.org/163843002/diff/1110001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/163843002/diff/1110001/content/renderer/rende... content/renderer/render_widget.cc:1091: if (WebInputEvent::isUserGestureEventType(input_event->type)) On 2014/02/19 20:00:07, jamesr wrote: > TouchStart is considered a user gesture, so if a user touches their devices > screen in any way during load this will fire and autofill will kick in. I > really don't see how this provides any useful indication of user intent. Have > you never tried to scroll a page before it's fully loaded? As mentioned in the comments, this is meant to be a mitigation for a very, very specific use case, namely rapid redirects of sites with an XSS to collect autofilled credentials. The user intent is meant to be as simple as "I am interacting with this navigation." Now, if an attacker wants to attack a user more precisely, this is certainly not a good mitigation. Ultimately, if the attacker has an XSS to a site that the user uses, the attacker will be able to obtain the credentials. It's more widespread harvesting that we're concerned about here.
On 2014/02/19 22:04:48, jww wrote: > lgtm > > https://codereview.chromium.org/163843002/diff/1110001/content/renderer/rende... > File content/renderer/render_widget.cc (right): > > https://codereview.chromium.org/163843002/diff/1110001/content/renderer/rende... > content/renderer/render_widget.cc:1091: if > (WebInputEvent::isUserGestureEventType(input_event->type)) > On 2014/02/19 20:00:07, jamesr wrote: > > TouchStart is considered a user gesture, so if a user touches their devices > > screen in any way during load this will fire and autofill will kick in. I > > really don't see how this provides any useful indication of user intent. Have > > you never tried to scroll a page before it's fully loaded? > As mentioned in the comments, this is meant to be a mitigation for a very, very > specific use case, namely rapid redirects of sites with an XSS to collect > autofilled credentials. The user intent is meant to be as simple as "I am > interacting with this navigation." > > Now, if an attacker wants to attack a user more precisely, this is certainly not > a good mitigation. Ultimately, if the attacker has an XSS to a site that the > user uses, the attacker will be able to obtain the credentials. It's more > widespread harvesting that we're concerned about here. jamesr: I should also mention, if you'd like a more thorough discussion of this topic, please see the original bug: https://crbug.com/163072.
https://codereview.chromium.org/163843002/diff/1110001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/163843002/diff/1110001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_browsertest.cc:208: } Drive-by nit: Can you use SimulateMouseClick() from //content/public/test/browser_test_utils.h rather than implementing your own version here?
Hi all, Ilya's comment and James renaming suggestion addressed. James, Do you have any remaining concerns, following the answer from Joel about security threat this is targeting? Thanks to everybody for your time spent on this CL. Vaclav https://codereview.chromium.org/163843002/diff/1110001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/163843002/diff/1110001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_browsertest.cc:208: } On 2014/02/19 23:54:57, Ilya Sherman wrote: > Drive-by nit: Can you use SimulateMouseClick() from > //content/public/test/browser_test_utils.h rather than implementing your own > version here? Done, thanks for making me aware of SimulateMouseClick!
I don't think the analysis on the bug takes into account the fact that on a mobile device *any* interaction with the screen of any sort will be considered a "user gesture" by this code, including scrolling or pinching or just wiping a piece of lint off the screen. That said, this patch doesn't appear to make that behavior any worse so lgtm https://codereview.chromium.org/163843002/diff/870002/content/renderer/render... File content/renderer/render_widget.h (right): https://codereview.chromium.org/163843002/diff/870002/content/renderer/render... content/renderer/render_widget.h:504: // event is being processed. s/is being processed/will be processed/
Jói, Could you please have a look at content/public/renderer/render_view_observer.h ? James: Thanks! I understand what you say about the user interaction on mobile devices. My understanding of Joel's argument was that this feature targets the situation where a password value is extracted during a rapid series of page redirects, during which even user's touch is hardly to be expected. Joel, could you confirm / clarify that? If the above is wrong, I could also exclude TouchStart from the accepted events (at the cost of making the code harder to understand). Thanks all, Vaclav https://codereview.chromium.org/163843002/diff/870002/content/renderer/render... File content/renderer/render_widget.h (right): https://codereview.chromium.org/163843002/diff/870002/content/renderer/render... content/renderer/render_widget.h:504: // event is being processed. On 2014/02/20 16:32:38, jamesr wrote: > s/is being processed/will be processed/ Done.
//content/public LGTM.
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/163843002/1390001
Message was sent while issue was closed.
Change committed as 252352 |
