Closed Bug 1172696 Opened 10 years ago Closed 10 years ago

Extensions contained in signed add-ons with type=32 aren't properly signed

Categories

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

Product:
addons.mozilla.org Graveyard
Component:
Developer Pages
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
2015-08

People

(Reporter: jorgev, Assigned: magopian)

References

(
URL
)

Details

STR: 1) Go to https://addons.mozilla.org/addon/metal-lion-australis-tiger-sp/ 2) Install latest signed version on Nightly. The result is that the internal extensions appear as unverified, even though they appear to be signed. The META-INF directory is there and the files are also present, but for some reason the signature isn't working correctly.
The sub-XPIs are being signed with a key that's only valid for the ID of the top-level XPI. This means that the signature will only be valid for packages where one of the sub-packages has the same ID as the top-level, and even then it will only be valid for that package.
Thanks a lot Kris for spotting the issue, how did you do it? PR: https://github.com/mozilla/olympia/pull/582
Assignee: nobody → mathieu
Blocks: 1070153
We have two choices: a) don't sign these at all (have the developer get each part signed, then submit a package with already-signed add-ons). b) have a special case in the script to sign each sub-add-on with an individual appropriate key
a) is the easiest "fix" on our side, of course
So let's go for solution a) (which will revert what was done in bug 1160969, at least partially). Thanks for the feedback!
I'd vote for: c) Sign sub-add-ons only if they have the same ID as the listing. We already have the working code for signing sub-packages, so it's just a matter of adding a filter. We should also add a warning to the validator if those packages support Firefox and don't have a mozilla.rsa file.
I'm confused, I thought it was not possible to have the same ID for different add-ons?
The individual add-ons all need to have different IDs, but one of them can have the same ID as the listing.
I would personally vote for dropping the support of multi-package XPIs. There's actually a total of 42 add-ons that are (or have been at some point) a multi-package. mysql> select addons.slug, addons.created, addons.modified, count(addons.id) as files_count from addons, versions, files where addons.status != 11 and versions.addon_id = addons.id and files.version_id = versions.id and files.is_multi_package=1 group by addons.id order by modified desc; +--------------------------------+---------------------+---------------------+-------------+ | slug | created | modified | files_count | +--------------------------------+---------------------+---------------------+-------------+ | tangerinefox | 2011-12-28 11:17:59 | 2015-07-04 23:09:24 | 16 | | tangofox | 2010-03-07 08:15:19 | 2015-07-04 23:05:40 | 16 | | charamel | 2009-09-13 06:48:27 | 2015-07-02 01:37:08 | 23 | | silvermel | 2008-06-07 15:29:56 | 2015-06-30 15:20:26 | 30 | | outwit-images | 2008-11-07 11:43:27 | 2015-06-09 08:50:13 | 22 | | outwit-docs | 2009-03-27 23:21:34 | 2015-06-09 08:48:55 | 17 | | outwit-hub | 2008-05-14 13:49:36 | 2015-06-09 08:48:05 | 29 | | noia-20-extreme-v3x | 2007-09-14 11:15:01 | 2015-02-27 09:16:58 | 18 | | add-art | 2008-03-26 18:57:54 | 2015-01-08 15:51:01 | 14 | | metal-lion-sea-monkey | 2014-06-17 07:44:42 | 2014-07-16 03:20:29 | 3 | | metal-lion-silver-sea-monkey | 2014-06-17 07:56:49 | 2014-07-16 03:20:29 | 3 | | noia-fox | 2011-04-02 16:17:12 | 2014-07-16 00:20:29 | 34 | | personal-menu | 2006-11-16 20:58:38 | 2014-05-20 17:10:30 | 1 | | metal-lion-australis-tiger | 2014-04-08 04:13:56 | 2014-05-04 09:15:41 | 7 | | metal-lion-australis-tiger-sp | 2014-04-08 03:32:54 | 2014-05-04 09:13:16 | 8 | | classic-compact | 2006-10-30 05:53:56 | 2014-05-04 06:07:44 | 14 | | metal-lion-australis-graphite | 2014-04-08 04:26:29 | 2014-04-17 01:51:27 | 7 | | gnomerunnerfd-revived | 2014-01-04 06:02:10 | 2014-02-12 02:39:00 | 4 | | gnomerunnergtk-revived | 2013-07-07 07:21:16 | 2014-02-12 02:38:34 | 6 | | classic-tb2 | 2011-09-22 23:57:40 | 2013-11-04 12:49:36 | 1 | | afterglow-1 | 2011-05-29 04:35:22 | 2013-09-07 10:42:05 | 7 | | oldfactory | 2011-06-09 11:20:35 | 2013-09-07 10:40:39 | 7 | | ignore-aero | 2011-07-15 17:09:43 | 2013-07-20 05:36:45 | 4 | | utopia-ffse-white | 2008-06-11 12:29:30 | 2013-05-24 09:35:12 | 3 | | bittorrent-surf-beta | 2013-04-04 14:50:51 | 2013-04-15 12:02:07 | 2 | | oxygen-kde | 2010-01-14 07:10:59 | 2012-11-12 16:08:09 | 8 | | clickreader | 2012-07-25 09:56:06 | 2012-11-09 14:19:34 | 2 | | wokitalk | 2012-02-08 18:31:09 | 2012-06-16 11:42:03 | 2 | | mediapin-pinning-add-on | 2012-04-08 23:31:08 | 2012-04-27 15:20:22 | 1 | | vermisst-add-on | 2012-01-27 12:04:15 | 2012-02-13 11:21:19 | 1 | | mkp-selenium-ide-tools | 2011-12-17 20:22:25 | 2012-01-17 06:20:25 | 1 | | tweequilla-bundle | 2011-01-04 13:11:28 | 2011-12-30 12:00:55 | 1 | | virtual-miracle-package | 2011-12-17 07:46:50 | 2011-12-30 00:20:39 | 1 | | qutextmark-smfb | 2011-10-30 13:18:47 | 2011-11-22 09:20:46 | 1 | | fjdalkfajl | 2011-07-29 14:19:41 | 2011-07-29 14:25:45 | 1 | | splashtop-connect-bundle | 2011-02-17 00:46:41 | 2011-07-05 14:35:29 | 2 | | official-kutless-theme-and-new | 2010-10-19 12:38:57 | 2010-11-02 14:21:09 | 3 | | glydo-related-youtube-videos-g | 2009-09-13 03:38:28 | 2010-08-05 22:33:42 | 1 | | glydo-cnn-addict-get-related-n | 2009-09-13 04:10:38 | 2010-08-05 22:33:42 | 1 | | chromifox-extreme-carbon | 2009-02-24 16:13:13 | 2010-08-05 16:20:10 | 1 | | chromifox-extreme | 2009-02-09 13:34:48 | 2010-08-05 16:15:24 | 5 | | glydo-shopping-save-money-and- | 2009-09-27 05:56:52 | 2010-08-05 16:14:51 | 1 | +--------------------------------+---------------------+---------------------+-------------+ 42 rows in set (0,48 sec) Apart from the 9 top ones, none of those have been updated in the last year. Only three devs are owning the first 7 addons. tangerinefox: latest version isn't a multi-package, only a theme tangofox: latest version isn't a multi-package, only a theme charamel: a theme and the silvermelxt extension, no common guid silvermel: a theme and the silvermelxt extension, no common guid outwit-images: outwit-images and outwit-kernel, no common guid outwit-docs: outwit-docs and outwit-kernel, no common guid outwit-hub: outwit-hub and outwit-kernel, no common guid noia-20-extreme-v3x: a theme and the noia extension, no common guid add-art: adblock-plus, add-art (displays art instead of ads), COMMON GUID So, from the 9 most updated multi-packages, only one might be problematic because it's using the same GUID for the outer (multi-package) XPI and the internal extension. This is problematic because if we don't implement c), then the a) "workaround" wouldn't work: the dev won't be able to submit the inner extension to be signed because the GUID is already used. However, if the internal addon has the same UUID of the outer package, it means it wasn't submitted yet. This means there's nothing preventing the developer to change the GUID (and thus submit it separately to have it signed). I would advise doing the following: 1/ implement a) (don't sign multi-package XPIs) 2/ modify https://developer.mozilla.org/en-US/docs/Multiple_Item_Packaging to explain that inner extensions will need to be signed already (so it means uploading inner extensions for signing if they aren't signed already) 3/ optionally adding a warning to the validator if inner extensions are detected as unsigned :kmag, :jorgev, thoughts?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(jorge)
If we go with this proposal, here's the PR: https://github.com/mozilla/olympia/pull/635
I think the suggested approach in comment #9 is fine given the data you presented.
Flags: needinfo?(jorge)
I'm fine with dropping support, but I think we need to do it properly. The validator will definitely need to warn if an inner XPI is not signed. I think we should go a step farther, though, and make it a hard error if a multi-package XPI is submitted without valid signatures on the inner XPIs, if they support Firefox, anyway.
Flags: needinfo?(kmaglione+bmo)
Blocks: 1190704
Commits pushed to master at https://github.com/mozilla/olympia https://github.com/mozilla/olympia/commit/dcde83ce83cab65fe148790a353900636bf0b9e9 Don't sign multi-packages at all (bug 1172696) https://github.com/mozilla/olympia/commit/6409dac5267856ade34389725ebff5637a1f7431 Merge pull request #635 from magopian/1172696-revert-multi-package-signing-code Don't sign multi-packages at all (bug 1172696)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Hi there, I'm the developer from two of the above mentioned addons, Silvermel and Charamel. How will this impact my addons?
You'll just need to submit your helper extension before you submit your themes, and then include the signed copy in the next submission of your theme.
(In reply to Kris Maglione [:kmag] from comment #15) > You'll just need to submit your helper extension before you submit your > themes, and then include the signed copy in the next submission of your > theme. Ok, thanks!
(In reply to Kris Maglione [:kmag] from comment #15) > You'll just need to submit your helper extension before you submit your > themes, and then include the signed copy in the next submission of your > theme. Could you explain in much greater detail about this? I ask because not only are my themes already signed, but so are the helper extensions bundled with them. Each extension already has its own META-INF package that upon reading relates directly (as you'd expect) to the extension in question. Do I re-submit these? (and get a 'these are already signed' message?) Do I submit them as non-public extensions, so they don't appear aimlessly in the public listings? None of the new procedure is clear to me at all. Thanks.
The helper extensions are currently incorrectly signed with the ID of the listing, rather than of the extension, which means that the signature is invalid, and the extensions will be disabled when mandatory signing is enabled. You'll still need to submit the extensions for review and then repackage the themes once they're signed. You'll need to submit the extensions either as public listings, or as unlisted add-ons with the "side-loaded" review option.
Please move the bug to right milestone when marking it as Resolved so that we know when the fix hit prod. Thanks!
Target Milestone: 2015-06 → 2015-08
(In reply to Kris Maglione [:kmag] from comment #18) > > You'll still need to submit the extensions for review and then repackage the > themes once they're signed. > > You'll need to submit the extensions either as public listings, or as > unlisted add-ons with the "side-loaded" review option. Thanks, that's now clear and I have just now successfully signed my scrollbars.xpi using the same .xpi prior to its original 'signing'.
I'm a bit confused. Do we go with the solution from comment 9 ? I have tried validating a packaged with the inner xpi files not signed and there where no warnings. I have validated a packaged with the inner xpi files signed and in the valdiation results there are the clasic messages that the existing signatures will be replaced. Also i have not seed any info about add-on signing in here: https://developer.mozilla.org/en-US/docs/Multiple_Item_Packaging validation results for a multi packaged add-on with inner xpi signed: http://screencast.com/t/iBQRF70mvGWJ
We did, but the validator changes are a separate bug (bug 1190704).
Verified as fixed in FF40(Win7) in addons-dev.allizom.org The process works as explained in comment 9 except the validation part but we will track that in Bug 1190704 Also I haven't seen anything related to signing process in https://developer.mozilla.org/en-US/docs/Multiple_Item_Packaging Is there a bug where we track that or should we file a new one? Closing bug.
Status: RESOLVED → VERIFIED
Jorge, do you know who's in charge of updating this MDN page to explain the signing specificities for multi-package XPIs?
Flags: needinfo?(jorge)
Everyone and no-one. What do you want added (haven't been following this discussion)?
Flags: needinfo?(jorge) → needinfo?(mathieu)
I believe what we decided is that we would not sign multi-package XPIs at all (nor their content). So developers need to only include already signed extensions. So either they download already reviewed extensions, or if it's their own extension, they first need to upload it and have it signed, and then only include it. There's one limitation to that: they can't have an inner extension with the same GUID as the multi-package XPI because there's a uniqueness constraint on it.
Flags: needinfo?(mathieu)
See Also: → 1205823
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.