1201176 - Some add-ons have broken signatures due to missing ID in the CN field

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P2)

Description

[Jorge Villalobos [:jorgev] (he/him)]

If you download and try to install this signed file, you'll be notified that it is unverified:
https://addons.mozilla.org/developers/addon/dangerous-websites-blocker-1/versions/1744154

Mossop identified the problem as a missing add-on ID in the CN field. Other add-ons (by the same developer) are affected, so we'll need to look into all other previously-signed extensions for similar problems.

Comment 1

[Mathieu Agopian [:magopian]]

After some investigation with :jason, it seems the issue is coming either from the trunion server or the signing-clients library.

The only thing that sticks out is that the GUID is kinda long: [e-mail]

Ryan, please let me know if there's anything I can do to help you debug that.

Comment 2

[Jason Thomas [:jason]]

I think we are hitting the CommonName field limit here, which seems to be 64. I've tried generating a CSR using openssl with 65 char CommonName and received error "string is too long, it needs to be less than  64 bytes long".

Comment 3

[Mathieu Agopian [:magopian]]

We tracked down the issue, thanks to :rtilder, and indeed the issue is that the X.509 specification has a 64 chars limit on the CN, and OpenSSL or M2Crypto are silently failing (and not adding a CN) instead of raising an error.

This means we need to enforce the 64 chars limit at AMO's level, during the submission.

We'll also need to do some specific processing (truncate the GUID?) when signing. We currently have 55 add-ons that have GUIDs longer than 64 chars. The query to have them:


If we truncate the GUID when POSTing to trunion for the signature, the CN will contain the truncated addon GUID, will that cause any issues in the client when attempting to install, because of the mismatch between the CN and the GUID in the install.rdf? What do you think :mossop?

Comment 4

[Mathieu Agopian [:magopian]]

The query to have the too long GUIDs from the database: 

select id, slug, guid from addons where length(guid) > 64;

Comment 5

[Dave Townsend [:mossop]]

