553020 - Jetpack-based extensions need unforgeable IDs

Status: RESOLVED FIXED

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

Description

[Atul Varma [:atul]]

In order for Jetpack-based extensions to be upgraded in a secure manner, they need to have an identifier that is unforgeable.

To do this in a distributed manner that doesn't have a single point of failure or control, we're planning on implementing a mechanism similar to Chrome Extensions [1] whereby all releases of an extension need to be self-signed by the party that created it.

This is a blocker for any API that wants to persistently store private data on behalf of an extension, such as the one specified by bug 548589. Without it, Jetpack-based extensions are either liable to being spoofed, or they will break in the future when they need to change their ID to one that is unforgeable.

[1] http://www.eecs.berkeley.edu/Pubs/TechRpts/2009/EECS-2009-185.html

Comment 1

[Atul Varma [:atul]]

Note that this is related to, but does not technically block or depend on, bug 548870.

Also, it's highly likely that we'll need to expose keypair generation and signing functionality from NSS to accomplish this; code from the patch for bug 513798 may help us do this without needing any binary components.

Comment 2

[Brian Warner [:warner :bwarner]]

I'm currently leaning towards 2048-bit RSA signing keys. The "JID" would be the SHA-256 hash of some type-prefix string and an encoded form of the public verifying key.

(incidentally, google Chrome appears to use 1024-bit RSA keys for this purpose)

Comment 3

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

I think Brian is working on this, and we're targeting the 0.3 release, but Brian: please correct me if I'm wrong!

Comment 4

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

Brian and I talked about this recently and came up with a plan for implementing it in the 0.4 release.  Retargeting appropriately.  Stay tuned for more info on the plan and progress from Brian.

Comment 5

[Brian Warner [:warner :bwarner]]

Attachment: proposal describing Jetpack IDs

Here's a document I wrote up (probably to turn into a blog post) about the JetpackID scheme, its rationale, and what the developer workflow should look like.

Comment 6

[Brian Warner [:warner :bwarner]]

Attachment: WIP patch to create JetpackID keys

here's a working patch to produce keypairs during 'cfx xpi' that follows the scheme outlined in the other attachment I just added. It uses ECDSA-192 keypairs, using a pure-python library that I manage externally (at http://github.com/warner/python-ecdsa): it has three .py files that were put in the public domain in 2006, and a bunch of other wrapper code that I wrote last week and that I distribute under the MIT license (although I could license it separately for mozilla use if necessary).

Comment 7

[Atul Varma [:atul]]

Comment on attachment 440895 [details]
proposal describing Jetpack IDs

