Bug 37008 – ScriptController::processingUserGestureEvent returns true for simulated clicks

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Andy Estes]

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.

[Andy Estes]

<rdar://problem/7783498>

[Andy Estes]

Created attachment 52385 [details]
patch

[Early Warning System Bot]

Attachment 52385 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1624189

[WebKit Review Bot]

Attachment 52385 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1609178

[Andy Estes]

Created attachment 52388 [details]
patch (v2)

Forgot to add UserGestureState.{h, cpp} to all build systems.

[Eric Seidel (no email)]

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]

Wasn't this supposed to be fixed in bug 21501 AKA <rdar://problem/4599085>?

[Adam Barth]

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]

Those with Chromium security bug access can see an example here: http://code.google.com/p/chromium/issues/detail?id=34414

[Eric Carlson]

 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]

+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]

(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]

(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]

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]

> 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]

> 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]

(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]

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]

Attachment 52474 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1568187

[Adam Barth]

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]

Created attachment 52484 [details]
patch (v4)

[Adam Barth]

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]

(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]

> 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]

Committed r57045: <http://trac.webkit.org/changeset/57045>

[Eric Seidel (no email)]

Looks like this turn a Gtk bot red:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r57045%20(1733)/fast/events/popup-allowed-from-gesture-initiated-event-pretty-diff.html

[Inferno]

*** Bug 34541 has been marked as a duplicate of this bug. ***

[Abhishek Arya]

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]

(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]

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]

UserGestureIndicator.cpp has non-ASCII characters, quotations around "AS IS".
They cause a build error on Japanese Windows.

[Andy Estes]

(In reply to comment #26)
> Looks like this turn a Gtk bot red:
> http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r57045%20(1733)/fast/events/popup-allowed-from-gesture-initiated-event-pretty-diff.html

Looks like this was fixed in http://trac.webkit.org/changeset/57052.

[Andy Estes]

Created attachment 52652 [details]
Updated license headers

Updated the license headers in UserGestureIndicator.{h, cpp} to remove non-ASCII characers.

Comment on attachment 52652 [details]
Updated license headers

rs=me

Committed attachment 52652 [details] as r57159.

[Daniel Bates]

Comment on attachment 52652 [details]
Updated license headers

Clearing commit-queue flag to get this out of the commit-queue.