Merge pull request mozilla#83 from kmaglione/pref-checks

Preference branch checks for bug 676815

tests/resources/content/regex_error.js

@@ -1 +1 @@
-var foo = "network.http";
+var foo = "network.http.";

tests/test_js_prefs.py

@@ -0,0 +1,60 @@
+from js_helper import _do_test_raw
+
+def test_pref_innocuous_branch():
+    """
+    Tests that innocuous preferences created outside of the "extensions." branch
+    from defaults/preferences/*.js files throw warnings, and that ones created
+    in proper branches don't.
+    """
+
+    assert _do_test_raw("""
+    pref("foo.bar", true);
+    """, path="defaults/preferences/prefs.js").failed()
+
+    assert _do_test_raw("""
+    user_pref("foo.bar", true);
+    """, path="defaults/preferences/prefs.js").failed()
+
+    assert _do_test_raw("""
+    pref("extensions.foo-bar", true);
+    """, path="defaults/preferences/prefs.js").failed()
+
+    assert not _do_test_raw("""
+    pref("extensions.foo-bar.baz", true);
+    """, path="defaults/preferences/prefs.js").failed()
+
+def test_pref_dangerous_branch():
+    """
+    Test that preferences created in dangerous branches from
+    defaults/preferences/*.js files throw warnings.
+    """
+
+    assert _do_test_raw("""
+    pref("extensions.getAddons.get.url", "http://evil.com/");
+    """, path="defaults/preferences/prefs.js").failed()
+
+    assert _do_test_raw("""
+    user_pref("extensions.getAddons.get.url", "http://evil.com/");
+    """, path="defaults/preferences/prefs.js").failed()
+
+
+def test_pref_complex_code():
+    """
+    Test that calls to functions other than pref() or user_pref() in default
+    preference files throw warnings and that calls to pref() and user_pref()
+    don't.
+    """
+
+    assert not _do_test_raw("""
+    pref();
+    user_pref();
+    """, path="defaults/preferences/prefs.js").failed()
+
+    assert _do_test_raw("""
+    foo();
+    """, path="defaults/preferences/prefs.js").failed()
+
+    assert _do_test_raw("""
+    foo.bar();
+    """, path="defaults/preferences/prefs.js").failed()
+

tests/test_regex.py

@@ -14,7 +14,7 @@ def test_valid():
 def test_basic_regex_fail():
     "Tests that a simple Regex match causes a warning"
 
-    assert _do_test_raw("var x = 'network.http';").failed()
+    assert _do_test_raw("var x = 'network.http.';").failed()
     assert _do_test_raw("var x = 'extensions.foo.update.url';").failed()
     assert _do_test_raw("var x = 'network.websocket.foobar';").failed()
     assert _do_test_raw("var x = 'browser.preferences.instantApply';").failed()
@@ -76,7 +76,7 @@ def test_processNextEvent_banned():
 
 def test_bug_652575():
     """Ensure that capability.policy gets flagged."""
-    assert _do_test_raw("var x = 'capability.policy';").failed()
+    assert _do_test_raw("var x = 'capability.policy.';").failed()
 
 
 def test_app_update_timer():

validator/testcases/javascript/actions.py

@@ -1,5 +1,6 @@
 import copy
 import math
+import re
 import types
 
 import spidermonkey
@@ -336,26 +337,44 @@ def _call_expression(traverser, node):
     map(traverser._traverse_node, args)
 
     member = traverser._traverse_node(node["callee"])
+
+    if (traverser.filename.startswith("defaults/preferences/") and
+        ("name" not in node["callee"] or
+         node["callee"]["name"] not in (u"pref", u"user_pref"))):
+
+        traverser.err.warning(("testcases_javascript_actions",
+                               "_call_expression",
+                               "complex_prefs_defaults_code"),
+                              "Complex code should not appear in preference defaults files",
+                              "Calls to functions other than 'pref' and 'user_pref' should "
+                              "not appear in defaults/preferences/ files.",
+                              filename=traverser.filename,
+                              line=traverser.line,
+                              column=traverser.position,
+                              context=traverser.context)
+
+
     if (member.is_global and
         "dangerous" in member.value and
         isinstance(member.value["dangerous"], types.LambdaType)):
 
         dangerous = member.value["dangerous"]
         t = traverser._traverse_node
         result = dangerous(a=args, t=t, e=traverser.err)
