302287 - Add crypto hashes for extensions

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Administration, defect, P1)

Description

[Daniel Veditz [:dveditz]]

To deal with potentially vulnerable mirror networks the Update system needs to
add a crypto hash of extensions to the database. When it generates each
extension page it would add this hash to the InstallTrigger.install() call.

The exact syntax will depend on the client implementation, bug 302284.

Comment 1

[Daniel Veditz [:dveditz]]

Bug 302284 has been fixed. The install syntax change adds an optional "Hash"
property which is a string of the format "<type>:<hexchars>".

The change would be simply:
- add the type:hash string to the database of extensions
- add an extra argument ("aHash") to the onclick call to install()
- modify function install() to add the line "Hash: aHash," to where
  it builds the params Array

An example hash string is "sha1:4ea112f70572759388e00203f505a7935839d68c".

Older versions of Firefox and the suite will simply ignore the unknown Hash
property. It is safe to add always without version sniffing.

In the implementation that was checked in a blank string is an invalid hash and
will result in an error. For packages that don't yet have a hash in the database
you will have to use null for the Hash property or omit it altogether.   Easily
done by passing null as the extra arg to the install() call, or changing the
Hash-setting line given above to "Hash: (aHash) ? aHash : null,"

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: sample update RDF

Bug 306478 which checks the updateURL for the xpi's hash is fixed on trunk and
attached is a sample update RDF.

Comment 3

[Michael Morgan [:morgamic]]

Dan, I'd like to make good on this bug and get it done -- should we generate the hash when the user uploads the extension, or does the hash need to be a value in install.rdf?

If it isn't in install.rdf, then I'm assuming the client generates it dynamically as well?

Whether or not it exists in any form in the install.rdf is the main question I have at this point.

So we're clear, here is my plan of attack:
1) add the hash column to the `version` table
2) add the hash generation in additem.php for when developers submit an extension
3) add the hash to the update RDF so clients can see it

Sorry it's been so long, I apologize for the delay.

Comment 4

[Robert Strong (they/them - no direct email)]

Hi Mike, the hashes are never in the install.rdf - they are only in the update rdf datasource and the call to InstallTrigger for installation.

There are samples of the syntax in attachment 197658 [details] and the testcase in bug 302284 (attachment 192917 [details])

Comment 5

[Michael Morgan [:morgamic]]

Sounds good -- thanks Robert.

Comment 6

[Michael Morgan [:morgamic]]

SQL for adding hash to version table:

ALTER TABLE `version` ADD `hash` VARCHAR( 255 ) AFTER `Size` ;

Any objections to storing type:hash in one field?  If not, I'll put in the request for getting this field added to production.

Comment 7

[Robert Strong (they/them - no direct email)]

I'd prefer it in one field. If there is ever the need to check the hash type or hash value it can still be done with a query and the main use will be using these the two concatenated.

Comment 8

[Michael Morgan [:morgamic]]

Sounds good.  Another question was what type of hash to use by default when addons are submitted to AMO.  I had originally thought about letting developers pick a hashtype, but it's not something they really need to determine.

So the question then becomes -- if we are going to generate and store a hash when extensions are uploaded, what type of hash should it be?

So mconnor and I discussed that, and he said, "sha256" pretty quickly -- but the server doesn't have built-in support for generating a sha256.

Would using sha1 as the default hashtype be out of the question?

Comment 9

[Robert Strong (they/them - no direct email)]

I defer to dveditz and mconnor as to whether using sha1 is out of the question. As for support for sha256 the following is what I used (note: sha256 worked fine - I had trouble getting sha384 and sha512 to work but did not have time to track down why):
http://www.saddi.com/software/sha/

Comment 10

[Michael Morgan [:morgamic]]

Well, the db field is there so I'm going to go ahead and attempt to implement with sha1 and see how that works.  PHP has built-in sha1 sum functions (like sha1_file()) that we can use to get the sum for the .xpi when a developer is uploading an update/new add-on.

Comment 11

[Michael Morgan [:morgamic]]

Attachment: Modifications to v1 additem script to insert sum information.

This should check the uploaded .xpi, gather the sha1 (only this for now) and insert the value in the 'type:sum' format in the hash column for `version`.

Additional changes need to happen for the public (v2) side to show the hash value in the update script.