(I'm marking this for review by me so it shows up in my Bugzilla dashboard.)

Comment 8

[Brian Warner [:warner :bwarner]]

Attachment: new WIP patch, with unit tests

new version, now with unit tests

Comment 9

[Atul Varma [:atul]]

Comment on attachment 440895 [details]
proposal describing Jetpack IDs

This looks good.

Comment 10

[Brian Warner [:warner :bwarner]]

Comment on attachment 441985 [details] [diff] [review]
new WIP patch, with unit tests

With a lot of help from dolske, I was able to confirm that ECDSA is present in NSS (in current minefield), and that the #define which enables it has been turned on since 2006. I'm still trying to get some js-ctypes code written to generate a keypair (as a runtime test), but we've made enough progress to convince me that this choice of crypto algorithm isn't going to cause us problems. So I'm ready to land this patch, and am now looking for feedback on the patch itself. Does the "preflight" workflow seem reasonable? I'm sure there are some developer-ergonomic improvements to be made.

Comment 11

[Atul Varma [:atul]]

Comment on attachment 441985 [details] [diff] [review]
new WIP patch, with unit tests

>-exports.main = function() {
>+exports.main = function(options, callbacks) {
>     console.log("My ID is " + self.id);
> 
>     var hello_html = self.data.load("sample.html");
>@@ -19,4 +19,6 @@
>     var icon_url = self.data.url("mom.png");
>     hello_html.replace("Mom", '<img source="'+icon_url+'">');
>     //let p = new Panel(content: hello_html).show();
>+
>+    callbacks.quit();

Since the test suite for reading-data actually calls exports.main()
with no parameters, you might want to either conditionally call
callbacks.quit() here or (probably preferably?) pass a null-op
callbacks.quit in from the test. Otherwise the test is raising
a 'TypeError: callbacks is undefined' exception.

(Speaking of which, we should probably have the actual tests from
the examples run when 'cfx testall' is called...)

> }
>diff --git a/examples/reading-data/package.json b/examples/reading-data/package.json
>--- a/examples/reading-data/package.json
>+++ b/examples/reading-data/package.json
>@@ -1,10 +1,10 @@

It looks like this file has bit-rotted, probably because 'version' is
now 0.4pre in the repo.

>+                    print "package.json needs modification: please update it and then re-run 'cfx xpi'"

Nit: if you could have this span two separate lines so the line stays
under 80 characters, it would be helpful.

>+def preflight_config(filename, stdout=sys.stdout, keydir=None):
>+    # check the top-level package.json for missing keys. We generate anything
>+    # that we can, and ask the user for the rest.
>+    if keydir is None:
>+        keydir = os.path.expanduser("~/.jetpack-keys")
>+    modified = False
>+    config = json.load(open(filename, 'r'))

Could you use cuddlefish.packaging.get_config_in_dir(path) here
instead of loading the file directly? Or since this is mainly called
from cuddlefish.run(), just take in the JSON blob from your caller,
and have cuddlefish.run() pass in its 'target_cfg' variable, which
is originally retrieved from cuddlefish.packaging.get_config_in_dir()?

This function automatically populates the returned JSON blob with
defaults, one of which is the 'name' property--which defaults to
the name of the directory the package is in:

  https://jetpack.mozillalabs.com/sdk/0.3/docs/#guide/package-spec

So developers would be confused if the docs said 'name' wasn't
required in package.json but 'cfx xpi' thought differently.

>+    if modified:
>+        i = 0
>+        backup = filename + ".backup"
>+        while os.path.exists(backup):
>+            if i > 1000:
>+                raise ValueError("wow, really?")

Heh, maybe make this highly-unlikely error a bit more helpful by telling the
user to delete their .backup files?

Other than that, this patch is looking good, though I haven't actually
tried 'cfx xpi' with it yet, so can't yet comment on the developer
ergonomics.

Comment 12

[Brian Warner [:warner :bwarner]]

Attachment: preflight patch

New patch addresses all the review comments except:

> Could you use cuddlefish.packaging.get_config_in_dir(path) here
> instead of loading the file directly? Or since this is mainly called
> from cuddlefish.run(), just take in the JSON blob from your caller,
> and have cuddlefish.run() pass in its 'target_cfg' variable, which
> is originally retrieved from cuddlefish.packaging.get_config_in_dir()?

I now copy 'target_cfg' into the preflight_config() function, and pull 'name'
from that, but we still need to pass in the filename because
preflight_config() is allowed to *write* a new version of package.json (with
the newly-generated JID). The safest way to emit a new package.json which is
as close as possible to the original is to read the original.. does that seem
reasonable?

A new thing to review: where should we put the private keys? On unixish
systems, ~/.jetpack-keys seems vaguely reasonable, but what about windows?

Comment 13

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

(In reply to comment #12)
> A new thing to review: where should we put the private keys? On unixish
> systems, ~/.jetpack-keys seems vaguely reasonable

Perhaps ~/.jetpack/keys, or am I optimizing prematurely?


> but what about windows?

Windows does have the concept of a user's home directory (mine are C:\Documents and Settings\myk on Windows XP and C:\Users\myk on Windows 7).  So the only issue would be that dot-prefixed files aren't hidden by default.  But I wouldn't think that'd be a big issue for users of the SDK.

Comment 14

[Brian Warner [:warner :bwarner]]

Yeah, ~/.jetpack/keys/ is a good idea. I'll update to match.

And if "C:\Documents and Settings\myk\.jetpack\keys\" seems ok to you, I'm cool with that too.

Comment 15

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

It seems ok to me, and I particularly like that it doesn't require us to do something special for Windows (like store the information in the registry) although admittedly I'm not a Windows-based developer, so I might be missing some significant issue with the approach.

Nevertheless, I suspect it's fine as a first start (we can always change it later if we get feedback from Windows developers about it).  We might want to hide the directory by default, however, just as it is hidden on *nix systems like Linux and Mac OS X.

According to some googling, the way to do that is to set the directory's "hidden" file attribute, which you can do on the command line via "attrib +h <filename>" ("attrib -h <filename>" undoes that).  I'm not sure how to do it from Python, but presumably "there's an API for that."

Comment 16

[Atul Varma [:atul]]

Does os.expanduser() work on Windows? I'd thought it pointed to the user's home dir equivalent (e.g. C:\Documents and Settings\myk) but I'm not sure... In any case, we certainly shouldn't hard-code "C:\Documents and Settings" in, as that directory varies depending on the version of Windows in use (I think Vista uses the much nicer C:\Users, f.e.) or whether the user has roaming profiles enabled, etc.  There's some kind of environment variable or somesuch we can use if os.expanduser() doesn't work.

Also, though, I'm wondering if we should use .jetpack-keys on unix, or actually make a .jetpack dir like we do on windows--advantage being that users could put all kinds of stuff in there, such as their local.json file which specifies defaults and configurations to use (see bug 559143 comment 1).

Comment 17

[Josh Matthews [:jdm]]

FYI, the env var is %USERPROFILE%.

Comment 18

[Atul Varma [:atul]]

I'd recommend against marking the file as "hidden" under windows if possible: in unix-land, files prefixed with a dot are a convention implying something about the file with the dot, whereas on windows "hidden" files are files that the OS is actively trying to hide from the user, meaning that it's actually quite a hassle for users to access them if and when they need to.

I'd much rather the directory just stay un-hidden, or even be named something more Windows-y like "Jetpack SDK Data" (ugh spaces, I know).

Another option is actually to put this in the Application Data directory, which is where Firefox puts its profile data, but since the SDK isn't really an "application" in the desktop sense, it might not make much sense.

It might also be worth asking for advice on the google group, with the expectation that this could lead to bikeshedding.

In any case, for expediency's sake, if we just want to implement something, I'd recommend just using '.jetpack' as a dir for all platforms and *not* explicitly hiding it on windows--that way no windows-only conditional code exists, too.

Comment 19

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

(In reply to comment #18)
> In any case, for expediency's sake, if we just want to implement something, I'd
> recommend just using '.jetpack' as a dir for all platforms and *not* explicitly
> hiding it on windows--that way no windows-only conditional code exists, too.

Sounds fine to me.

Comment 20

[Brian Warner [:warner :bwarner]]

Yeah, I was planning to use os.expanduser("~/.jetpack/keys"), which IIRC does
the right thing on windows too. And by using $HOME/.jetpack/, we can put other
stuff in there too, and only need to specify the contents of the keys/ subdir
for now.

I'm inclined to hold off on trying to hide that directory for now.. wait until
somebody on windows complains about it first.

Comment 21

[Brian Warner [:warner :bwarner]]

Attachment: preflight patch

changed the private-key directory to ~/.jetpack/keys/ , so we can put other stuff in ~/.jetpack/ later.

Comment 22

[Brian Warner [:warner :bwarner]]

Attachment: preflight patch

This version changes the name of the package.json key from "id" to "jetpackID", to avoid colliding with the CommonJS use of "id". It updates some harness files (like rdf.py) to read "jetpackID" instead of "id" when determining things like the GUID of the generated XPI file. It also adds some docs to dev-guide/xpi.md and package-spec.md .

I'm not entirely sure I got this "jetpackID" part right, so I'd appreciate feedback. Should we leave a separate definition for "id" in package-spec.md? (my hunch is no). Also, I'm having (unrelated) test failures right now, so I can't be sure that the unit tests are still passing.. there don't seem to be any new failures. But I'd like to generate a real XPI with this new code and make sure it works and has a sensible id.

Comment 23

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

Comment on attachment 445501 [details] [diff] [review]
preflight patch

Overall, this approach looks good.  Just a few notes:

1. Although Jetpack is currently the name of the SDK, that won't always be the case, since Jetpack is a project name, and the name of the product that the SDK eventually becomes will be something more like Add-on Builder.

Thus I'd like to avoid using "jetpack" in the names of data fields (and packages, and APIs) that are visible to developers, and rather than calling this "jetpackID" in package.json, let's call it something like "addonID" or "programID" (Atul: do you have a preference?).

2. Per the install manifest docs <https://developer.mozilla.org/en/Install_Manifests#id>, the "em:id" field in install.rdf needs to be either a GUID or an email-like string (i.e. [e-mail]).  In this case, we could use an email-like string, putting the unforgeable ID in the "foo" section before the @ and anything in the "bar" and "baz" sections (iam.kul, thereisonly.html, addon.moz, etc.).

Comment 24

[Atul Varma [:atul]]

Hmm, good question myk.

Would publicKey be too obtuse? I guess I'm not sure what the role of these identifiers might be for "jetpack programs", i.e. packages with a main() that might ultimately be standalone apps or addons. I guess I would prefer 'programID' from an architectural standpoint, though from an end-user standpoint, for the immediate purposes of the Jetpack SDK, addonID actually seems more developer-ergonomic.

Comment 25

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

(In reply to comment #24)

> Would publicKey be too obtuse?

Yes. :-)


> I guess I would prefer 'programID' from an architectural standpoint,

Sold!

Comment 26

[Brian Warner [:warner :bwarner]]

what about "packageID"? Hrm, actually, I guess that collides with the use of "package" as the common-js thingy that is bigger than a "module" but smaller than a "program".

(it would be nice to have a picture showing how these different terms relate.. I get very confused by them).

re: email-like string.. I think the main question to ask is: what is the purpose of the non-keyid part of the string? For the security properties of the keyid to be maintained, we need to make sure that nothing in the platform pays attention to the non-keyid portion (specifically that we don't make any decisions based upon that part), otherwise "JID1@lookatme" would be a valid forgery of "JID2@lookatme".

IIRC, the reason for encouraging an email-like string is primarily to get sufficiently-unique IDs from folks who don't want/can't produce a random GUID, and secondarily to deal with subsequent bad interactions where somebody (end user?) is presented with a list of IDs and asked to do pick one of them (like looking in your profile directory for source code from a given add-on).

The cryptographic JID is just like the old GUID, except that the random string has some additional useful properties. It still looks completely random, though.

Anyways, whichever way we go, we just need to make sure the ID is machine-parseable, and that no code ever makes security decisions based upon something other than the cryptographic portion, and that users are not enticed into making security decisions on the non-crypto portions either. The easiest way to achieve all of those is to make the ID purely out of the cryptographic JID, with nothing else. But that's not an absolute requirement.

Comment 27

[Atul Varma [:atul]]

(In reply to comment #26)
> what about "packageID"? Hrm, actually, I guess that collides with the use of
> "package" as the common-js thingy that is bigger than a "module" but smaller
> than a "program".
> 
> (it would be nice to have a picture showing how these different terms relate..
> I get very confused by them).

Erm, sorry. :(  A package can be thought of as a "bundle" in OS X, if you're familiar with those, and a "program" is like an app bundle: a particular kind of package that represents a program.

Maybe it makes more sense to explicitly separate out the use case of "application" vs. "addon", since they're increasingly different things--I think that's where some of the "program" terminology becomes confusing, b/c it essentially encompasses both "application" and "addon". It might also be useful to actually use the terms "application package" and "addon package", as this conveys that the entity being described is both an application *and* a package, or an addon *and* a package, respectively.

> The cryptographic JID is just like the old GUID, except that the random string
> has some additional useful properties. It still looks completely random,
> though.

When Myk (or other Mozilla engineer-folk) says "GUID", I think he means Microsoft's Universally Unique Identifier standard:

  http://en.wikipedia.org/wiki/Universally_Unique_Identifier

Which has to be 128 bits. If our keys are any smaller, I guess we could use padding, but if they're bigger, I'm not sure how we'd format them so they looked like a UUID.

I could be wrong though.

Comment 28

[Atul Varma [:atul]]

Comment on attachment 445501 [details] [diff] [review]
preflight patch

>diff --git a/static-files/md/dev-guide/xpi.md b/static-files/md/dev-guide/xpi.md
>--- a/static-files/md/dev-guide/xpi.md
>+++ b/static-files/md/dev-guide/xpi.md
>@@ -135,4 +135,51 @@
> examples is a unique identifier that the SDK prepends to all
> `resource:` URIs to namespace the XPI's resources so they don't
> collide with anything else, including other extensions built by the
>-SDK and containing the same packages.
>+SDK and containing the same packages. This GUID is built from the
>+"Jetpack ID", described below.
>+
>+The JetpackID
>+-------------

Hmm, so I'm not sure if this is actually the best place to communicate what the importance of the JetpackID is. While some of the more technical aspects of it can be put here, it seems like the bulk of the "big picture" and workflow should be explained in the "how to make your first Jetpack-based addon" tutorial.

>+When you run `cfx xpi` for the first time on a new add-on, your
>+`package.json` will be examined for the presence of a `jetpackID` key. If
>+missing, a new keypair will be generated for you: the public key will be
>+written into `package.json`, and the private key will be saved in a file in
>+`~/.jetpack/keys/` (or a similar place on windows). You will be asked to
>+re-run the `cfx xpi` command so it can pick up the updated ID.

I'm wondering how hard it'd be to have cfx just restart itself for the user, to that making a really simple addon stays simple.

>+The private key is very important! If you lose it, you will not be able to
>+upgrade your add-on: you'll have to create a new add-on ID, and your users
>+will have to manually uninstall the old one and install the new one. If
>+somebody else gets a copy of your private key, they will be able to write
>+add-ons that could displace your own.

We might want to address exactly how a team of developers ought to work using this mechanism, especially since lots of open-source projects have multiple developers on them, any one of whom theoretically may need to be able to put out a new release at any moment. Even a link to some kind of article on how to go about doing this securely would be helpful.

>+If you start your add-on work by copying somebody else's source code, you'll
>+need to remove their Jetpack ID from the `package.json` file before you can
>+build your own XPIs. Again, `cfx xpi` will remind you of this, and your
>+options, when you attempt to build an XPI from a `package.json` that
>+references a private key that you don't have in `~/.jetpack/keys/`.

Do you think it'd be hard for cfx to just recognize when this is the case and prompt the user if they want to make a new key?  Just in the interest of making the process of remixing addons or building on someone else's work as easy as possible.

Aside from the above concerns, there's also the one myk mentioned about renaming jetpackID to something else.

Finally, I think some part of your documentation refers to require("self").id... Shouldn't this be require("self").jetpackID or .programID or whatever other name we end up deciding to use?

Comment 29

[Brian Warner [:warner :bwarner]]

> Hmm, so I'm not sure if this is actually the best place to communicate what
> the importance of the JetpackID is. While some of the more technical
> aspects of it can be put here, it seems like the bulk of the "big picture"
> and workflow should be explained in the "how to make your first
> Jetpack-based addon" tutorial.

Yeah, I was concerned about injecting too much detail into that tutorial and
distracting from the flow.. it's already pretty long. Maybe a small sidebar
about what the key is used for, with a pointer to a larger article. But
should the larger article go in "XPI Generation", or a separate appendix? The
keygen stuff didn't seem important enough to justify a whole appendix.

>+`~/.jetpack/keys/` (or a similar place on windows). You will be asked to
>+re-run the `cfx xpi` command so it can pick up the updated ID.

> I'm wondering how hard it'd be to have cfx just restart itself for the
> user, to that making a really simple addon stays simple.

I looked briefly at this, but I wanted to minimize the surgery I'd have to do
on cuddlefish/__init__.py: I couldn't tell how early that code stashes
the information it pulls from package.json . If I just re-executed the:

        pkg_cfg = packaging.build_config(env_root, target_cfg)

line, would that be sufficient?

> We might want to address exactly how a team of developers ought to work
> using this mechanism ... Even a link to some kind of article on how to go
> about doing this securely would be helpful.

Yeah. I'll add a page to the wiki (maybe Labs/Jetpack/JetpackID?) with more
details, and link to that from the right paragraph in "XPI Generation".

>+Again, `cfx xpi` will remind you of this, and your options, when you
>+attempt to build an XPI from a `package.json` that references a private key
>+that you don't have in `~/.jetpack/keys/`.

> Do you think it'd be hard for cfx to just recognize when this is the case
> and prompt the user if they want to make a new key?

Yeah, but I was trying to avoid making "cfx xpi" too interactive. The two
directions we can go are 1: tell the user to fix something and then re-run
"cfx xpi", or 2: ask the user about fixing things, wait for their response
(with input()), then fix them and keep going. Using blocking/modal inputs
makes it hard to build automation or frontend scripts against "cfx xpi",
unless we then also build in "always-yes" or "never-ask" sorts of arguments.

Of course, this second "you don't have the private key" case is more
important to bring to the user's attention than the earlier "you didn't
specify a key" case, because the add-on's continuity of identity is at stake.

> Finally, I think some part of your documentation refers to
> require("self").id... Shouldn't this be require("self").jetpackID or
> .programID or whatever other name we end up deciding to use?

Hm. I think of the "self" module as implicitly defined to talk about the
add-on/jetpack/program as a whole, so putting that term in the property name
as well would be redundant (and would require updating it to match the
changes we're discussing now).

On the other hand, having the same name for package.json and "self" might
help those developers who have to touch both pieces.

Comment 30

[Brian Warner [:warner :bwarner]]

Ok, after more discussion with Atul, I've renamed the package.json field back to just "id". I think we can use this one without colliding with anything that CommonJS will dictate (we *are* the sort of "package management tool" that the "id" name is reserved for).

Myk: sorry, I didn't look closely enough at that "Install Manifests" page, I see now that firefox actually enforces the GUID-or-emailish format (I thought it was merely a convention before). Yeah, so, I need to change the ID generation code to append an @ and some other letters at the end.

Comment 31

[Brian Warner [:warner :bwarner]]

Attachment: preflight patch

this one renames the id property back to 'id', and makes the IDs email-like so that FF won't reject them. Docs shuffled slightly.

Comment 32

[Atul Varma [:atul]]

Comment on attachment 445878 [details] [diff] [review]
preflight patch

Looks great! The only thing I might change is explaining what the '~' directory is for Windows users; perhaps just pointing them to the Wikipedia page on "Home directory" might help:

  http://en.wikipedia.org/wiki/Home_directory

Anyhow, just a suggestion. Looks good as-is.

Comment 33

[Brian Warner [:warner :bwarner]]

pushed, in 4c19e848f223

Comment 34

[Piotr Zalewa [:zalun]]

I get the feeling that the ID was used only in the Addon context. For the sake of uniqueness inside FlightDeck database I will need an ID generated for the Libraries as well. I don't think it changes much, but please correct me if I'm wrong.

Comment 35

[Brian Warner [:warner :bwarner]]

Piotr: hm, we should discuss this further (maybe in another bug?). I think that modules and things-that-can-be-require()d should have IDs, but it's much less obvious to me what their ID value ought to be.

But yeah, the IDs described in this bug are only for the overall addon.

Comment 36

[Piotr Zalewa [:zalun]]

Good to know, but no worries - ID generation can be different for Libraries and Addons.

Comment 37

[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".