Bug 1087674 - Handle XHR abort()/timeout and certificate errors more gracefully in GMPInstallmanager. r=gfritzsche, a=sledru
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Thu, 30 Oct 2014 11:39:44 +0100
changeset 218155 0de6e2b5507a
parent 218154 e0486ab5ac2c
child 218156 08e3c773c747
child 218158 2c6590150a85
push id557
push userryanvm@gmail.com
push date2014-10-30 13:09 +0000
treeherdermozilla-release@0de6e2b5507a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgfritzsche, sledru
bugs1087674
milestone33.1
Bug 1087674 - Handle XHR abort()/timeout and certificate errors more gracefully in GMPInstallmanager. r=gfritzsche, a=sledru Prior to this patch, a Man in the Middle (MITM) attack on SSL could cause GMPInstallManager to fail during the check for updates, which in turn would cause a crash during shutdown. This was observed in the wild by users of recent versions of Avast, which performs such attacks on SSL as part of its "HTTPS scanning" feature. With this patch, errors are handled more gracefully. The attack still prevents any update (including the install of OpenH264) but at least it does not cause a crash anymore.
toolkit/modules/GMPInstallManager.jsm
toolkit/modules/tests/xpcshell/test_GMPInstallManager.js
--- a/toolkit/modules/GMPInstallManager.jsm
+++ b/toolkit/modules/GMPInstallManager.jsm
@@ -44,16 +44,26 @@ XPCOMUtils.defineLazyGetter(this, "gCert
   let temp = { };
   Cu.import("resource://gre/modules/CertUtils.jsm", temp);
   return temp;
 });
 
 XPCOMUtils.defineLazyModuleGetter(this, "UpdateChannel",
                                   "resource://gre/modules/UpdateChannel.jsm");
 
