572020 - implement privilege-reducing "manifest" metadata

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Brian Warner [:warner :bwarner]]

Attachment: preliminary patch: generate manifest, implement require("chrome")

We've kicked around ideas on how we're going to approach the whole reduced-privilege goal in Jetpack. One of our starting points is to define a "manifest" which lists, for each module, what other modules it requires and whether it wants "chrome authority" or not (i.e. if it uses Components.classes and friends). The idea is that the manifest is machine-parseable, reviewers can look at it (instead of trying to understand the code) to see who gets to use what, and the jetpack loader can use it to enforce limitations on what require() does at runtime.

The first pass at this is to have a tool in the SDK scan the JS code (at XPI-building time) and look for require() statements. It then creates the manifest and includes it in the harness-options.json file (inside the XPI). From there, the loader will be able to read it and affect the way that require() works.

The idea is that module authors have to be up-front about what sorts of modules (and by extension, what sorts of authority) their code gets to use. They won't be able to trick the reviewers, e.g. make their module *look* like it doesn't need much authority, but really sneak in a disguised require("something-powerful") call later on, because the runtime-enforced require() restrictions will be working off the same information that the reviewer got to see. So if they try to be tricky, their require() calls will just fail.

To use "chrome authority", the magic incantation is:

 var {Cc, Ci} = require("chrome");

(or let, or const, etc). This is a special-case of require(), in that there is no actual "chrome.js" module. Instead, this returns an object with the usual Cc/Ci/Cu/Cr/Cm aliases.

I've got a patch which generates the manifest, warns about old-style use of raw Components.classes, changes require("chrome") to work in the new style, and updates all the jetpack-core modules to use the new style. It does not yet do anything special with require() of other modules: I'll need Atul's help to figure out how to get require() to change behavior depending upon which module calls it (i.e. to give a different require() function to each module).

Comment 1

[Brian Warner [:warner :bwarner]]

Attachment: generate manifest, implement require("chrome")

updated patch: now has unit tests. Still working on docs.

This is enough to catch+encourage use of approved 'require("chrome")' pattern, but does not attempt to actually confine the runtime access.

Comment 2

[Brian Warner [:warner :bwarner]]

Attachment: generate manifest, implement require("chrome")

ok, docs are in. This patch is ready for review.

Comment 3

[Atul Varma [:atul]]

Thanks! I've taken a really quick look at the patch and I'm a bit concerned that this may be too volatile a change to introduce at the very end of a development cycle, and I wonder if it'd be better to introduce it at the beginning of 0.6 instead.

Aside from any technical issues, this would also give us a nice way of understanding the change's developer ergonomics, as we'd effectively be incubating this new style on the core jetpack team, working out any dev erg issues throughout the dev cycle, and then be releasing something we have a good feeling for once we release to the public--rather than landing something that may miss some major use cases to a wider audience.

Thoughts?

Comment 4

[Brian Warner [:warner :bwarner]]

yeah, that's kind of what I'm thinking too.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 451395 [details] [diff] [review]
generate manifest, implement require("chrome")

I see some usage of Components.stack in the codebase, both in cuddlefish.js and securable-module.js, which don't need to request chrome access, and memory.js and traceback.js, which do.  Is that something that cuddlefish/securable-module will need to provide to the latter two modules?

I also see a reference to Components.ID in packages/development-mode/lib/harness.js.  Does that need to be provided as well?

Finally, I see a variety of tests accessing Components.  Presumably we don't need to limit test access to chrome, but if we stop injecting Components.* objects into the global scopes of modules, does that affect test modules as well?


>diff --git a/python-lib/cuddlefish/tests/test_manifest.py b/python-lib/cuddlefish/tests/test_manifest.py

>+        # these forms are commented out, and thus ignored
>+
>+        mod = """// var foo = require('one');"""
>+        requires, chrome = self.scan(mod)
>+        self.failUnlessEqual(requires, [])
>+        self.failUnlessEqual(chrome, False)
>+
>+        mod = """/* var foo = require('one');"""
>+        requires, chrome = self.scan(mod)
>+        self.failUnlessEqual(requires, [])
>+        self.failUnlessEqual(chrome, False)
>+
>+        mod = """ * var foo = require('one');"""
>+        requires, chrome = self.scan(mod)
>+        self.failUnlessEqual(requires, [])
>+        self.failUnlessEqual(chrome, False)