(In reply to Mathieu Agopian [:magopian] from comment #3)
> If we truncate the GUID when POSTing to trunion for the signature, the CN
> will contain the truncated addon GUID, will that cause any issues in the
> client when attempting to install, because of the mismatch between the CN
> and the GUID in the install.rdf? What do you think :mossop?

Yes, the client expects the full ID to be in the CN field, if it is truncated it will count as a broken signature. We could relax that rule to just a prefix, I don't know that that reduces the security any given that the install.rdf contains the full ID and is signed by the certificate anyway. What do you think Dan?

Comment 6

[Ryan Tilder [:rtilder]]

(In reply to Mathieu Agopian [:magopian] from comment #3)
> We tracked down the issue, thanks to :rtilder, and indeed the issue is that
> the X.509 specification has a 64 chars limit on the CN, and OpenSSL or
> M2Crypto are silently failing (and not adding a CN) instead of raising an
> error.
> 

Just an informational post.  OpenSSL silently fails but reports success.  Bounds checking is performed but doesn't generate an error deep in the bowels of ASN.1 processing 

http://tools.ietf.org/html/rfc5280#appendix-A.1

Look for "-- Upper Bounds" to find the specifics.

> If we truncate the GUID when POSTing to trunion for the signature, the CN
> will contain the truncated addon GUID, will that cause any issues in the
> client when attempting to install, because of the mismatch between the CN
> and the GUID in the install.rdf? What do you think :mossop?

Hexidecimal representations of SHA256 hashes are 64 bytes but I'm not certain if that is would be a good idea.  Losing the (relatively) human readable nature of the GUID may be worse than truncation.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Ryan Tilder [:rtilder] from comment #6)
> > If we truncate the GUID when POSTing to trunion for the signature, the CN
> > will contain the truncated addon GUID, will that cause any issues in the
> > client when attempting to install, because of the mismatch between the CN
> > and the GUID in the install.rdf? What do you think :mossop?
> 
> Hexidecimal representations of SHA256 hashes are 64 bytes but I'm not
> certain if that is would be a good idea.  Losing the (relatively) human
> readable nature of the GUID may be worse than truncation.

I don't know that it matters much. It's helpful to me when debugging why add-on signatures seem to be wrong but I can get a hash of an ID pretty easily anyway. So this might be a decent option but we probably want to use it only in the case where the ID is too long otherwise we're going to have the problem that we'd have to resign everything again and existing Firefox releases would reject the new signatures.

So that means that we'd be accepting two valid CN values for an add-on. Again I'd like Dan to weigh in on the consequences of that.

Comment 8

[Mathieu Agopian [:magopian]]

Just fyi, here's some numbers (download counts for those addons over a week, and last modified time):

mysql> select addons.id, addons.modified, sum(download_counts.count) as downloads_last_week from addons, download_counts where length(addons.guid) > 64 and download_counts.addon_id=addons.id and download_counts.date between '2015-08-26' and '2015-09-01' group by addons.id;
+--------+---------------------+---------------------+
| id     | modified            | downloads_last_week |
+--------+---------------------+---------------------+
| 329794 | 2014-10-26 22:39:54 |                 202 |
| 360544 | 2013-04-17 14:20:25 |                  61 |
| 364578 | 2012-05-02 10:52:58 |                  10 |
| 422062 | 2013-02-16 09:30:02 |                   6 |
| 457543 | 2014-09-12 07:20:30 |                  81 |
| 464617 | 2013-11-11 18:20:26 |                   1 |
| 611078 | 2015-08-22 11:20:27 |                  23 |
+--------+---------------------+---------------------+
7 rows in set (0,84 sec)

(Didn't want to paste slugs or guids in this bug which is public, just in case). There's only 7 addons, which means that (if my request is correct) only those 7 addons (out of the 55 problematic ones) had any downloads during the last week.

I've heard that there might be a way to change an addon's guid if needed, and still serve updates to them, and that :jorgev might know how to do that.

So another solution than using a sha256 and needing client side modifications could be to
1/ prevent the submission of addons with a GUID which is longer than 64 chars (to not have this issue anymore in the future)
2/ contact the addon authors to ask them if their addon is still maintained, if they would mind res-submitting their addon with a shorter guid
3/ for the problematic cases, have a specific hack to still serve updates to those addons even if the guid has changed.

Also, for what it's worth, truncating the current GUIDs to 64 for those 55 addons yields no duplicates.

Thoughts?

Comment 9

[Dave Townsend [:mossop]]

(In reply to Mathieu Agopian [:magopian] from comment #8)
> Just fyi, here's some numbers (download counts for those addons over a week,
> and last modified time):
> 
> mysql> select addons.id, addons.modified, sum(download_counts.count) as
> downloads_last_week from addons, download_counts where length(addons.guid) >
> 64 and download_counts.addon_id=addons.id and download_counts.date between
> '2015-08-26' and '2015-09-01' group by addons.id;
> +--------+---------------------+---------------------+
> | id     | modified            | downloads_last_week |
> +--------+---------------------+---------------------+
> | 329794 | 2014-10-26 22:39:54 |                 202 |
> | 360544 | 2013-04-17 14:20:25 |                  61 |
> | 364578 | 2012-05-02 10:52:58 |                  10 |
> | 422062 | 2013-02-16 09:30:02 |                   6 |
> | 457543 | 2014-09-12 07:20:30 |                  81 |
> | 464617 | 2013-11-11 18:20:26 |                   1 |
> | 611078 | 2015-08-22 11:20:27 |                  23 |
> +--------+---------------------+---------------------+
> 7 rows in set (0,84 sec)
> 
> (Didn't want to paste slugs or guids in this bug which is public, just in
> case). There's only 7 addons, which means that (if my request is correct)
> only those 7 addons (out of the 55 problematic ones) had any downloads
> during the last week.
> 
> I've heard that there might be a way to change an addon's guid if needed,
> and still serve updates to them, and that :jorgev might know how to do that.
> 
> So another solution than using a sha256 and needing client side
> modifications could be to
> 1/ prevent the submission of addons with a GUID which is longer than 64
> chars (to not have this issue anymore in the future)
> 2/ contact the addon authors to ask them if their addon is still maintained,
> if they would mind res-submitting their addon with a shorter guid
> 3/ for the problematic cases, have a specific hack to still serve updates to
> those addons even if the guid has changed.
> 
> Also, for what it's worth, truncating the current GUIDs to 64 for those 55
> addons yields no duplicates.
> 
> Thoughts?

How many of those even work with current Firefox? I can also make the add-ons manager reject add-ons with long IDs.

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Mathieu Agopian [:magopian] from comment #8)
> I've heard that there might be a way to change an addon's guid if needed,
> and still serve updates to them, and that :jorgev might know how to do that.

My understanding is that it's not easy nor reliable, so we shouldn't do this.

As for forbidding long IDs to try to get rid of this problem in the future, I think it would be okay to forbid it for new add-ons. We can always use the admin override if an old unlisted add-on is submitted and fails for this reason.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Dave Townsend [:mossop] from comment #7)
> So that means that we'd be accepting two valid CN values for an add-on.
> Again I'd like Dan to weigh in on the consequences of that.

I don't think we need to accept two valid CN values. We know what ID we're expecting based on install.rdf, so for IDs that are 64 chars or shorter, we can require that they not be hashed, and for IDs that are longer, we can require that they are hashed. Any given ID would still have only one valid CN value. And given that a 64-character hex string is not a valid add-on ID, a given CN value would only have one valid corresponding add-on ID (ignoring hash collisions).

(In reply to Jorge Villalobos [:jorgev] from comment #10)
> My understanding is that it's not easy nor reliable, so we shouldn't do this.

It is reliable, and is easy enough so long as the add-on isn't hosted on AMO. If it is hosted on AMO, it can be done, but requires manual intervention from staff.

Comment 12

[Andy McKay]

magopian will file a bug and create a pull request for preventing the submission of add-ons with a long GUID.

Comment 13

[Daniel Veditz [:dveditz]]

Changing the cert to a hash of the ID would be secure, but would require re-signing everything and a product break where every add-on is either compatible with future versions or past versions, but never both. That's a no-go.

I guess we have two possible solutions.

a) Dave's proposal in comment 5 means trunion doesn't have to change and the client change is minimal. Client change magically fixes existing addons without re-signing. Requires work in AMO to make sure we never have IDs that collide in the first 64 characters. If we never ever issue certs to anyone other than ourselves (my original proposal assumes that we would issue certs to "partners") then it's not that big a deal if the AMO enforcement is not changed right away: collisions aren't that terrible if we're doing the signing.

b) Kris' proposal in comment 11 requires changes in trunion and slightly more involved client changes (but not terrible). Also requires re-signing the 55 add-ons with over-long IDs, or at least the 7 that people actually download. No AMO changes required.

Of the two Kris' proposal is more secure, or less likely to have its baseline requirements be violated in the future. All the rules are enforced in code. Dave's proposal is more expedient and "secure enough" as long as we do all the signing, but will be painful to change in the future if we ever want to issue certs. I'm reluctant to preclude that option.

Comment 14

[Dave Townsend [:mossop]]

(In reply to Daniel Veditz [:dveditz] from comment #13)
> Changing the cert to a hash of the ID would be secure, but would require
> re-signing everything and a product break where every add-on is either
> compatible with future versions or past versions, but never both. That's a
> no-go.
> 
> I guess we have two possible solutions.
> 
> a) Dave's proposal in comment 5 means trunion doesn't have to change and the
> client change is minimal. Client change magically fixes existing addons
> without re-signing.

The existing add-ons have an empty CN not a truncated one. Now it's true that a blank string is the prefix of any ID but I think we want to get the existing add-ons resigned with at least something in there that narrows the possible set of IDs they match.

Comment 15

[Daniel Veditz [:dveditz]]

actually both require changes to trunion since we're currently getting a blank CN--Dave's proposal already assumed a change to truncate the IDs in the signature (either trunion, or the post to trunion). Given that I think we should go with Kris' proposal which is more defensible on technical grounds.

Comment 16

[Andy McKay]

We've got 55 add-ons with the large IDs and magopian has changed AMO to block any more add-ons with large IDs (bug 1202016). Maintaining code in the client forever to check if an add-on ID is large and hash it seems like just a little bit more technical debt we'll have to forever carry around in the client. I'm leaning towards the solution in comment 5, just check the prefix matches. But I'll let :mossop decide on that point.

Comment 17

[Ryan Tilder [:rtilder]]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> actually both require changes to trunion since we're currently getting a
> blank CN--Dave's proposal already assumed a change to truncate the IDs in
> the signature (either trunion, or the post to trunion). Given that I think
> we should go with Kris' proposal which is more defensible on technical
> grounds.

The blank CN is due to an odd OpenSSL bug.  Even though it has bounds checking on the CN value, it still returns success from the ASN.1 functions that do the checks and changes to the X.509 subject.  Trunion should check for that particular issue but it makes me worry more than a bit about the remaining checks in OpenSSL's ASN.1 logic.  The other work is server side but in the olympia module.  I'm fairly certain he's already on that.

Comment 18

[Dave Townsend [:mossop]]

(In reply to Andy McKay [:andym] from comment #16)
> We've got 55 add-ons with the large IDs and magopian has changed AMO to
> block any more add-ons with large IDs (bug 1202016). Maintaining code in the
> client forever to check if an add-on ID is large and hash it seems like just
> a little bit more technical debt we'll have to forever carry around in the
> client. I'm leaning towards the solution in comment 5, just check the prefix
> matches. But I'll let :mossop decide on that point.

If we want these 55 add-ons to work (is that established? How many of these add-ons actually work with recent Firefoxes?) then we're going to have to do some special code in the client for the long ID case regardless of which solution we take. Hashing an ID doesn't take much more code than getting its prefix so I'd go for the hash based on Dan's assessment. So we need a bug to update the client code and a bug to fix the signing for those add-ons.

Comment 19

[Kris Maglione [:kmag]]

It turns out that there are a lot of IDs longer than 64 characters in the wild. 351 based on an FHR query in a two week window.

Of these, *most* follow the pattern:

{uuid}@{different-uuid}.com

And the vast majority of those seem to be foistware. There are also several which follow one of:

{30-char-hex-string}@{different-30-char-hex-string}.com
{40-char-hex-string}@builder.extensionfactory.com

The following are the only IDs I see that don't follow the first pattern:

>= ~1000 users:

backupfox_959a5970_ada3_11e0_9f1c_0800200c9a66@mozillafirefoxextension	BackupFox
[e-mail]	Le Figaro
[e-mail]	Dangerous Websites Blocker
[e-mail]	Le Parisien
[e-mail]	Virtual Keyboard

< ~1000 users:

[e-mail]	Searcher's Family
close-other-tabs-by-double-click@04a5b2a8-2d53-43d5-b3cc-b2ca846f0b9d	Close Other Tabs by Double-Click
[e-mail]	Askmebuy Easy Buy
[e-mail]	Nouvelobs
[e-mail]	Cinema Now
firefox-password-dialog-begone@8ece4de4-738c-11e2-ba91-14dae923a2d4	Password dialog begone
jid0-uvgxcdfhdhfgmfgjfgjzdfgd651zfvger64df9wiyymus6Mky9LfJJ0nGas@jetpack	PoliceWEB.net
[e-mail]{ec8030f7-c20a-464f-9b0e-13a3a9e97384}	(no name)

Comment 20

[Andy McKay]

I can see that a long ID a small aid to malicious add-ons so they can easily change their IDs, but it will happen anyway. There sounds like some legit add-ons there and more than the ones we have in the queue now.

There was an IRC chat and it sounds like we all like the idea of the hash of IDs more than 64 chars. If no-one objects, let's file the bugs to complete that work. When its completed we can maybe remove the block on uploading.

Comment 21

[Kris Maglione [:kmag]]

Filed bug 1203365 for the AMO side.

Comment 22

[Andy McKay]

Filed bug 1203787 for the Firefox side.

Comment 23

[Mathieu Agopian [:magopian]]

Filed bug 1203915 to re-allow the submission of long GUIDs once bug 1203365 and bug 1203787 are implemented.

Comment 24

[Andy McKay]

Filed bug 1210032 to remind us to flip the waffle switch. There's not much else to do on this tracking bug, closing.