Closed Bug 419906 Opened 16 years ago Closed 15 years ago

Upgrade install hashes to use SHA-256 instead of SHA-1

Categories

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

defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: laura, Assigned: reed)

Details

(Whiteboard: [sg:want])

Attachments

(1 file, 1 obsolete file)

Right now the file hashes for addons are SHA-1.  The security review for the Addons Manager suggested we should move to SHA-256 to be consistent with internal standards.

There are two things that need to be done:
1.  Check length of hash column in remora and modify remora.sql if it's not long enough.  SHA-256 hashes are 64 chars long plus any manifests.

2.  There is a script in bin that was previously used to update all the file hashes.  Use this as a starting point, modify it, and run it on the prod db.
Attached patch patch - v1 (obsolete) — Splinter Review
remora.sql has:

`hash` varchar(255) default NULL,

Looks large enough to me.

I took this opportunity to modify update-hashes.php to use database.class.php, too...
Assignee: nobody → reed
Status: NEW → ASSIGNED
Attachment #306086 - Flags: review?(laura)
Comment on attachment 306086 [details] [diff] [review]
patch - v1

Before I grant review: is 'sha256' the string the client is looking for?
3. Update Developer Tools additem script to correctly calculate future hashes

4. Update Admin Tools hash recalculator to correctly calculate hashes
Attached patch patch - v2Splinter Review
Go ahead and change everything.

Note that site/app/webroot/services/install.php will need to be updated when the hashes change. Should also probably modify the test data to use sha256 hashes, too.
Attachment #306086 - Attachment is obsolete: true
Attachment #306090 - Flags: review?(laura)
Attachment #306086 - Flags: review?(laura)
Seems like a better approach would be to store the hashtype and hashvalue in the db instead of hard-coding it everywhere.
(In reply to comment #2)
> (From update of attachment 306086 [details] [diff] [review])
> Before I grant review: is 'sha256' the string the client is looking for?

<reed> Mossop: https://bugzilla.mozilla.org/show_bug.cgi?id=419906#c2 <-- can you answer that? from looking at the code, I'm pretty sure that's right, but want to double check
<Mossop> reed: Yes, looks right
Attachment #306090 - Flags: review?(fligtar)
Severity: normal → minor
Target Milestone: 3.4 → 3.x (triaged)
Attachment #306090 - Flags: review?(fligtar) → review?(fwenzel)
Comment on attachment 306090 [details] [diff] [review]
patch - v2

see comment #5
Attachment #306090 - Flags: review?(laura)
Attachment #306090 - Flags: review?(fwenzel)
Attachment #306090 - Flags: review-
alternatively, we could use the length of the value as well and add something to constants.php to map those to the hashFunction.
-> Cleaning up this Component because we Retire the "add-ons" Component in
Bugzilla 
--> Over to Administration
Component: Add-ons → Administration
QA Contact: add-ons → administration
(In reply to comment #5)
> Seems like a better approach would be to store the hashtype and hashvalue in
> the db instead of hard-coding it everywhere.

We _do_ store the hashtype and hashvalue of each file in the database... It's done under the form "hashtype:hashvalue". My patch changes the default hash type from SHA-1 to SHA-256 and then updates update_hashes.php so that it can be run against the current database to update all the current hashes.

What exactly are you wanting? Some way to change the hash used on the fly via some admin panel? That seems unnecessary and pretty useless.
Comment on attachment 306090 [details] [diff] [review]
patch - v2

Wil and Laura - could you take a look at this?  Think it's ready for primetime?
Attachment #306090 - Flags: review?(laura)
Attachment #306090 - Flags: review?(clouserw)
Attachment #306090 - Flags: review-
Target Milestone: 4.x (triaged) → 5.0.1
Comment on attachment 306090 [details] [diff] [review]
patch - v2

worked well for me
Attachment #306090 - Flags: review?(clouserw) → review+
Attachment #306090 - Flags: review?(laura)
Sending        bin/update-hashes.php
Sending        site/app/controllers/admin_controller.php
Sending        site/app/controllers/components/developers.php
Transmitting file data ...
Committed r21480.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: push-needed
Resolution: --- → FIXED
Whiteboard: [sg:want]
I'm waiting for bug 472784 to be fixed before I can test uploading an add-on on preview.
(In reply to comment #15)
> So does this only affect new and updated addons? Or is it just a tool and
> someone needs to run it? When I check current versions I'm still seeing sha1
> hashes from versioncheck.

Note the "push-needed" keyword. This isn't live yet, and I don't have any ETA on when that might be. You can test using preview.addons.mozilla.org for now.
Verified FIXED for the following couple cases:

[1] NEW add-on: uploaded a test add-on (https://preview.addons.mozilla.org/en-US/firefox/addon/9600), which had:

addonHash="sha256:59cf3deb3d1ce9b721b8d5a2434a0edc8c18c4ac6871ba6476c8df0f0ee587ce"

[2] EXISTING add-on: page source of https://preview.addons.mozilla.org/en-US/firefox/addon/421, as an example is using sha256 as well.
Status: RESOLVED → VERIFIED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: