544660 - Crypto hashes and icons are ignored by install buttons

Status: VERIFIED FIXED

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

Description

[Giorgio Maone [:ma1]]

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 [:ma1]]

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]]

(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 [:ma1]]

(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]]

(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 5

[Ben Bucksch (:BenB)]

http://www.carbonwind.net/blog/post/Firefox-Extension-download-process-and-the-mess-within.aspx

Comment 6

[Scott McCammon [:mccammos]]

Attachment: Fix

Comment 7

[Wil Clouser [: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.

Comment 8

[Scott McCammon [:mccammos]]

Patched in r61960

Comment 9

[Scott McCammon [:mccammos]]

Preview looks good to me, but someone else should verify.

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

Giorgio, mind taking a look on preview?  Thanks!

Comment 11

[Giorgio Maone [:ma1]]

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]]

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] Not actively reading bugmail]

Verified FIXED per comment 11; thanks.

Fligtar: yeah, we can probably local-patch this on prod.

Comment 14

[Justin Scott [:fligtar]]

Filed bug 545327 for production patch since clouserw is PTO today and not responding on IRC and reed is getting antsy.

Comment 15

[Reed Loden [:reed]]

http://blog.ivanristic.com/2010/02/firefox-extension-installation-process-vulnerable-to-mitm-attack-.html

Comment 16

[Justin Scott [:fligtar]]

For any onlookers, this fix was pushed to production last night and addons.mozilla.org should no longer have this vulnerability.