Closed Bug 306478 Opened 19 years ago Closed 19 years ago

Extension manager should use xpinstall crypto hashes

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dveditz, Assigned: robert.strong.bugs)

References

Details

(Keywords: fixed1.8, Whiteboard: requires 310421 also)

Attachments

(2 files, 2 obsolete files)

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?
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
Attached patch patch in progress (obsolete) — Splinter Review
Attached file testcase
Attached patch patch - almost there (obsolete) — Splinter Review
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
Attached patch patchSplinter Review
Attachment #197355 - Attachment is obsolete: true
Attachment #197381 - Flags: review?(benjamin)
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
Closed: 19 years ago
Resolution: --- → FIXED
Is this not 1.8 branch material?
Flags: blocking1.8b5?
(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 on attachment 197381 [details] [diff] [review]
patch

This looks pretty safe to me.
Attachment #197381 - Flags: approval1.8b5?
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.
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-
Blocks: 310421
Filed bug 310421 as a better dupe catcher rather than reopening this one.
a quick question: how did you generate the sha256 hash key? Can I just use
md5sum or sha1sum?
Whiteboard: requires 310421 also
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
Flags: blocking1.8b5? → blocking1.8b5+
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
(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)
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.
Building the toolkit without crypto  is not supported and I don't really intend
to support it.
(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
(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.