Closed Bug 544660 Opened 14 years ago Closed 14 years ago

Crypto hashes and icons are ignored by install buttons

Categories

(addons.mozilla.org Graveyard :: Public Pages, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ma1, Assigned: smccammon)

References

()

Details

Attachments

(1 file)

Current AMO buttons are not passing the "Hash" and the "IconURL" parameters to the InstallTrigger.
This is because they assume the event target for a click is the anchor element which contains these parameters as HTML attributes, while the real target is the inner span element.

This is easily demonstrated by running the following bookmarklet on https://addons.mozilla.org/en-US/firefox/addon/722 :

javascript:InstallTrigger._i = InstallTrigger._i || InstallTrigger.install; InstallTrigger.install = function() { this._i.apply(this, arguments); alert(arguments.callee.caller + "\n\nOuch, InstallTrigger parameter is:\n" + arguments[0]["NoScript"].toSource() + "!\n\nNo surprise, target was:\n" + arguments.callee.caller.arguments[0].target); };alert('Please click the "Add to Firefox" button')

This is a very important issue because, since add-on XPIs are served from unsecured mirrors, the only integrity check is the crypto hash which is currently unused.
BTW, I'm quite worried by the fact this bug couldn't be detected earlier because there was no hint at it (aside the missing icons for first time installations).

Maybe installation should fail noisily if the download URL is insecure and/or redirected and no hash has been provided.
(In reply to comment #1)
> BTW, I'm quite worried by the fact this bug couldn't be detected earlier
> because there was no hint at it (aside the missing icons for first time
> installations).

Agreed: This definitely needs a test in Zamboni.

> Maybe installation should fail noisily if the download URL is insecure and/or
> redirected and no hash has been provided.

That's a client improvement then, right? Not something we can do on the AMO side?
(In reply to comment #2)
> Agreed: This definitely needs a test in Zamboni.

Of course, unit testing the install process will help specifically on AMO. But I was thinking about the general case.

> > Maybe installation should fail noisily if the download URL is insecure and/or
> > redirected and no hash has been provided.
> 
> That's a client improvement then, right? Not something we can do on the AMO
> side?

You're right. This part needs a separate bug, if deemed worth.
AMO install buttons need to be fixed ASAP, instead :)
(In reply to comment #3)
> > > Maybe installation should fail noisily if the download URL is insecure and/or
> > > redirected and no hash has been provided.
> > 
> > That's a client improvement then, right? Not something we can do on the AMO
> > side?
> 
> You're right. This part needs a separate bug, if deemed worth.
> AMO install buttons need to be fixed ASAP, instead :)

The client bug is basically bug 310355 I think.
Attached patch FixSplinter Review
Assignee: nobody → smccammon
Status: NEW → ASSIGNED
Attachment #425889 - Flags: review?(clouserw)
Comment on attachment 425889 [details] [diff] [review]
Fix

Please land on preview!

Giorgio:  Once this lands, can you check https://preview.addons.mozilla.org/en-US/firefox/addon/722 ?  Make sure to do a hard-refresh to get the new JS.
Attachment #425889 - Flags: review?(clouserw) → review+
Patched in r61960
Preview looks good to me, but someone else should verify.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: push-needed
Resolution: --- → FIXED
Giorgio, mind taking a look on preview?  Thanks!
Looks fine to me.

I checked both "white box", with the original bookmarklet (both Hash and IconURL are present) and "black box", with the following bookmarklet which alters the hash attribute:

javascript:ib = $(".install-button a"); hash = ib.attr("addonHash"); ib.attr("addonHash", hash.replace(/\d/g, 'a')); alert("Try to install now")

As expected, installation failed with the "invalid hash" error.
I tried both mouse click and keyboard only navigation, and both seem to work correctly.

A curiosity of mine: how are you going to unit-test this, per comment #2?
Flags: in-testsuite?
Flags: in-litmus?
So is this good to be pushed live then? Since the issue is public, I don't think it can wait until next release.
Verified FIXED per comment 11; thanks.

Fligtar: yeah, we can probably local-patch this on prod.
Status: RESOLVED → VERIFIED
Filed bug 545327 for production patch since clouserw is PTO today and not responding on IRC and reed is getting antsy.
Depends on: 545327
Keywords: push-needed
Hardware: x86 → All
For any onlookers, this fix was pushed to production last night and addons.mozilla.org should no longer have this vulnerability.
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: