276006 - modified Signed extensions appear to install (but don't). Report the error

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Alex Sirota (iosart)]

The tests here:
http://www.mozilla.org/projects/xpinstall/signed/testcases/index.html fail.

Meaning that an original signed extension appears signed, and so does the
altered version. 

Another test - download a signed extension from here:
http://www.j-maxx.net/abtrans/abextension.php, modify a file inside the
extension keeping the cert. (metadata) intact. The extension still appears signed.

IMHO this is a serious security bug - a user can be installing a malware
believing it is signed by an entity he/she trusts.

Comment 1

[Alex Sirota (iosart)]

I tested this a bit further, and it seems that this is an extension manager bug
with the latest FF nightly.

If you press "Install" it actually says "Install success". 
After you restart FF, the extension appears in EM, but without its full name -
only the filename that was downloaded. 

The extension seems -not- to be installed - it just sits there. So, it now looks
more like an EM bug than a security one. 

Comment 2

[Daniel Veditz [:dveditz]]

firefox 1.0 does the right thing (almost). A dialog box appears saying it
couldn't _download_ the file (wrong, it downloaded) because "Signing not
verified" (correct). While the alert is showing the extension appears in the EM
dialog behind as "Install Success" (wrong), but disappears from the EM dialog
when you click OK on the error alert (correct in the end).

Over to the Firefox product, it's the front end that's wrong.

Comment 3

[Daniel Veditz [:dveditz]]

Clearing the security flag so more people who could fix the UI might see it.

Comment 4

[Robert Strong (they/them - no direct email)]

When installing the extension from http://www.j-maxx.net/abtrans/abextension.php
it displayed as signed and installed successfully when unmodified. I then
downloaded it, modified the install.rdf inside the xpi, and tried to install it.
The ui showed as signed and the extension failed to install with an alert stating:
Deer Park could not download the file at
<path to file>/albhed-0.1.2.xpi
because: Signing could not be verified.

This was reported to the js console:
Signature Verification Error: the signature on install.rdf is invalid because
the MANIFEST.MF file does not contain a valid hash of the file being verified.

The extension was also removed from the ui after it failed to install.

This is WFM but I am going to hold off on resolving since this may not be
entirely the desired behavior though I believe it is close enough. Also, I
suspect that the patch from bug 286034 may have fixed this bug.

Comment 5

[Daniel Veditz [:dveditz]]

This is basically correct behavior. The only remaining problem is unrelated to
signing: when there's an installation error the message starts out "Firefox
could not download the file at" when it's quite often not a download error.

Would be better to say "Firefox could not install the file", and for the few
times it is a download problem finish with "because: download error". (Right now
it will say it could not download the file because of download error, which is
annoying.) The message issue is separate and probably already filed, although I
couldn't find one.

The string is
http://lxr.mozilla.org/mozilla/source/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties#64

The code is
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/content/extensions.js#361

The code doesn't handle status 999 correctly -- that's not an install failure,
that's a success with restart required to finish replacing in-use files.

Comment 6

[Robert Strong (they/them - no direct email)]

Thanks Daniel - I have a couple of other patches in progress including one that
handles the status 999 and will fix this after the freeze is lifted.

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: patch

I split this out from another patch. Per bsmedberg I changed
errorInstallMessage to errorInstallMsg to make it easier for translators to see
there is a change. I test for 999 instead of just going with all errors < 0
since there may be other errors above 0 that should be displayed though I
suspect this is unlikely.

Comment 8

[Robert Strong (they/them - no direct email)]

I doubt that a safer patch could be found to fix the remaining issues so
nominating for 1.8b2

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 184718 [details] [diff] [review]
patch

Benjamin - this is a very simple / safe patch that fixes the remaining issues
as pointed out by dveditz and with blockers that were just added for 1.8b2 I
think that it may be appropriate to land this.

Comment 10

[chris hofmann]

Comment on attachment 184718 [details] [diff] [review]
patch

a=chofmann,  lets get this in quick

Comment 11

[Benjamin Smedberg]

Fixed on trunk for 1.1a1.

Comment 12

[Mike Shaver (:shaver emeritus)]

Comment on attachment 184718 [details] [diff] [review]
patch

I wish we had constants for the '999' that meant something, or perhaps even a
comment, but a=shaver nonetheless.

Comment 13

[Robert Strong (they/them - no direct email)]

*** Bug 296954 has been marked as a duplicate of this bug. ***