It looks like the scanner misses the following, which is legal JS:

  var foo;
  1
   * (foo = require("tabs"));
  // foo is now the tabs module's exported object.

However, this probably falls in the category of "tricky, denied".


>+        # multiple requires
>+
>+        mod = """const foo = require('one');
>+        const foo = require('two');"""
>+        requires, chrome = self.scan(mod)
>+        self.failUnlessEqual(requires, ["one", "two"])
>+        self.failUnlessEqual(chrome, False)

Presumably this (and the next test) means to assign "two" to a different const?


>+        # this doesn't work right now. We should decide whether to support or
>+        # explicitly reject this. Our goal is to require a clear statement of
>+        # intent, so it wouldn't be too bad if we forced module authors to
>+        # put their requirements on separate lines.
>+        mod = """const foo = require('one'); const foo = require('two');"""

These feels less like "tricky, denied" and more "convenient, albeit uncommon", and thus reasonable to support, although I'm ok with forgoing it if it's hard to do.

It'd also be handy to scan some uses of "require" embedded into statements that do additional work, as "require" is commonly used in this way throughout the core library.


>+class Package(unittest.TestCase):
>+    def test_jetpack_core(self):
>+        # this has a side-effect of asserting that all the SDK's jetpack-core
>+        # modules are clean.
>+        jp_core = "packages/jetpack-core/lib"
>+        assert os.path.isdir(jp_core) # we expect to be run from the SDK top
>+        stderr = StringIO()
>+        manifest, has_problems = scan_package("jetpack-core", jp_core, stderr)
>+        stderr.seek(0)
>+        err = stderr.readlines()
>+        self.failUnlessEqual(err, [])

This is failing with errors in localization.js, tabs.js, and selection.js, which would make sense, given that those APIs landed late in 0.5, after you created this patch.  It's also fantastic, because it shows that the checking works!  However, the test output is difficult to read, as it serializes to an array of escaped newline-delimited strings:

--------------------------------------------------------------------------------
(jsdk-manifest)myk@myk:~/Projects/jsdk-manifest$ cfx testcfx
...................F...................................
======================================================================
FAIL: test_jetpack_core (cuddlefish.tests.test_manifest.Package)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/myk/Projects/jsdk-manifest/python-lib/cuddlefish/tests/test_manifest.py", line 142, in test_jetpack_core
    self.failUnlessEqual(err, [])
AssertionError: ['\n', 'To use chrome authority, as in:\n', ' packages/jetpack-core/lib/localization.js\n', ' 154> let fh = Cc["@mozilla.org/file/directory_service;1"].\n', ' 155> getService(Ci.nsIProperties).\n', ' 156> get("ProfD", Ci.nsIFile);\n', " 171> let file = Cc['@mozilla.org/file/local;1'].\n", ' 172> createInstance(Ci.nsILocalFile);\n', 'You must enable it with something like:\n', '  const {Cc,Ci} = require("chrome");\n', '\n', 'To use chrome authority, as in:\n', ' packages/jetpack-core/lib/selection.js\n', ' 223> QueryInterface: require("xpcom").utils.generateQI([Ci.nsISelectionListener]),\n', ' 244> Ci.nsISelectionListener[type + "_REASON"]) || selection.toString() == "")\n', ' 295> if (selection instanceof Ci.nsISelectionPrivate)\n', ' 312> if (selection instanceof Ci.nsISelectionPrivate)\n', 'You must enable it with something like:\n', '  const {Ci} = require("chrome");\n', 'Bad use of chrome authority at line 45 in:\n', ' packages/jetpack-core/lib/tabs.js\n', ' > Components.utils.import("resource://gre/modules/NetUtil.jsm");\n', 'Use:\n', "  let {Cc,Ci,Cu,Cr,Cm} = require('chrome');\n", 'instead.\n', '\n', '\n', 'To use chrome authority, as in:\n', ' packages/jetpack-core/lib/tabs.js\n', ' 85> return Cc["@mozilla.org/browser/favicon-service;1"].\n', ' 86> getService(Ci.nsIFaviconService).\n', ' 175> const wm = Cc["@mozilla.org/appshell/window-mediator;1"].\n', ' 176> getService(Ci.nsIWindowMediator);\n', 'You must enable it with something like:\n', '  const {Cc,Ci} = require("chrome");\n'] != []

----------------------------------------------------------------------
Ran 55 tests in 3.139s

FAILED (failures=1)
--------------------------------------------------------------------------------

It should display those errors in a more readable fashion.