+/**
+ * Number of milliseconds after which we need to cancel `checkForAddons`.
+ *
+ * Bug 1087674 suggests that the XHR we use in `checkForAddons` may
+ * never terminate in presence of network nuisances (e.g. strange
+ * antivirus behavior). This timeout is a defensive measure to ensure
+ * that we fail cleanly in such case.
+ */
+const CHECK_FOR_ADDONS_TIMEOUT_DELAY_MS = 20000;
+
 // Used to determine if logging should be enabled
 XPCOMUtils.defineLazyGetter(this, "gLogEnabled", function() {
   return GMPPrefs.get(GMPPrefs.KEY_LOG_ENABLED);
 });
 
 
 function getScopedLogger(prefix) {
   var parentScope = gLogEnabled ? PARENT_LOGGER_ID + "." : "";
@@ -364,18 +374,21 @@ GMPInstallManager.prototype = {
     // The Cache-Control header is only interpreted by proxies and the
     // final destination. It does not help if a resource is already
     // cached locally.
     this._request.setRequestHeader("Cache-Control", "no-cache");
     // HTTP/1.0 servers might not implement Cache-Control and
     // might only implement Pragma: no-cache
     this._request.setRequestHeader("Pragma", "no-cache");
 
-    this._request.addEventListener("error", this.onErrorXML.bind(this) ,false);
-    this._request.addEventListener("load", this.onLoadXML.bind(this), false);
+    this._request.timeout = CHECK_FOR_ADDONS_TIMEOUT_DELAY_MS;
+    this._request.addEventListener("error", event => this.onFailXML("onErrorXML", event), false);
+    this._request.addEventListener("abort", event => this.onFailXML("onAbortXML", event), false);
+    this._request.addEventListener("timeout", event => this.onFailXML("onTimeoutXML", event), false);
+    this._request.addEventListener("load", event => this.onLoadXML(event), false);
 
     log.info("sending request to: " + url);
     this._request.send(null);
 
     return this._deferred.promise;
   },
   /**
    * Installs the specified addon and calls a callback when done.
@@ -501,60 +514,70 @@ GMPInstallManager.prototype = {
   overrideLeaveDownloadedZip: false,
 
   /**
    * The XMLHttpRequest succeeded and the document was loaded.
    * @param event The nsIDOMEvent for the load
   */
   onLoadXML: function(event) {
     let log = getScopedLogger("GMPInstallManager.onLoadXML");
-    log.info("request completed downloading document");
+    try {
+      log.info("request completed downloading document");
+
+      let certs = null;
+      if (!Services.prefs.prefHasUserValue(GMPPrefs.KEY_URL_OVERRIDE) &&
+          GMPPrefs.get(GMPPrefs.KEY_CERT_CHECKATTRS, undefined, true)) {
+        certs = gCertUtils.readCertPrefs(GMPPrefs.CERTS_BRANCH);
+      }
 
-    let certs = null;
-    if (!Services.prefs.prefHasUserValue(GMPPrefs.KEY_URL_OVERRIDE) &&
-        GMPPrefs.get(GMPPrefs.KEY_CERT_CHECKATTRS, undefined, true)) {
-      certs = gCertUtils.readCertPrefs(GMPPrefs.CERTS_BRANCH);
+      let allowNonBuiltIn = !GMPPrefs.get(GMPPrefs.KEY_CERT_REQUIREBUILTIN,
+                                          undefined, true);
+      log.info("allowNonBuiltIn: " + allowNonBuiltIn);
+      gCertUtils.checkCert(this._request.channel, allowNonBuiltIn, certs);
+
+      this.parseResponseXML();
+    } catch (ex) {
+      log.error("could not load xml: " + ex);
+      this._deferred.reject({
+        target: event.target,
+        status: this._getChannelStatus(event.target),
+        message: "" + ex,
+      });
+      delete this._deferred;
     }
-
-    let allowNonBuiltIn = !GMPPrefs.get(GMPPrefs.KEY_CERT_REQUIREBUILTIN,
-                                        undefined, true);
-    log.info("allowNonBuiltIn: " + allowNonBuiltIn);
-    gCertUtils.checkCert(this._request.channel, allowNonBuiltIn, certs);
-
-    this.parseResponseXML();
   },
 
   /**
    * Returns the status code for the XMLHttpRequest
    */
   _getChannelStatus: function(request) {
     let log = getScopedLogger("GMPInstallManager._getChannelStatus");
-    let status = 0;
+    let status = null;
     try {
       status = request.status;
       log.info("request.status is: " + request.status);
     }
     catch (e) {
     }
 
-    if (status == 0) {
+    if (status == null) {
       status = request.channel.QueryInterface(Ci.nsIRequest).status;
     }
     return status;
   },
 
   /**
    * There was an error of some kind during the XMLHttpRequest
    * @param event The nsIDOMEvent for the error
   */
-  onErrorXML: function(event) {
-    let log = getScopedLogger("GMPInstallManager.onErrorXML");
+  onFailXML: function(failure, event) {
+    let log = getScopedLogger("GMPInstallManager." + failure);
     let request = event.target;
     let status = this._getChannelStatus(request);
-    let message = "request.status: " + status;
+    let message = "request.status: " + status +  " (" + event.type + ")";
     log.warn(message);
     this._deferred.reject({
       target: request,
       status: status,
       message: message
     });
     delete this._deferred;
   },
--- a/toolkit/modules/tests/xpcshell/test_GMPInstallManager.js
+++ b/toolkit/modules/tests/xpcshell/test_GMPInstallManager.js
@@ -139,16 +139,85 @@ add_test(function test_checkForAddons_40
   }, function(err) {
     do_check_true(!!err);
     do_check_eq(err.status, 404);
     installManager.uninit();
     run_next_test();
   });
 });
 
+/**
+ * Tests that a xhr abort() works as expected
+ */
+add_test(function test_checkForAddons_abort() {
+  let xhr = overrideXHR(200, "", { dropRequest: true} );
+  let installManager = new GMPInstallManager();
+  let promise = installManager.checkForAddons();
+  xhr.abort();
+  promise.then(function() {
+    do_throw("abort() should reject");
+  }, function(err) {
+    do_check_eq(err.status, 0);
+    installManager.uninit();
+    run_next_test();
+  });
+});
+
+/**
+ * Tests that a defensive timeout works as expected
+ */
+add_test(function test_checkForAddons_timeout() {
+  overrideXHR(200, "", { dropRequest: true, timeout: true });
+  let installManager = new GMPInstallManager();
+  let promise = installManager.checkForAddons();
+  promise.then(function() {
+    do_throw("Defensive timeout should reject");
+  }, function(err) {
+    do_check_eq(err.status, 0);
+    installManager.uninit();
+    run_next_test();
+  });
+});
+
+/**
+ * Tests that we throw correctly in case of ssl certification error.
+ */
+add_test(function test_checkForAddons_bad_ssl() {
+  //
+  // Add random stuff that cause CertUtil to require https.
+  //
+  let PREF_KEY_URL_OVERRIDE_BACKUP =
+    Preferences.get(GMPPrefs.KEY_URL_OVERRIDE, undefined);
+  Preferences.reset(GMPPrefs.KEY_URL_OVERRIDE);
+
+  let CERTS_BRANCH_DOT_ONE = GMPPrefs.CERTS_BRANCH + ".1";
+  let PREF_CERTS_BRANCH_DOT_ONE_BACKUP =
+    Preferences.get(CERTS_BRANCH_DOT_ONE, undefined);
+  Services.prefs.setCharPref(CERTS_BRANCH_DOT_ONE, "funky value");
+
+
+  overrideXHR(200, "");
+  let installManager = new GMPInstallManager();
+  let promise = installManager.checkForAddons();
+  promise.then(function() {
+    do_throw("Defensive timeout should reject");
+  }, function(err) {
+    do_check_true(err.message.contains("SSL is required and URI scheme is not https."));
+    installManager.uninit();
+    if (PREF_KEY_URL_OVERRIDE_BACKUP) {
+      Preferences.set(GMPPrefs.KEY_URL_OVERRIDE,
+        PREF_KEY_URL_OVERRIDE_BACKUP);
+    }
+    if (PREF_CERTS_BRANCH_DOT_ONE_BACKUP) {
+      Preferences.set(CERTS_BRANCH_DOT_ONE,
+        PREF_CERTS_BRANCH_DOT_ONE_BACKUP);
+    }
+    run_next_test();
+  });
+});
 
 /**
  * Tests that gettinga a funky non XML response works as expected
  */
 add_test(function test_checkForAddons_notXML() {
   overrideXHR(200, "3.141592653589793....");
   let installManager = new GMPInstallManager();
   let promise = installManager.checkForAddons();
@@ -595,65 +664,110 @@ function makeHandler(aVal) {
   if (typeof aVal == "function")
     return { handleEvent: aVal };
   return aVal;
 }
 /**
  * Constructs a mock xhr which is used for testing different aspects
  * of responses.
  */
-function xhr(inputStatus, inputResponse) {
+function xhr(inputStatus, inputResponse, options) {
   this.inputStatus = inputStatus;
   this.inputResponse = inputResponse;
+  this.status = 0;
+  this.responseXML = null;
+  this._aborted = false;
+  this._onabort = null;
+  this._onprogress = null;
+  this._onerror = null;
+  this._onload = null;
+  this._onloadend = null;
+  this._ontimeout = null;
+  this._url = null;
+  this._method = null;
+  this._timeout = 0;
+  this._notified = false;
+  this._options = options || {};
 }
 xhr.prototype = {
   overrideMimeType: function(aMimetype) { },
   setRequestHeader: function(aHeader, aValue) { },
   status: null,
   channel: { set notificationCallbacks(aVal) { } },
-  _url: null,
-  _method: null,
   open: function(aMethod, aUrl) {
     this.channel.originalURI = Services.io.newURI(aUrl, null, null);
     this._method = aMethod; this._url = aUrl;
   },
   abort: function() {
+    this._dropRequest = true;
+    this._notify(["abort", "loadend"]);
   },
   responseXML: null,
   responseText: null,
   send: function(aBody) {
-    let self = this;
     do_execute_soon(function() {
-      self.status = self.inputStatus;
-      self.responseText = self.inputResponse;
       try {
-        let parser = Cc["@mozilla.org/xmlextras/domparser;1"].
-                     createInstance(Ci.nsIDOMParser);
-        self.responseXML = parser.parseFromString(self.inputResponse,
-                                                  "application/xml");
-      } catch (e) {
-        self.responseXML = null;
+        if (this._options.dropRequest) {
+          if (this._timeout > 0 && this._options.timeout) {
+            this._notify(["timeout", "loadend"]);
+          }
+          return;
+        }
+        this.status = this.inputStatus;
+        this.responseText = this.inputResponse;
+        try {
+          let parser = Cc["@mozilla.org/xmlextras/domparser;1"].
+                createInstance(Ci.nsIDOMParser);
+          this.responseXML = parser.parseFromString(this.inputResponse,
+            "application/xml");
+        } catch (e) {
+          this.responseXML = null;
+        }
+        if (this.inputStatus === 200) {
+          this._notify(["load", "loadend"]);
+        } else {
+          this._notify(["error", "loadend"]);
+        }
+      } catch (ex) {
+        do_throw(ex);
       }
-      let e = { target: self };
-      if (self.inputStatus === 200) {
-        self.onload(e);
-      } else {
-        self.onerror(e);
-      }
-    });
+    }.bind(this));
   },
-  _onprogress: null,
+  set onabort(aValue) { this._onabort = makeHandler(aValue); },
+  get onabort() { return this._onabort; },
   set onprogress(aValue) { this._onprogress = makeHandler(aValue); },
   get onprogress() { return this._onprogress; },
-  _onerror: null,
   set onerror(aValue) { this._onerror = makeHandler(aValue); },
   get onerror() { return this._onerror; },
-  _onload: null,
   set onload(aValue) { this._onload = makeHandler(aValue); },
   get onload() { return this._onload; },
+  set onloadend(aValue) { this._onloadend = makeHandler(aValue); },
+  get onloadend() { return this._onloadend; },
+  set ontimeout(aValue) { this._ontimeout = makeHandler(aValue); },
+  get ontimeout() { return this._ontimeout; },
+  set timeout(aValue) { this._timeout = aValue; },
+  _notify: function(events) {
+    if (this._notified) {
+      return;
+    }
+    this._notified = true;
+    for (let item of events) {
+      let k = "on" + item;
+      if (this[k]) {
+        do_print("Notifying " + item);
+        let e = {
+          target: this,
+          type: item,
+        };
+        this[k](e);
+      } else {
+        do_print("Notifying " + item + ", but there are no listeners");
+      }
+    }
+  },
   addEventListener: function(aEvent, aValue, aCapturing) {
     eval("this._on" + aEvent + " = aValue");
   },
   flags: Ci.nsIClassInfo.SINGLETON,
   implementationLanguage: Ci.nsIProgrammingLanguage.JAVASCRIPT,
   getHelperForLanguage: function(aLanguage) null,
   getInterfaces: function(aCount) {
     let interfaces = [Ci.nsISupports];
@@ -678,26 +792,27 @@ xhr.prototype = {
 };
 
 /**
  * Helper used to overrideXHR requests (no matter to what URL) with the
  * specified status and response.
  * @param status The status you want to get back when an XHR request is made
  * @param response The response you want to get back when an XHR request is made
  */
-function overrideXHR(status, response) {
+function overrideXHR(status, response, options) {
   let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
   if (overrideXHR.myxhr) {
     registrar.unregisterFactory(overrideXHR.myxhr.classID, overrideXHR.myxhr);
   }
-  overrideXHR.myxhr = new xhr(status, response);
+  overrideXHR.myxhr = new xhr(status, response, options);
   registrar.registerFactory(overrideXHR.myxhr.classID,
                             overrideXHR.myxhr.classDescription,
                             overrideXHR.myxhr.contractID,
                             overrideXHR.myxhr);
+  return overrideXHR.myxhr;
 }
 
 /**
  * Compares binary data of 2 arrays and returns true if they are the same
  *
  * @param arr1 The first array to compare
  * @param arr2 The second array to compare
 */