Bug 37008 - ScriptController::processingUserGestureEvent returns true for simulated clicks
Summary: ScriptController::processingUserGestureEvent returns true for simulated clicks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 37046
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-01 21:52 PDT by Andy Estes
Modified: 2010-07-23 22:10 PDT (History)
15 users (show)

See Also:


Attachments
test case (631 bytes, text/html)
2010-04-01 21:52 PDT, Andy Estes
no flags Details
patch (25.35 KB, patch)
2010-04-01 23:42 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
patch (v2) (27.94 KB, patch)
2010-04-02 00:11 PDT, Andy Estes
abarth: review-
Details | Formatted Diff | Diff
patch (v3) (25.13 KB, patch)
2010-04-02 19:54 PDT, Andy Estes
abarth: review-
abarth: commit-queue-
Details | Formatted Diff | Diff
patch (v4) (26.08 KB, patch)
2010-04-02 23:21 PDT, Andy Estes
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Updated license headers (6.34 KB, patch)
2010-04-06 11:05 PDT, Andy Estes
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 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.
Comment 1 Andy Estes 2010-04-01 21:53:29 PDT
<rdar://problem/7783498>
Comment 2 Andy Estes 2010-04-01 23:42:52 PDT
Created attachment 52385 [details]
patch
Comment 3 Early Warning System Bot 2010-04-01 23:57:36 PDT
Attachment 52385 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1624189
Comment 4 WebKit Review Bot 2010-04-02 00:04:33 PDT
Attachment 52385 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1609178
Comment 5 Andy Estes 2010-04-02 00:11:42 PDT
Created attachment 52388 [details]
patch (v2)

Forgot to add UserGestureState.{h, cpp} to all build systems.
Comment 6 Eric Seidel (no email) 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.
Comment 7 Alexey Proskuryakov 2010-04-02 08:20:53 PDT
Wasn't this supposed to be fixed in bug 21501 AKA <rdar://problem/4599085>?
Comment 8 Adam Barth 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.
Comment 9 Adam Barth 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
Comment 10 Eric Carlson 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)?
Comment 11 Geoffrey Garen 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.)
Comment 12 Andy Estes 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").
Comment 13 Andy Estes 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.
Comment 14 Adam Barth 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.  :)
Comment 15 Geoffrey Garen 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?
Comment 16 Adam Barth 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.
Comment 17 Maciej Stachowiak 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.
Comment 18 Andy Estes 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.
Comment 19 WebKit Review Bot 2010-04-02 20:20:18 PDT
Attachment 52474 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1568187
Comment 20 Adam Barth 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.
Comment 21 Andy Estes 2010-04-02 23:21:39 PDT
Created attachment 52484 [details]
patch (v4)
Comment 22 Adam Barth 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.
Comment 23 Andy Estes 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!
Comment 24 Adam Barth 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.
Comment 25 Adam Barth 2010-04-03 00:06:17 PDT
Committed r57045: <http://trac.webkit.org/changeset/57045>
Comment 27 Inferno 2010-04-05 10:15:50 PDT
*** Bug 34541 has been marked as a duplicate of this bug. ***
Comment 28 Abhishek Arya 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.
Comment 29 Adam Barth 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.
Comment 30 Chris Evans 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.
Comment 31 Kent Tamura 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.
Comment 33 Andy Estes 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.
Comment 34 mitz 2010-04-06 11:11:40 PDT
Comment on attachment 52652 [details]
Updated license headers

rs=me
Comment 35 mitz 2010-04-06 11:18:31 PDT
Committed attachment 52652 [details] as r57159.
Comment 36 Daniel Bates 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.