RESOLVED FIXED 37008
ScriptController::processingUserGestureEvent returns true for simulated clicks
https://bugs.webkit.org/show_bug.cgi?id=37008
Summary ScriptController::processingUserGestureEvent returns true for simulated clicks
Andy Estes
Reported 2010-04-01 21:52:55 PDT
Created attachment 52373 [details] test case FrameLoader::isProcessingUserGesture is used by the various parts of WebCore to prevent scripts from doing things without input from a user, for example open a pop-up window. This function calls ScriptController::processingUserGestureEvent which only considers the type of the current event, not whether or not it was actually generated by a user gesture. It is possible to programatically call click() on an HTML button element to open a popup window without an underlying user gesture, as generated by the attached test case.
Attachments
test case (631 bytes, text/html)
2010-04-01 21:52 PDT, Andy Estes
no flags
patch (25.35 KB, patch)
2010-04-01 23:42 PDT, Andy Estes
no flags
patch (v2) (27.94 KB, patch)
2010-04-02 00:11 PDT, Andy Estes
abarth: review-
patch (v3) (25.13 KB, patch)
2010-04-02 19:54 PDT, Andy Estes
abarth: review-
abarth: commit-queue-
patch (v4) (26.08 KB, patch)
2010-04-02 23:21 PDT, Andy Estes
abarth: review+
abarth: commit-queue-
Updated license headers (6.34 KB, patch)
2010-04-06 11:05 PDT, Andy Estes
mitz: review+
Andy Estes
Comment 1 2010-04-01 21:53:29 PDT
Andy Estes
Comment 2 2010-04-01 23:42:52 PDT
Early Warning System Bot
Comment 3 2010-04-01 23:57:36 PDT
WebKit Review Bot
Comment 4 2010-04-02 00:04:33 PDT
Andy Estes
Comment 5 2010-04-02 00:11:42 PDT
Created attachment 52388 [details] patch (v2) Forgot to add UserGestureState.{h, cpp} to all build systems.
Eric Seidel (no email)
Comment 6 2010-04-02 00:17:33 PDT
Comment on attachment 52388 [details] patch (v2) WTFNonHeapAllocatable seems like a totally separate patch for a totally separate bug. I think this would be much easier to review if you split them.
Alexey Proskuryakov
Comment 7 2010-04-02 08:20:53 PDT
Wasn't this supposed to be fixed in bug 21501 AKA <rdar://problem/4599085>?
Adam Barth
Comment 8 2010-04-02 08:31:21 PDT
Comment on attachment 52388 [details] patch (v2) This is great. I've wanted to do this for a while but hadn't gotten around to it. Some nits: + createdByDOM I think you can remove this concept now. + NonHeapAllocatable.h You've add this header to the xcodeproj but not to the other build systems. Please add it to all the build systems (except the "pro", which doesn't include headers). As for whether this is fixed, we have more popup blocker bypasses that this either will fix or give us the tools to fix.
Adam Barth
Comment 9 2010-04-02 08:33:31 PDT
Those with Chromium security bug access can see an example here: http://code.google.com/p/chromium/issues/detail?id=34414
Eric Carlson
Comment 10 2010-04-02 09:40:06 PDT
25 * dom/UserGestureState.cpp: Added. This class follows the RAAI pattern 26 to manage a global flag indicating that a user gesture is being 27 processed. Don't you mean "follows the RAII pattern" (Resource Acquisition Is Initialization)?
Geoffrey Garen
Comment 11 2010-04-02 10:12:40 PDT
+namespace WTFNonHeapAllocatable { You should include the same comment that Noncopyable has to explain why it gets its own namespace. +class NonHeapAllocatable { What a mouthful! :) I guess I don't have a better idea, though :(. + void* operator new(size_t) + { + ASSERT_NOT_REACHED(); + return reinterpret_cast<void*>(0xbadbeef); + } + + void* operator new[](size_t) + { + ASSERT_NOT_REACHED(); + return reinterpret_cast<void*>(0xbadbeef); + } It's better just to declare these functions, without defining them. That way, if you somehow tricked a compiler into issuing a call to them, linking would still fail. Also, it's more concise. You might as well throw in the delete operators for good measure. bool Event::fromUserGesture() { - if (createdByDOM()) + if (!UserGestureState::isProcessingUserGesture()) This will work, but it leaves the code in a bit of a confusing state. A good future cleanup would be for UserGestureState to keep a HashCountedSet of frames that are processing user gestures. Then, instead of asking an event whether it thought it was a user gesture, you could ask UserGestureState whether a given frame was processing a user gesture. That would eliminate the need for things like currentEvent, too. + static inline bool isProcessingUserGesture() { return processingUserGesture; } It's unfortunate when one thing has two names. Here, the accessor name gets the word "is," but the variable name doesn't -- I assume to avoid name conflict. One of the nice benefits of prefixing variable names is that it solves such conflicts. I would recommend renaming processingUserGesture to s_ isProcessingUserGesture. + UserGestureState gestureIndicator; This is another case of one thing having two names. (Is it a "user gesture" or a "gesture?" Is it a "state" or an "indicator?") I'd recommend picking one name and using it everywhere. I like "user gesture indicator" or "user gesture record" best, since it's more specific about both the gesture and the state. (Technically, "state" could be anything, including "nothing's happening." So creating an empty "state" doesn't say as much.)
Andy Estes
Comment 12 2010-04-02 11:02:56 PDT
(In reply to comment #10) > 25 * dom/UserGestureState.cpp: Added. This class follows the RAAI > pattern > 26 to manage a global flag indicating that a user gesture is being > 27 processed. > Don't you mean "follows the RAII pattern" (Resource Acquisition Is > Initialization)? Right you are (I always forget that it is "is" not "as").
Andy Estes
Comment 13 2010-04-02 14:58:02 PDT
(In reply to comment #6) > (From update of attachment 52388 [details]) > WTFNonHeapAllocatable seems like a totally separate patch for a totally > separate bug. I think this would be much easier to review if you split them. Good point, Eric. See https://bugs.webkit.org/show_bug.cgi?id=37046. Once that patch lands, I'll re-post a patch to this bug that addresses Adam's and Geoff's comments.
Adam Barth
Comment 14 2010-04-02 15:12:19 PDT
Not to nit-pick, but it would be awesome to re-spin this as a generic RAII boolean class via templates. The user gesture could then be a typedefed instantiation of the template. :)
Geoffrey Garen
Comment 15 2010-04-02 16:09:40 PDT
> Not to nit-pick, but it would be awesome to re-spin this as a generic RAII > boolean class via templates. The user gesture could then be a typedefed > instantiation of the template. :) Urgh. You could do that. But what's the benefit?
Adam Barth
Comment 16 2010-04-02 16:12:20 PDT
> Urgh. You could do that. But what's the benefit? The benefit is that we can write the next RAII boolean in one line of code instead of 78. The point is mostly that UserGestureState.* has nothing to do with user gestures and can profitably be abstracted into WTF.
Maciej Stachowiak
Comment 17 2010-04-02 16:27:54 PDT
(In reply to comment #16) > > Urgh. You could do that. But what's the benefit? > > The benefit is that we can write the next RAII boolean in one line of code > instead of 78. The point is mostly that UserGestureState.* has nothing to do > with user gestures and can profitably be abstracted into WTF. Sounds like a reasonable idea, but I think the extra abstraction can be separate from the bugfix.
Andy Estes
Comment 18 2010-04-02 19:54:38 PDT
Created attachment 52474 [details] patch (v3) Based on discussions with Maciej, I decided that since some common heap allocation scenarios could not be prevented using NonHeapAllocatable, it wasn't worth indicating and giving a false sense of security. This patch removes NonHeapAllocatable and also addresses other comments from Adam and Geoff.
WebKit Review Bot
Comment 19 2010-04-02 20:20:18 PDT
Adam Barth
Comment 20 2010-04-02 20:43:49 PDT
Comment on attachment 52474 [details] patch (v3) + namespace WebCore { We usually have a blank line under the namespace declaration. + static inline bool processingUserGesture() { return s_processingUserGesture; } No need to declare inline here. + #include <new> Why do we need this here? Also, please fix the Chromium build failure. If you were a committer, I would r+ your patch and ask you to fix these things on landing, but as a non-committer, I'm going to ask you to upload a new patch so we can land it using the bot. Thanks for the patch.
Andy Estes
Comment 21 2010-04-02 23:21:39 PDT
Created attachment 52484 [details] patch (v4)
Adam Barth
Comment 22 2010-04-02 23:26:57 PDT
Comment on attachment 52484 [details] patch (v4) + public: ^^^ shouldn't be indented. +} // namespace WebCore ^^^ blank line before this line. r=me I'm happy to land this for you (with these style changes) if you're tired of re-spinning the patch. I'm leaving the r? for a bit so that all the EWS bots run on your patch.
Andy Estes
Comment 23 2010-04-02 23:35:26 PDT
(In reply to comment #22) > (From update of attachment 52484 [details]) > + public: > > ^^^ shouldn't be indented. > > +} // namespace WebCore > > ^^^ blank line before this line. > > r=me > > I'm happy to land this for you (with these style changes) if you're tired of > re-spinning the patch. I'm leaving the r? for a bit so that all the EWS bots > run on your patch. Sorry about the style bugs :( If all the EWS bots go green, feel free to land the patch. Thanks!
Adam Barth
Comment 24 2010-04-02 23:43:40 PDT
> Sorry about the style bugs :( No problem. It's too bad the style bot can't catch more of these automatically. > If all the EWS bots go green, feel free to land > the patch. Thanks! Looks like we're good to go. I'll land this now.
Adam Barth
Comment 25 2010-04-03 00:06:17 PDT
Inferno
Comment 27 2010-04-05 10:15:50 PDT
*** Bug 34541 has been marked as a duplicate of this bug. ***
Abhishek Arya
Comment 28 2010-04-05 10:55:19 PDT
The patch does not look to fix the popup blocker bypass here - http://www.securethoughts.com/security/popupbypass/popup.html. any idea why.
Adam Barth
Comment 29 2010-04-05 10:59:37 PDT
(In reply to comment #28) > The patch does not look to fix the popup blocker bypass here - > http://www.securethoughts.com/security/popupbypass/popup.html. any idea why. There are lots of popup blocker bypasses. This patch adds a tool we can use to fix them. You'll probably have to debug the issue. I bet we're missing a processingUserGesture check somewhere in this code path.
Chris Evans
Comment 30 2010-04-05 15:10:31 PDT
I'm just fixing a Chromium-specific bug whereby block preferences don't necessarily get considered properly. I'll retest all the window.open() bypasses once I have this change and my change together.
Kent Tamura
Comment 31 2010-04-06 00:36:15 PDT
UserGestureIndicator.cpp has non-ASCII characters, quotations around "AS IS". They cause a build error on Japanese Windows.
Andy Estes
Comment 33 2010-04-06 11:05:19 PDT
Created attachment 52652 [details] Updated license headers Updated the license headers in UserGestureIndicator.{h, cpp} to remove non-ASCII characers.
mitz
Comment 34 2010-04-06 11:11:40 PDT
Comment on attachment 52652 [details] Updated license headers rs=me
mitz
Comment 35 2010-04-06 11:18:31 PDT
Daniel Bates
Comment 36 2010-07-23 22:10:59 PDT
Comment on attachment 52652 [details] Updated license headers Clearing commit-queue flag to get this out of the commit-queue.
Note You need to log in before you can comment on or make changes to this bug.