Closed
Bug 306478
Opened 19 years ago
Closed 19 years ago
Extension manager should use xpinstall crypto hashes
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
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)
3.39 KB,
text/html
|
Details | |
21.61 KB,
patch
|
benjamin
:
review+
dveditz
:
superreview+
asa
:
approval1.8b5+
|
Details | Diff | Splinter Review |
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•19 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?
Assignee | ||
Comment 2•19 years ago
|
||
I've got the essentials for this working. -> me
Assignee: nobody → robert.bugzilla
Assignee | ||
Comment 3•19 years ago
|
||
Assignee | ||
Comment 4•19 years ago
|
||
Assignee | ||
Comment 5•19 years ago
|
||
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
Assignee | ||
Comment 6•19 years ago
|
||
Attachment #197355 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #197381 -
Flags: review?(benjamin)
Updated•19 years ago
|
Attachment #197381 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 7•19 years ago
|
||
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
Reporter | ||
Comment 9•19 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•19 years ago
|
||
Comment on attachment 197381 [details] [diff] [review] patch This looks pretty safe to me.
Attachment #197381 -
Flags: approval1.8b5?
Comment 11•19 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.
Assignee | ||
Comment 12•19 years ago
|
||
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•19 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 | ||
Comment 14•19 years ago
|
||
Filed bug 310421 as a better dupe catcher rather than reopening this one.
Comment 15•19 years ago
|
||
a quick question: how did you generate the sha256 hash key? Can I just use md5sum or sha1sum?
Reporter | ||
Updated•19 years ago
|
Whiteboard: requires 310421 also
Reporter | ||
Comment 16•19 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-
Assignee | ||
Comment 17•19 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? 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•19 years ago
|
Flags: blocking1.8b5? → blocking1.8b5+
Updated•19 years ago
|
Attachment #197381 -
Flags: approval1.8b5? → approval1.8b5+
Assignee | ||
Comment 18•19 years ago
|
||
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•19 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•19 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•19 years ago
|
||
Building the toolkit without crypto is not supported and I don't really intend to support it.
Comment 22•19 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•19 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.
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•