-        if result:
+        if result and "name" in member.value:
             # Generate a string representation of the params
             params = u", ".join([_get_as_str(t(p).get_literal_value()) for
                                  p in args])
             traverser.err.warning(("testcases_javascript_actions",
                                    "_call_expression",
                                    "called_dangerous_global"),
-                                  "Global called in dangerous manner",
+                                  "'%(name)s' function called in potentially dangerous manner"
+                                    % member.value,
                                   result if
-                                    isinstance(result, types.StringTypes) else
-                                  "A global function was called using a set "
-                                  "of dangerous parameters. These parameters "
-                                  "have been disallowed.",
+                                    isinstance(result, (types.StringTypes, list, tuple)) else
+                                  "The global %(name)s function was called using a set "
+                                  "of dangerous parameters. %(name)s calls of this nature "
+                                  "are deprecated." % member.value,
                                   filename=traverser.filename,
                                   line=traverser.line,
                                   column=traverser.position,
@@ -387,7 +406,44 @@ def _call_settimeout(a, t, e):
     those, too.
     """
 
-    return a and a[0]["type"] != "FunctionExpression"
+    if a and a[0]["type"] != "FunctionExpression":
+        return ("In order to prevent vulnerabilities, the setTimeout "
+                "and setInterval functions should be called only with "
+                "function expressions as their first argument.",
+                "Variables referencing function names are acceptable "
+                "but deprecated as they are not amenable to static "
+                "source validation.")
+
+
+def _call_create_pref(a, t, e):
+    """
+    Handler for pref() and user_pref() calls in defaults/preferences/*.js files
+    to ensure that they don't touch preferences outside of the "extensions."
+    branch.
+    """
+
+    if not t.im_self.filename.startswith("defaults/preferences/") or len(a) == 0:
+        return
+
+    value = str(t(a[0]).get_literal_value())
+
+    from predefinedentities import BANNED_PREF_BRANCHES, BANNED_PREF_REGEXPS
+    for banned in BANNED_PREF_BRANCHES:
+        if value.startswith(banned):
+            return ("Extensions should not alter preferences in the '%s' "
+                    "preference branch" % banned)
+
+    for banned in BANNED_PREF_REGEXPS:
+        if re.match(banned, value):
+            return ("Extensions should not alter preferences matching /%s/"
+                        % banned)
+
+    if not value.startswith("extensions.") or value.rindex(".") < len("extensions."):
+        return ("Extensions should not alter preferences outside of the "
+                "'extensions.' preference branch. Please make sure that "
+                "all of your extension's preferences are prefixed with "
+                "'extensions.add-on-name.', where 'add-on-name' is a "
+                "distinct string unique to and indicative of your add-on.")
 
 
 def _readonly_top(t, r, rn):

validator/testcases/javascript/jstypes.py

@@ -205,7 +205,7 @@ def apply_value(name):
                     if name in self.value:
                         output.value[name] = self.value[name]
 
-                map(apply_value, ("dangerous", "readonly", "context"))
+                map(apply_value, ("dangerous", "readonly", "context", "name"))
                 output.is_global = True
                 output.context = self.context
                 return output

validator/testcases/javascript/predefinedentities.py

@@ -16,6 +16,29 @@
                          "instead wherever possible",
 }
 
+BANNED_PREF_BRANCHES = [
+    u"browser.preferences.instantApply",
+    u"capability.policy.",
+    u"extensions.alwaysUnpack",
+    u"extensions.blocklist.",
+    u"extensions.bootstrappedAddons",
+    u"extensions.checkCompatibility",
+    u"extensions.dss.",
+    u"extensions.getAddons.",
+    u"extensions.getMoreThemesURL",
+    u"extensions.installCache",
+    u"extensions.lastAppVersion",
+    u"extensions.pendingOperations",
+    u"extensions.update.",
+    u"general.useragent.",
+    u"network.http.",
+    u"network.websocket.",
+]
+
+BANNED_PREF_REGEXPS = [
+    r"extensions\..*\.update\.(url|enabled|interval)",
+]
+
 # See https://github.com/mattbasta/amo-validator/wiki/JS-Predefined-Entities
 # for details on entity properties.
 
@@ -472,7 +495,11 @@ def build_quick_xpcom(method, interface, traverser):
                        lambda t: {"value": GLOBAL_ENTITIES}}}},
     u"opener":
         {"value":
-             lambda t: {"value": GLOBAL_ENTITIES}}
+             lambda t: {"value": GLOBAL_ENTITIES}},
+
+    # Preference creation in pref defaults files
+    u"pref": {"dangerous": actions._call_create_pref},
+    u"user_pref": {"dangerous": actions._call_create_pref},
 }
 
 CONTENT_DOCUMENT = GLOBAL_ENTITIES[u"content"]["value"][u"document"]

validator/testcases/javascript/traverser.py

@@ -309,17 +309,19 @@ def _build_global(self, name, entity):
                 self.err.warning(("testcases_javascript_traverser",
                                   "_build_global",
                                   "dangerous_global"),
-                                 "Dangerous Global Object",
+                                 "Illegal or deprecated access to the '%s' global" % name,
                                  [dang if
-                                  isinstance(dang, types.StringTypes) else
-                                  "A dangerous or banned global object was "
-                                  "accessed by some JavaScript code.",
-                                  "Accessed object: %s" % name],
+                                  isinstance(dang, (types.StringTypes, list, tuple)) else
+                                  "Access to the '%s' property is deprecated "
+                                  "for security or other reasons." % name],
                                  self.filename,
                                  line=self.line,
                                  column=self.position,
                                  context=self.context)
 
+        if "name" not in entity:
+            entity["name"] = name
+
         # Build out the wrapper object from the global definition.
         result = JSWrapper(is_global=True, traverser=self, lazy=True)
         result.value = entity

validator/testcases/regex.py

@@ -14,25 +14,10 @@
 GENERIC_PATTERNS = {
     r"globalStorage\[.*\].password":
         "Global Storage may not be used to store passwords.",
-    r"browser\.preferences\.instantApply":
-        "Changing the value of instantApply can lead to UI problems in the "
-        "browser.",
-    r"network\.http": NP_WARNING,
-    r"network\.websocket": "Websocket preferences should not be modified.",
-    r"extensions(\..*)?\.update\.url": EUP_WARNING,
-    r"extensions(\..*)?\.update\.enabled": EUP_WARNING,
-    r"extensions(\..*)?\.update\.interval": EUP_WARNING,
-    r"extensions\.blocklist\.url": NP_WARNING,
-    r"extensions\.blocklist\.level": NP_WARNING,
-    r"extensions\.blocklist\.interval": NP_WARNING,
-    r"extensions\.blocklist\.enabled": NP_WARNING,
-    r"general\.useragent": NP_WARNING,
     r"launch\(\)":
         "Use of 'launch()' is disallowed because of restrictions on "
         "nsILocalFile. If the code does not use nsILocalFile, consider a "
-        "different function name.",
-    r"capability\.policy":
-        "The preference 'capability.policy' is potentially unsafe."}
+        "different function name."}
 
 # JS category hunting; bug 635423
 # Generate regexes for all of them. Note that they all begin with
@@ -106,6 +91,20 @@ def _generic_test(pattern, title, message):
                 line=line,
                 context=context)
 
+    def _substring_test(pattern, title, message):
+        """Run a single substringest."""
+        index = document.find(pattern)
+        if ~index:
+            line = context.get_line(index)
+            err.warning(
+                err_id=("testcases_javascript_regex", "generic",
+                        "_generic_test"),
+                warning=title,
+                description=message,
+                filename=filename,
+                line=line,
+                context=context)
+
     def _compat_test(pattern, title, message, compatibility_type=None,
                      appversions=None):
         """Run a single regex test and return a compatibility message."""
@@ -124,6 +123,22 @@ def _compat_test(pattern, title, message, compatibility_type=None,
                 for_appversions=appversions,
                 tier=5)
 
+    if not filename.startswith("defaults/preferences/"):
+        from javascript.predefinedentities import BANNED_PREF_BRANCHES, BANNED_PREF_REGEXPS
+        for pattern in BANNED_PREF_REGEXPS:
+            _generic_test(
+                re.compile("[\"']" + pattern),
+                "Potentially unsafe preference branch referenced",
+                "Extensions should not alter preferences matching /%s/"
+                    % pattern)
+
+        for branch in BANNED_PREF_BRANCHES:
+            _substring_test(
+                branch,
+                "Potentially unsafe preference branch referenced",
+                "Extensions should not alter preferences in the '%s' "
+                "preference branch" % branch)
+
     for pattern, message in GENERIC_PATTERNS.items():
         _generic_test(
                 re.compile(pattern),