Bug 1902690 - More fully disable authenticode checks when using DISABLE_UPDATER_AUTHENTICODE_CHECK r=nalexander,application-update-reviewers

This should have no effect on any production code since no sane production configuration would turn this on. It is only for testing.

Differential Revision: https://phabricator.services.mozilla.com/D214212

toolkit/components/maintenanceservice/workmonitor.cpp

@@ -394,11 +394,7 @@ static bool UpdaterIsValid(LPWSTR updater, LPWSTR installDir,
     return false;
   }
 
-#ifndef DISABLE_UPDATER_AUTHENTICODE_CHECK
   return DoesBinaryMatchAllowedCertificates(installDir, updater);
-#else
-  return true;
-#endif
 }
 
 /**

toolkit/mozapps/update/common/moz.build

@@ -32,6 +32,9 @@ Library("updatecommon")
 DEFINES["NS_NO_XPCOM"] = True
 USE_STATIC_LIBS = True
 
+if CONFIG["DISABLE_UPDATER_AUTHENTICODE_CHECK"]:
+    DEFINES["DISABLE_UPDATER_AUTHENTICODE_CHECK"] = True
+
 if CONFIG["OS_ARCH"] == "WINNT":
     # This forces the creation of updatecommon.lib, which the update agent needs
     # in order to link to updatecommon library functions.

toolkit/mozapps/update/common/registrycertificates.cpp

@@ -15,18 +15,38 @@
 /**
  * Verifies if the file path matches any certificate stored in the registry.
  *
- * @param  filePath The file path of the application to check if allowed.
+ * @param  filePath
- * @param  allowFallbackKeySkip when this is TRUE the fallback registry key will
+ *         The file path of the application to check if allowed.
- *   be used to skip the certificate check.  This is the default since the
+ * @param  allowFallbackKeySkip
- *   fallback registry key is located under HKEY_LOCAL_MACHINE which can't be
+ *         When this is TRUE the fallback registry key can be used to skip the
- *   written to by a low integrity process.
+ *         certificate check.  This is the default since the fallback registry
- *   Note: the maintenance service binary can be used to perform this check for
+ *         key is located under HKEY_LOCAL_MACHINE which can't be written to by
- *   testing or troubleshooting.
+ *         a low integrity process.
+ *         Note: The maintenance service binary can be used to perform this
+ *               check for testing or troubleshooting.
+ *         Note: When this is `TRUE` and we are building with
+ *               `DISABLE_UPDATER_AUTHENTICODE_CHECK`, this function will
+ *               unconditionally return `TRUE` since that flag is meant to
+ *               disable specifically this. We don't fall through in the `FALSE`
+ *               case since currently the only time when we don't allow the
+ *               fallback key is when we are running this for debugging purposes
+ *               and, in that case, it's more helpful if we return something
+ *               meaningful here.
+ *
  * @return TRUE if the binary matches any of the allowed certificates.
  */
 BOOL DoesBinaryMatchAllowedCertificates(LPCWSTR basePathForUpdate,
                                         LPCWSTR filePath,
                                         BOOL allowFallbackKeySkip) {
+#ifdef DISABLE_UPDATER_AUTHENTICODE_CHECK
+  if (allowFallbackKeySkip) {
+    LOG_WARN(("Skipping authenticode check"));
+    return TRUE;
+  } else {
+    LOG(("Performing a diagnostic authenticode check"));
+  }
+#endif
+
   WCHAR maintenanceServiceKey[MAX_PATH + 1];
   if (!CalculateRegistryPathFromFilePath(basePathForUpdate,
                                          maintenanceServiceKey)) {