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

VERIFIED FIXED in 5.0.1

Status

addons.mozilla.org Graveyard
Administration
--
minor
VERIFIED FIXED
10 years ago
2 years ago

People

(Reporter: laura, Assigned: reed)

Tracking

unspecified
5.0.1

Details

(Whiteboard: [sg:want])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 306086 [details] [diff] [review]
patch - v1

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)
(Reporter)

Comment 2

10 years ago
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
(Assignee)

Comment 4

10 years ago
Created attachment 306090 [details] [diff] [review]
patch - v2

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.
(Assignee)

Comment 6

10 years ago
(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
(Assignee)

Updated

10 years ago
Attachment #306090 - Flags: review?(fligtar)
Severity: normal → minor

Updated

10 years ago
Target Milestone: 3.4 → 3.x (triaged)
(Assignee)

Updated

10 years ago
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
(Assignee)

Comment 10

10 years ago
(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+
(Assignee)

Updated

10 years ago
Attachment #306090 - Flags: review?(laura)
(Assignee)

Comment 13

10 years ago
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
Last Resolved: 10 years ago
Keywords: push-needed
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Whiteboard: [sg:want]
I'm waiting for bug 472784 to be fixed before I can test uploading an add-on on preview.
(Assignee)

Comment 16

10 years ago
(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
Keywords: push-needed
status1.9.1: --- → unaffected
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.