Comment 12

[Michael Morgan [:morgamic]]

Attachment: Changes to v2 update script and template for updateHash addition.

These changes should:
* update the query to return the hash value from the version table
* show the updateHash attribute if it exists
* not show the updateHash attribute if it doesn't exist in the db (is this desired?)

Comment 13

[Michael Morgan [:morgamic]]

Attachment: Script to grab and update file hashes based on database contents (the uri for each version).

This should let us update existing entries with their URI -- might need an adjustment to the repo path, depending on where it is run.

Comment 14

[Michael Morgan [:morgamic]]

Attachment: Combined patch for update rdf, installTrigger, and accompanying template changes.

Updated patch to cover the installTrigger stuff.  As-is, it won't support themes, that depends on changes to the client.

Comment 15

[Daniel Veditz [:dveditz]]

> As-is, it won't support themes, that depends on changes to the client.

Would it make sense to change installChrome() now, in advance of client changes? Might reduce the overhead of making this kind of change twice, and an extra parameter isn't going to hurt in the meanwhile.

If you're interested, the syntax would be the same string as the Hash property in the install() arguments: "sha1:22309482398434982304209283409843"

Comment 16

[Mike Shaver (:shaver emeritus)]

Comment on attachment 227300 [details] [diff] [review]
Script to grab and update file hashes based on database contents (the uri for each version).

>+ * The purpose of this document is to perform periodic tasks that should not be
>+ * done everytime a download occurs in install.php.  This should reduce
>+ * unnecessary DELETE and UPDATE queries and lighten the load on the database
>+ * backend.

Why not do this on upload of extension, instead of redoing them all periodically?

>+$db->query("SELECT name,type,vid,uri FROM main m INNER JOIN version v ON v.id = m.id WHERE m.id=735", SQL_ALL, SQL_ASSOC);

I don't think the WHERE clause here is what we want.

Comment 17

[Mike Shaver (:shaver emeritus)]

Comment on attachment 227303 [details] [diff] [review]
Combined patch for update rdf, installTrigger, and accompanying template changes.

r=shaver

Comment 18

[Mike Shaver (:shaver emeritus)]

Comment on attachment 227274 [details] [diff] [review]
Modifications to v1 additem script to insert sum information.

Assuming that check_filename is going to make sure things can't get out of the place where the stuff is kept, r=shaver.

Comment 19

[Michael Morgan [:morgamic]]

Comment on attachment 227300 [details] [diff] [review]
Script to grab and update file hashes based on database contents (the uri for each version).

This is just a small script that can be used to update hashes for existing addons that don't have hashes, in case we would want to retro-actively add hash values for these items.  It's not something that would get cron'd -- the comments were from maintenance.php, and I'll remove them since they are misleading.

Comment 20

[Michael Morgan [:morgamic]]

Attachment: New retro-active hash generation patch with clearer comments and better checking.

Comment 21

[Mike Shaver (:shaver emeritus)]

Comment on attachment 227712 [details] [diff] [review]
New retro-active hash generation patch with clearer comments and better checking.

Do we not want to put the hashes in for the unapproved ones as well?  Seems like we might as well, though it's not a big deal.

r=shaver

Comment 22

[Michael Morgan [:morgamic]]

Yeah, we could adjust the path and the WHERE clause if/when we decide to run it.  They are all in different directories so in this case I didn't want to repeatedly check for files I knew weren't there (non-approved files are in /approval not /ftp).

Comment 23

[Michael Morgan [:morgamic]]

Code is checked in, waiting for update in bug 343225.

Comment 24

[Giorgio Maone [:ma1]]

Attachment: Fixes install.js fatal bug introduced by attachment #227303

The install.js patch was broken because of a bad property reference, which caused the script to abort before reaching install trigger if extHash was present (e.g. AdBlock, currently).
This patch fixes it.

Comment 25

[Mike Shaver (:shaver emeritus)]

Comment on attachment 227828 [details] [diff] [review]
Fixes install.js fatal bug introduced by attachment #227303 [details] [diff] [review]

r=shaver

Comment 26

[Mike Shaver (:shaver emeritus)]

Fix committed, requesting update of live.  Thanks, Giorgio, and sorry for missing it on review.  I'm supposed to know JS!