Last Comment Bug 544660 - Crypto hashes and icons are ignored by install buttons
: Crypto hashes and icons are ignored by install buttons
Status: VERIFIED FIXED
:
Product: addons.mozilla.org Graveyard
Classification: Graveyard
Component: Public Pages (show other bugs)
: unspecified
: All All
: -- major
: ---
Assigned To: Scott McCammon [:mccammos]
:
Mentors:
http://www.carbonwind.net/blog/post/F...
Depends on: 545327
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-06 07:08 PST by Giorgio Maone [:mao]
Modified: 2016-02-04 14:51 PST (History)
8 users (show)
stephen.donner: in‑testsuite?
stephen.donner: in‑litmus?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix (1.63 KB, patch)
2010-02-08 16:10 PST, Scott McCammon [:mccammos]
wclouser: review+
Details | Diff | Splinter Review

Description Giorgio Maone [:mao] 2010-02-06 07:08:37 PST
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.
Comment 1 Giorgio Maone [:mao] 2010-02-06 07:36:03 PST
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.
Comment 2 Fred Wenzel [:wenzel] 2010-02-06 07:53:50 PST
(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?
Comment 3 Giorgio Maone [:mao] 2010-02-06 08:03:43 PST
(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 :)
Comment 4 Dave Townsend [:mossop] 2010-02-06 08:40:37 PST
(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.
Comment 6 Scott McCammon [:mccammos] 2010-02-08 16:10:43 PST
Created attachment 425889 [details] [diff] [review]
Fix
Comment 7 Wil Clouser [:clouserw] 2010-02-08 16:21:55 PST
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.
Comment 8 Scott McCammon [:mccammos] 2010-02-08 16:26:58 PST
Patched in r61960
Comment 9 Scott McCammon [:mccammos] 2010-02-09 10:00:21 PST
Preview looks good to me, but someone else should verify.
Comment 10 Stephen Donner [:stephend] 2010-02-09 10:09:59 PST
Giorgio, mind taking a look on preview?  Thanks!
Comment 11 Giorgio Maone [:mao] 2010-02-09 10:24:06 PST
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?
Comment 12 Justin Scott [:fligtar] 2010-02-09 13:56:35 PST
So is this good to be pushed live then? Since the issue is public, I don't think it can wait until next release.
Comment 13 Stephen Donner [:stephend] 2010-02-09 17:12:12 PST
Verified FIXED per comment 11; thanks.

Fligtar: yeah, we can probably local-patch this on prod.
Comment 14 Justin Scott [:fligtar] 2010-02-09 21:47:45 PST
Filed bug 545327 for production patch since clouserw is PTO today and not responding on IRC and reed is getting antsy.
Comment 16 Justin Scott [:fligtar] 2010-02-10 08:45:03 PST
For any onlookers, this fix was pushed to production last night and addons.mozilla.org should no longer have this vulnerability.

Note You need to log in before you can comment on or make changes to this bug.