Extension manager should use xpinstall crypto hashes

RESOLVED FIXED

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: dveditz, Assigned: rstrong)

Tracking

({fixed1.8})

Trunk
fixed1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: requires 310421 also)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
bug 302284 added crypto hashing support to xpinstall to protect against mirror
network hacks. bug 302287 will add support for hashing to AMO and the extensions
database. update.js and nsExtensionManager.js should use this data to validate
the downloaded extensions.

As part of bug 302284 a new method was added to nsIXPInstallmanager.
initManagerWithHashes replaces initManagerFromChrome and should be called in the
two places it's used in the extension manager code. When hashes are not known
null can be passed instead, either for the entire argument, or for an individual
item in the array.

The update service will need to add hash data (if in the database) to the rdf
returned to the client.

The client will have to read the hash from the update data.

The client then passes the hash to the install manager.

The AMO database changes already have a separate bug (bug 302287). Should the
update service rdf changes be split out from the client changes?
(Reporter)

Comment 1

13 years ago
Nominating for Firefox 2.  Much as I'd like to see the added security in 1.5
this is feature work, and the AMO service bug 302287 should keep our mirrors clean.
Flags: blocking-aviary2.0?
I've got the essentials for this working.

-> me
Assignee: nobody → robert.bugzilla
Created attachment 197355 [details] [diff] [review]
patch - almost there

This works with the testcase and I've also tested success and failure scenarios
while updating multiple extensions some with and some without a hash specified.
I just need to go through all the call sites before requesting review.
Attachment #197313 - Attachment is obsolete: true
Created attachment 197381 [details] [diff] [review]
patch
Attachment #197355 - Attachment is obsolete: true
Attachment #197381 - Flags: review?(benjamin)

Updated

13 years ago
Attachment #197381 - Flags: review?(benjamin) → review+
Fixed on trunk

Checking in mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl;
/cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v 
<--  nsIExtensionManager.idl
new revision: 1.42; previous revision: 1.41
done
Checking in mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--
 nsExtensionManager.js.in
new revision: 1.158; previous revision: 1.157
done
Checking in mozilla/toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <-- 
extensions.js
new revision: 1.79; previous revision: 1.78
done
Checking in mozilla/toolkit/mozapps/extensions/content/update.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/update.js,v  <--  update.js
new revision: 1.19; previous revision: 1.18
done
Checking in mozilla/toolkit/mozapps/extensions/content/update.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/update.xml,v  <--  update.xml
new revision: 1.4; previous revision: 1.3
done
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 8

13 years ago
Is this not 1.8 branch material?
Flags: blocking1.8b5?
(Reporter)

Comment 9

13 years ago
(In reply to comment #8)
> Is this not 1.8 branch material?

See comment 1 -- It's a late feature request that I thought might be too much to
ask for. But yes, if we can get it in 1.8 I'll sleep better at night :-)

Comment 10

13 years ago
Comment on attachment 197381 [details] [diff] [review]
patch

This looks pretty safe to me.
Attachment #197381 - Flags: approval1.8b5?

Comment 11

13 years ago
Dan or Ben, can you please tell us what this patch is for? I'm confused about
how close we are and what we already have or would need to have to make this
useful. 
bug 302284 added crypto hashing support to xpinstall to protect against mirror
network hacks. This only applied to installing extensions but it did not include
updating extensions. This patch adds this for updates as well.
(Reporter)

Comment 13

13 years ago
Comment on attachment 197381 [details] [diff] [review]
patch

>+   * The string Hash for the XPI file. Can be null and if supplied must be in
>+   * the format of "type:hash" (see the types in nsICryptoHash and
>+   * nsIXPInstallManager::initManagerWithHashes).
>+   */
>+  readonly attribute AString  xpiHash;

This comment is true, but then the rest of the code uses an empty string
instead of null and all updates against the current server will fail with a bad
hash (unknown type). I really want hash support, but it has to be an optional
item for compatibility with existing update files/sites (including especially
addons.mozilla.org which has not yet implemented a new update format to support
this).


>+      hashes.push(currItem.xpiHash);
>+        hashes.push(checkboxes[i].hash);

You could change these to 
  hashes.push(currItem.xpiHash ? currItem.xpiHash : null);

Or we could figure that other people will do the same and make xpinstall handle
a blank string as no hash, by changing
http://lxr.mozilla.org/mozilla/source/xpinstall/src/nsXPITriggerInfo.cpp#95
-   if (aHash)
+   if (aHash && *aHash)

I guess Doug and I were thinking that "" might indicate code trying to require
a hash, and that treating a blank string as no-hash would not be as safe a
failure mode in those cases.
Attachment #197381 - Flags: superreview-
Attachment #197381 - Flags: approval1.8b5?
Attachment #197381 - Flags: approval1.8b5-
(Reporter)

Updated

13 years ago
Blocks: 310421
(Reporter)

Comment 14

13 years ago
Filed bug 310421 as a better dupe catcher rather than reopening this one.

Comment 15

13 years ago
a quick question: how did you generate the sha256 hash key? Can I just use
md5sum or sha1sum?
(Reporter)

Updated

13 years ago
Whiteboard: requires 310421 also
(Reporter)

Comment 16

13 years ago
Comment on attachment 197381 [details] [diff] [review]
patch

Quick work on 310421. I'm OK with the rest of this patch so I'll withdraw my
minuses that were based on the regression. But remember to land the other bug
if this goes on the 1.8 branch.
Attachment #197381 - Flags: superreview-
Attachment #197381 - Flags: superreview+
Attachment #197381 - Flags: approval1.8b5?
Attachment #197381 - Flags: approval1.8b5-
(In reply to comment #15)
> a quick question: how did you generate the sha256 hash key? Can I just use
> md5sum or sha1sum?
I used the following http://www.saddi.com/software/sha/ - I forget what I use on
Win32 but I found it via a search
The types that can be used are specified in nsICryptoHash... there is also a
testcase in bug 302284 that uses md5

Updated

13 years ago
Flags: blocking1.8b5? → blocking1.8b5+

Updated

13 years ago
Attachment #197381 - Flags: approval1.8b5? → approval1.8b5+
Fixed on MOZILLA_1_8_BRANCH (included the fix from bug 310421)

Checking in mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl;
/cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v 
<--  nsIExtensionManager.idl
new revision: 1.41.2.1; previous revision: 1.41
done
Checking in mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--
 nsExtensionManager.js.in
new revision: 1.144.2.14; previous revision: 1.144.2.13
done
Checking in mozilla/toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <-- 
extensions.js
new revision: 1.72.2.7; previous revision: 1.72.2.6
done
Checking in mozilla/toolkit/mozapps/extensions/content/update.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/update.js,v  <--  update.js
new revision: 1.17.2.2; previous revision: 1.17.2.1
done
Checking in mozilla/toolkit/mozapps/extensions/content/update.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/update.xml,v  <--  update.xml
new revision: 1.3.4.1; previous revision: 1.3
done
Flags: blocking-aviary2.0?
Keywords: fixed1.8
(Reporter)

Comment 19

13 years ago
(In reply to comment #15)
> a quick question: how did you generate the sha256 hash key? Can I just use
> md5sum or sha1sum?

You can use md5sum or sha1sum, but keep in mind that those are crumbling.

nss 3.10 (available from mozilla.org) contains a fairly cryptic bltest program
that supports the same set of hashes as xpinstall (of course: xpinstall calls nss)
  bltest -H -m sha256 < myfile [or -i myfile]

This outputs binary which is not all that useful. A few pipes help:
bltest -H -m sha256 -i myfile | od -w -t x1 | sed -e "s/^0*//" -e "s/ //g" -e q

(obviously you'd want to put that into a shell script if you were going to do it
regularly)

Comment 20

13 years ago
will this require --enable-crypto?  at this time we cannot build with
--enable-crypto, and I've hacked some stuff to allow us to disable it.  Looking
at the patch it doesn't seem to be an issue, just wanted to ask.

Comment 21

13 years ago
Building the toolkit without crypto  is not supported and I don't really intend
to support it.

Comment 22

13 years ago
(In reply to comment #21)
> Building the toolkit without crypto  is not supported and I don't really intend
> to support it.

That's somewhat contrary to your comment in bug 227792
(Reporter)

Comment 23

13 years ago
(In reply to comment #20)
> will this require --enable-crypto?

crypto is required for the hashes to actually work, but it will build and run
find without the crypto library. If a web-site InstallTrigger or update.rdf
specifies a hash and the crypto libraries are not installed then the install
will fail because it can't verify the hash.
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.