>diff --git a/python-lib/cuddlefish/xpi.py b/python-lib/cuddlefish/xpi.py

>-        zf.writestr(zipfile.ZipInfo(base_arcpath + "/"), "")
>+        zf.write('.empty', base_arcpath+"/.empty")

This has the side-effect of adding unused ".empty" files to the XPI bundles.  Is there a reason it's better than using ZipFile.writestr?

Also, you need to update static-files/md/dev-guide/xpi.md to show the correct output based on these changes.  Note: when the xpi.md test fails for me, I see a lot of false positives (lines that the test output claims are different, although there appears to be no difference between them).  Those go away after I fix the actual differences, though.  Not sure what's going on there.


>diff --git a/static-files/md/dev-guide/chrome.md b/static-files/md/dev-guide/chrome.md

>+Your modules should refrain from using chrome privileges unless they are
>+absolutely necessary. Chrome-authority-using modules must receive extra
>+security review, and most bugs in these modules are security-critical.

When you say "most bugs", don't you mean "most security bugs"?  Presumably most bugs in these modules don't actually expose vulnerabilities, although the ones they do expose tend to be critical.

Comment 6

[Drew Willcoxon :adw]

Wait, this actually scans the source for "require"?  Will it catch these?

  this["require"]("doom");
  this["r" + "e" + "q" + "u" + "i" + "r" + "e"]("doom");
  this["eriuqer".split("").reverse().join("")]("doom");

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> Wait, this actually scans the source for "require"?  Will it catch these?
> 
>   this["require"]("doom");
>   this["r" + "e" + "q" + "u" + "i" + "r" + "e"]("doom");
>   this["eriuqer".split("").reverse().join("")]("doom");

No, but that's ok, since the primary point is actually not to catch tricky usages.  The idea is that conventional usage of require will result in an entry in the manifest specifying that the add-on uses the module being required.

Then, the next step is to enforce the list of modules that the manifest says are being required, so no other modules can be required via tricky means like the ones above.

That means it's actually ok for tricky usage not to be caught, because that also means it won't be written to the manifest, so it won't succeed at runtime (once we start enforcing the manifest).

That's my understanding, anyway.  Brian should correct any inaccuracies, though.

Comment 8

[Drew Willcoxon :adw]

(In reply to comment #7)
> That means it's actually ok for tricky usage not to be caught, because that
> also means it won't be written to the manifest, so it won't succeed at runtime

Oh right.  I had it backwards.  Nevermind me! :)

Comment 9

[Atul Varma [:atul]]

I concur with Myk, though it's also important that we provide good feedback for why a module that a developer might be trying to require() won't load--e.g. providing a link to documentation on this in the exception that's thrown.

Comment 10

[Brian Warner [:warner :bwarner]]

>>   this["eriuqer".split("").reverse().join("")]("doom");

> No, but that's ok, since the primary point is actually not to catch tricky
> usages. The idea is that conventional usage of require will result in an
> entry in the manifest specifying that the add-on uses the module being
> required.
> 
> That's my understanding, anyway. Brian should correct any inaccuracies,
> though.

Right. The rule is that you must provide clear+parseable require() statements
or else your module won't get the imports that it wants. Tricky/obsfuscated
require() statements won't work (they'll be legal JS, but the manifest
scanner won't see them, so they'll give a runtime error).

This is a case where the scanner can be pretty dumb, and we can enforce some
style rules, as long as it isn't too painful to follow those rules. The main
"not too tricky, should accept" form that isn't currently accepted is the
"require(one); require(two)" form, but I think I can fix that pretty easily
(split each line on ";" before processing).

> It'd also be handy to scan some uses of "require" embedded into statements
> that do additional work, as "require" is commonly used in this way
> throughout the core library.

I think that ought to work right now, but I'll add a test to be sure.

> I see some usage of Components.stack in the codebase, both in cuddlefish.js
> and securable-module.js, which don't need to request chrome access, and
> memory.js and traceback.js, which do. Is that something that
> cuddlefish/securable-module will need to provide to the latter two modules?

Hm. To accomodate those, and also Components.isSuccessCode (which got used in
text-streams.js), I'll add 'components' (note the lower-case) next to
Cc/Ci/etc . Then e.g. memory.js will use:

 var {components} = require("chrome");
 var frame = components.stack.caller;

I'm generally assuming that cuddlefish.js and securable-module.js are the
bootstrapping modules, and will need to access Components without a
require().

