Cordova-js: Event Listener Hijacking

[threatpointer]

Motivation and Context

This issue was originially reported to [email protected].

the core issue remains the prevention of code injection: we do not consider any post-injection attack techniques as vulnerabilities. The change you propose may possibly be interesting as a security hardening improvement. For those, you don't need to use the private security reporting mechanisms, but you can use the regular open contribution workflow - you can find more information about that at https://cordova.apache.org/contribute/

About the Issue

The current code modifies the default behavior of addEventListener and removeEventListener for both document and window objects to handle custom events. This modification can potentially be exploited if not properly secured.

Relevant Code

The manipulation of the document.addEventListener and window.addEventListener methods is crucial to the DOM event system. Overriding these methods allows directing certain events to custom documentEventHandlers and windowEventHandlers objects while allowing the default behavior for others. Malicious code (XSS) might modify these handler objects to include or replace handlers for specific events.

Description of the Fix

To mitigate such risks, we can encapsulate the modified addEventListener methods within an IIFE (Immediately Invoked Function Expression) to limit the scope and potential impact of the modifications. Additionally, checks are added to ensure that the subscribe function exists before calling it.

`
(function() {
    var originalDocumentAddEventListener = document.addEventListener;
    var originalWindowAddEventListener = window.addEventListener;
    var documentEventHandlers = {};
    var windowEventHandlers = {};

    document.addEventListener = function (evt, handler, capture) {
        var e = evt.toLowerCase();
        if (typeof documentEventHandlers[e] !== 'undefined') {
            if (typeof documentEventHandlers[e].subscribe === 'function') {
                documentEventHandlers[e].subscribe(handler);
            } else {
                console.warn('No subscribe function defined for event:', e);
            }
        } else {
            originalDocumentAddEventListener.call(document, evt, handler, capture);
        }
    };

    window.addEventListener = function (evt, handler, capture) {
        var e = evt.toLowerCase();
        if (typeof windowEventHandlers[e] !== 'undefined') {
            if (typeof windowEventHandlers[e].subscribe === 'function') {
                windowEventHandlers[e].subscribe(handler);
            } else {
                console.warn('No subscribe function defined for event:', e);
            }
        } else {
            originalWindowAddEventListener.call(window, evt, handler, capture);
        }
    };

    // Securely define your event handlers
    documentEventHandlers['click'] = {
        subscribe: function(handler) {
            var secureHandler = function(event) {
                // Perform necessary checks or actions before invoking the handler
                if (event && event.target) {
                    var allowedElements = ['button', 'a', 'div'];
                    if (allowedElements.includes(event.target.tagName.toLowerCase())) {
                        handler(event);
                    } else {
                        console.warn('Click event handler ignored for disallowed element:', event.target.tagName);
                    }
                } else {
                    console.warn('Invalid event object in secure handler.');
                }
            };
            originalDocumentAddEventListener.call(document, 'click', secureHandler, false);
        }
    };

    windowEventHandlers['resize'] = {
        subscribe: function(handler) {
            var secureHandler = function(event) {
                // Perform necessary checks or actions before invoking the handler
                handler(event);
            };
            originalWindowAddEventListener.call(window, 'resize', secureHandler, false);
        }
    };
})();



`

By implementing these practices, we can significantly reduce the risk of event listener hijacking. Having said that, if you beleive this should be left to the application to follow security best practices for input validation, CSP & loading of secure libraries of thrid party scripts and as a framework we shouldn't restrict such events. I am happy for you to close this PR with no action! This is more of defense in depth strategy to bake in security.

Testing

Limited testing performed, could use some help here.

[breautek]

The lint failures is blocking the actual unit tests from running, so once the lint issues are resolved there might be additional changes required.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 25.00000% with 27 lines in your changes missing coverage. Please review.

Project coverage is 79.81%. Comparing base (ab52fd7) to head (057b4fd).

Files	Patch %	Lines
src/cordova.js	25.00%	27 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
- Coverage   83.62%   79.81%   -3.81%     
==========================================
  Files          14       14              
  Lines         519      555      +36     
==========================================
+ Hits          434      443       +9     
- Misses         85      112      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[breautek]

As per the original report, you claimed these must not be globally accessible so they should be declared in the IIFE function, so that they will be out of scope of any external code.

Honestly I'm still not sure if this will be breaking or if there is existing cordova code that is external that depends on these global variables. I'll need to consult with other members who are more familiar with this part of the codebase.

[breautek]

After discussing this with a colleague we determined that the changes in this PR is high risk of breaking plugins and/or applications with low reward/value. As a result we will be declining this PR.

There's a couple of reasons why we consider this low reward/value and I'll list them.

1. The PR doesn't actually prevents "hijacking" of events.

The PR aims to isolate an object containing global event listeners because if an hypothetical malicious actor could replace the stored function with another. By removing this object from the global namespace, you remove the ability to replace the handler function.

The reality is, hypothetical malicious actors do not need this object. They could simply attach an click event (or any other event) on the document. They can go as far as use the Capture phase so that their event is triggered before most other events (which by default uses the bubble phase), escaping propagation prevention.

document.addEventListener('click', function(e) { ... }, true);

Is all that is needed to listen for click events on any target and inspect what is actually being clicked.

This is true even with standard browser APIs that isn't being overwritten.

2. standard window/document properties can simply be replaced

A hypothetical malicious actor who wants to hijack events can do so by replacing the cordova's implementation with their own. This is true even if Cordova doesn't overwrite the addEventListener itself.

The reality of webviews cannot load untrusted code. Any the attempts at hardening doesn't really do anything to deter or prevent a malicious actor in this case, hijack events. Once untrusted code is loaded, the app is at risk and at that point there is very little the application can do to "harden" itself. This is because the nature of the javascript engine is very dynamic and the runtime can be customized in the runtime on the fly. Webview environments aren't like other environments where you can minimize impact by implementing permission models or limiting access to resources, etc. The JS runtime can simply change anything it wants. As noted in the security email thread, there are best practices to follow to prevent unauthorized code from executing and if users follow them they are expected to be safe. For reference these guidelines can be found here

Edit:

I was also recently informed that the code in question is also its "prebuilt" stage. The source code in this repo gets wrapped in a function already in the final cordova.js that is provided to the platforms.

For example this code will be placed as follows:

// file: src/cordova.js
define("cordova", function(require, exports, module) {

// Workaround for Windows 10 in hosted environment case
// http://www.w3.org/html/wg/drafts/html/master/browsers.html#named-access-on-the-window-object
if (window.cordova && !(window.cordova instanceof HTMLElement)) {
    throw new Error('cordova already defined');
}

var channel = require('cordova/channel');
var platform = require('cordova/platform');

/**
 * Intercept calls to addEventListener + removeEventListener and handle deviceready,
 * resume, and pause events.
 */
var m_document_addEventListener = document.addEventListener;
var m_document_removeEventListener = document.removeEventListener;
var m_window_addEventListener = window.addEventListener;
var m_window_removeEventListener = window.removeEventListener;

/**
 * Houses custom event handlers to intercept on document + window event listeners.
 */
var documentEventHandlers = {};
var windowEventHandlers = {};

document.addEventListener = function (evt, handler, capture) {
    var e = evt.toLowerCase();
...

These variables are within a function block and thus aren't scoped in a global accessible area. The hardening steps to isolate these functions in an IIFE is actually already being done.