Skip to content
This repository has been archived by the owner. It is now read-only.

Fix signed extension checks #277

Closed
wolfbeast opened this issue Jan 6, 2018 · 12 comments

Comments

Projects
None yet
3 participants
@wolfbeast
Copy link
Member

commented Jan 6, 2018

Our checks for extension signatures has been broken for a while.

This needs to be fixed up properly, in 2 stages:

  • Revert #251 and get the cert ID in a different location for WE only
  • Rewrite XPIProvider's signing check parts to stop using Mozilla's hard-coded root certs and use normal libJAR signature checking
@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2018

normal libJAR signature checking

Could you please give at least one example of a modern add-on that uses this type of signing and is compatible with either Pale Moon or UXP?

@mattatobin

This comment has been minimized.

Copy link
Member

commented Jan 6, 2018

@JustOff EDIT: There are none anymore... Though, I manually did it the old way and self signed and imported my ca to the store for testing but of course that won't work on Tycho or Moebius.


Excerpt from my forum post:

Pale Moon 26 and below would validate extension signatures according to str8 up CAs in the certificate store. However if the issuer was not known it would allow install of extensions in a valid or tampered with state regardless.

  • Signed Known Issuer (Valid XPI) - Allow Install
  • Signed Known Issuer (Tampered with XPI) - Block install (Add-on is corrupt)
  • Signed Unknown Issuer (Valid and Tampered with XPI) - Allow install

Pale Moon 27 and UXP will ONLY validate extension signatures against a hard coded implementation of AMO's Certificate Authority and ONLY when Add-on Signing is enforced from compile time. Otherwise it is treated as Signed Unknown Issuer as above.

It is noteworthy to add that when Mozilla first started signing Add-ons on AMO for extensions, in Pale Moon 26 and older we had to remove the signatures for edits and forks or else get that "add-on is corrupted" error. I do know that Mozilla signed their entire datastore twice. I can only assume the second time was to resign them to match this hardcoded c++ implemented CA that Pale Moon 27 and UXP (and everything at Mozilla) uses now.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2018

Then, since no one actually uses libJAR compliant signatures, I honestly don't understand why not to just get rid of all this nonsense with signing. All we need is a way to get an unique id for WEs that do not have it in the manifest.

@mattatobin

This comment has been minimized.

Copy link
Member

commented Jan 6, 2018

If we get rid of checking signed extensions.. People will accuse us of being insecure or ignoring security or some other dipshit conclusion.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

Update here: getting the ID-less WebExtensions to behave properly in any other way is currently not possible. XPInstaller has become incredibly fragile and will need to eventually be rewritten.
Checking author-supplied signatures is not possible any longer because Mozilla broke it a long time ago; the only signature checking that is present is against mozilla's hard-coded add-on CA certificates since the usage of NSS was removed from it.

So our current situation is:

  • #251 is required for ID-less WebExtensions to work.
  • Signature checking is enforced as a result, and will complain about any extensions that fail the check (meaning specifically not signed against Mozilla's certificates that are in the private CertDB)
  • Author-supplied JAR signatures can NOT be checked

The only reasonable solution without completely rewriting the extension installer here is to remove the warning about "not-Mozilla-signed" extensions (which includes unsigned, signed by someone else, signed-invalid) in the situation where signing is not marked as required.

We can look at if it's desirable to keep the Mozilla signing check or not; I'm leaning towards simply removing all of it, since all it can do anymore is check against the walled garden signing of Mozilla and it doesn't provide any security benefits.

@wolfbeast wolfbeast added the Wontfix label Jan 9, 2018

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

Neither of the two goals of this issue can be achieved. The warning was removed in 25384fd which is all we can reasonably do right now.

@wolfbeast wolfbeast closed this Jan 9, 2018

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2018

I've been researching this issue for the last two days, so I'd like to make some corrections to the statements above:

  • It's possible to get IDs for ID-less WebExtensions without using Mozilla's hard-coded root certs (I have a working code for this), so we can safely revert #251.
  • With #251 reverted back and ADDON_SIGNING = false the author-supplied signatures verification based on NSS works just fine (I checked this using self-signed cert + CA)

If you are interested, I'm ready to fill the corresponding PR which will revert #251 + 25384fd and provide the solution for ID-less WebExtensions without breaking the libJAR signature checking.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

I must have missed something in my working on this code then, because I was neither able to get IDs for ID-less WebExtensions, nor getting certs from the zip archives otherwise using the available tools (it would simply come back with no cert).

If you've found a way around this or fixing this, then of course we'd be interested.

@mattatobin

This comment has been minimized.

Copy link
Member

commented Jan 9, 2018

This assumes that Mozilla will always use a GUID as the cert add-on id.. I am not sure that is a safe assumption.. Though, this code should work when signing its self is ripped out.. So, if it works.. Go for it.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2018

This assumption is based entirely on Mozilla's own code. And yes, although I checked this myself, it would always be good to get an independent confirmation.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2018

Probably the Wontfix label needs to be removed now.

@wolfbeast wolfbeast added Incomplete and removed Wontfix labels Jan 10, 2018

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

Partially done. It's still incomplete, though, but CBA right now. We have much more important things to worry about first.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.