> I also see a reference to Components.ID in
> packages/development-mode/lib/harness.js.  Does that need to be provided as
> well?

Good eye, I'll fix that one with the same technique.

> Finally, I see a variety of tests accessing Components.  Presumably we don't
> need to limit test access to chrome, but if we stop injecting Components.*
> objects into the global scopes of modules, does that affect test modules as
> well?

I've just done a pass to fix the unit tests by adding require("chrome") lines
to them too.

I should have a new version of the patch up later this afternoon.

Comment 11

[Atul Varma [:atul]]

(In reply to comment #10)
> I'm generally assuming that cuddlefish.js and securable-module.js are the
> bootstrapping modules, and will need to access Components without a
> require().

Yes, that's correct.

Comment 12

[Atul Varma [:atul]]

I've seen 'CC' refer to 'Components.Constructor' before, so you might want to put this in the 'chrome' module too.

Also, you should probably remove or augment the "security roadmap" file from the SDK documentation [1] in this patch.

[1] https://jetpack.mozillalabs.com/sdk/0.4/docs/#guide/security-roadmap

Comment 13

[Atul Varma [:atul]]

Oh, also, if the jsscan module isn't being used, I'm cool with just removing it from the patch, so we don't commit "dead" code that's never used.

Comment 14

[Brian Warner [:warner :bwarner]]

> >-        zf.writestr(zipfile.ZipInfo(base_arcpath + "/"), "")
> >+        zf.write('.empty', base_arcpath+"/.empty")
> 
> This has the side-effect of adding unused ".empty" files to the XPI
> bundles. Is there a reason it's better than using ZipFile.writestr?

Oops, I forgot that was in there.. I should move that into a different
bug/patch. The improvement is that, at least for me, the zipfile directories
created by writestr() wind up with a weird all-zeros mode, and then "rm -r"
won't delete them. The ".empty" files seemed to work better.

But I'll move that out to a different bug.

> I've seen 'CC' refer to 'Components.Constructor' before, so you might want
> to put this in the 'chrome' module too.

I'm updating the patch to spot "Components." on lines that don't appear to be
creating one of the common aliases, and advise developers to use "var
{components} = require("chrome");" instead. I think that should cover
Components.stack, Components.isSuccessCode, Components.Constructor, and other
less-popular siblings.

> Also, you should probably remove or augment the "security roadmap" file
> from the SDK documentation [1] in this patch.

Ok, will do. I feel bad about removing something large and replacing it with
something small, but it's probably better to be clear about our lack of
clarity.

> Oh, also, if the jsscan module isn't being used, I'm cool with just
> removing it from the patch, so we don't commit "dead" code that's never
> used.

Sure thing. I was uncertain about which way to go with the parser, but at
this point I think the simple line-oriented regexp form is good enough.

Comment 15

[Brian Warner [:warner :bwarner]]

Attachment: generate manifest, implement require("chrome")

New patch which should address all the feedback issues except removing/rewriting security-roadmap.md (I don't yet know what should go there, and the existing text has enough value to leave in place for now). "cfx testall" passes now. Dead code removed.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 453258 [details] [diff] [review]
generate manifest, implement require("chrome")

The fixes seem fine, and the conflict on tip is trivial to resolve.  But when I run cfx testall, it raises an exception:

Testing cfx...
Traceback (most recent call last):
  File "/home/myk/Projects/jsdk-manifest/bin/cfx", line 6, in <module>
    cuddlefish.run()
  File "/home/myk/Projects/jsdk-manifest/python-lib/cuddlefish/__init__.py", line 318, in run
    test_all(env_root, defaults=options.__dict__)
  File "/home/myk/Projects/jsdk-manifest/python-lib/cuddlefish/__init__.py", line 190, in test_all
    result = test_cfx(env_root, defaults['verbose'])
  File "/home/myk/Projects/jsdk-manifest/python-lib/cuddlefish/__init__.py", line 215, in test_cfx
    retval = cuddlefish.tests.run(verbose)
  File "/home/myk/Projects/jsdk-manifest/python-lib/cuddlefish/tests/__init__.py", line 55, in run
    tests = get_tests()
  File "/home/myk/Projects/jsdk-manifest/python-lib/cuddlefish/tests/__init__.py", line 21, in get_tests
    module = __import__(full_name, fromlist=[package.__name__])
  File "/home/myk/Projects/jsdk-manifest/python-lib/cuddlefish/tests/test_manifest.py", line 5, in <module>
    from cuddlefish.manifest import scan_module, scan_package
ImportError: cannot import name scan_module

It seems like something simple, but it isn't obvious at first glance.

Comment 17

[Brian Warner [:warner :bwarner]]

myk: do you perhaps have a leftover python-lib/cuddlefish/manifest/ directory lying around? A previous version of the patch had more new files, so I was putting things in manifest/*.py , but then I shrunk it enough to fit it all into just one file. If you still have the manifest/__init__.py(c) from the earlier patch, python might be picking that ahead of the new manifest.py file.

I'll upload a new version of the patch that resolves the tiny merge-to-trunk conflict.

Comment 18

[Brian Warner [:warner :bwarner]]

Attachment: generate manifest, implement require("chrome")

fix the tiny merge conflict with current trunk

Comment 19

[Atul Varma [:atul]]

Comment on attachment 454572 [details] [diff] [review]
generate manifest, implement require("chrome")

>-     var globals = {Cc: Components.classes,
>-                    Ci: Components.interfaces,
>-                    Cu: Components.utils,
>-                    Cr: Components.results};
>+     var globals = {};

Very minor nit: I'm wondering if there's a way to deprecate these for one SDK release rather than remove them completely, possibly by adding getters to the global scope's prototype or somesuch that returns the object but also logs a warning telling the developer that they'll go away in the next release. If it's not a cinch to implement that kind of thing, though, I guess we shouldn't worry about it.

>diff --git a/packages/jetpack-core/lib/tabs.js b/packages/jetpack-core/lib/tabs.js
>--- a/packages/jetpack-core/lib/tabs.js
>+++ b/packages/jetpack-core/lib/tabs.js
>@@ -42,8 +42,10 @@ if (!require("xul-app").is("Firefox")) {
>   ].join(""));
> }
> 
>-Components.utils.import("resource://gre/modules/NetUtil.jsm");
> 
>+
>+const {Cc,Ci,Cu} = require("chrome");
>+Cu.import("resource://gre/modules/NetUtil.jsm");

Does this work with the bug that makes it impossible for Cu.import() to inject a module's exported symbols into the global scope of a CommonJS module? I don't think we have that bug in Bugzilla, I should probably add it.  You get around this bug in the changes you make to text-streams.js in this patch, but you don't seem to here.

Other than that, this looks great!

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 454572 [details] [diff] [review]
generate manifest, implement require("chrome")

Ah, I did indeed have an old python-lib/cuddlefish/manifest/__init__.pyc (and jsscan.pyc and scan.pyc).  I'd purged the repository in which I was testing with |hg purge|, but it left that file around (without mentioning it in |hg status|), probably because *.pyc files are .hgignored.

I see the problem Atul mentioned in tabs.js (it shows up as a test failure with |cfx testall -a firefox|).  Also, test-private-browsing.js, test-context-menu.js, and test-tab-browser.js need |require("chrome")| added to them (which doesn't manifest on |cfx testall|, just on |cfx testall -a firefox|).

r=myk with those fixed!

Comment 21

[Brian Warner [:warner :bwarner]]

Attachment: generate manifest, implement require("chrome")

Ok, hopefully the last revision: I fixed the NetUtil problem in tabs.js, and added require("chrome") clauses to everything in jetpack-core/tests/ . (I also added a __main__ clause to python-lib/cuddlefish/manifest.py so you can run it directly on the files of your choice, to make it easier to run against files outside of lib/ directories).

I don't know how to approach the globals Cc/Ci getters/deprecation thing, and Atul says to not worry about it for now. Good enough for me.

Running tests with "cfx testall -a firefox" revealed a lot of problems that I didn't see before with merely "cfx testall". All of those are fixed now, except for a browser.js JS error which Atul says is not happening in jetpack at all (something about the test framework opening and closing a window so quickly that FF gets upset), and it appears that this one happens for other people without this patch, so I think landing this patch won't cause a regression.

I'll leave this to bake another day, in case anyone else wants to look it over and provide feedback. Then I'll land it tomorrow.

thanks!
 -Brian

Comment 22

[Brian Warner [:warner :bwarner]]

landed, in http://hg.mozilla.org/labs/jetpack-sdk/rev/073497977f97

I'll be iterating over the manifest format for a little while, so I may leave this ticket open for a bit longer.

Comment 23

[Atul Varma [:atul]]

I've spoken to Brian, and we'll close this bug and mark bug 591515 as its successor.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".