The default bug view has changed. See this FAQ.

Crypto hashes and icons are ignored by install buttons

VERIFIED FIXED

Status

addons.mozilla.org Graveyard
Public Pages
--
major
VERIFIED FIXED
7 years ago
a year ago

People

(Reporter: mao, Assigned: mccammos)

Tracking

unspecified
Bug Flags:
in-testsuite ?
in-litmus ?

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 1

7 years ago
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?
(Reporter)

Comment 3

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

Comment 5

7 years ago
http://www.carbonwind.net/blog/post/Firefox-Extension-download-process-and-the-mess-within.aspx
(Assignee)

Comment 6

7 years ago
Created attachment 425889 [details] [diff] [review]
Fix
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+
(Assignee)

Comment 8

7 years ago
Patched in r61960
(Assignee)

Comment 9

7 years ago
Preview looks good to me, but someone else should verify.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: push-needed
Resolution: --- → FIXED
Giorgio, mind taking a look on preview?  Thanks!
(Reporter)

Comment 11

7 years ago
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?
(Reporter)

Updated

7 years ago
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
http://blog.ivanristic.com/2010/02/firefox-extension-installation-process-vulnerable-to-mitm-attack-.html
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.