1159055 - tampered XPIs with old-style signatures accepted as "unsigned" rather than "corrupt"

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Mark Straver]

As a follow-up to bug# 1158467 I'm filing this bug because I noticed an issue with signature checking of XPI files when trying to test a theory of the indirect checking of a hash of the manifest in the .sf being the only thing needed to prevent tampering.

It seems that in FF37 at least, checking of signed XPI files simply doesn't work and the browser accepts extensions that have been signed but have been changed since. I tested a 3rd-party signed XPI file (Adblock Plus) that follows the JAR signing specification. I can freely edit any file in the extension and it will not complain. The same edited extension gives a "corrupt" error in older code bases and refuses to install. This is a regression; I don't have the time right now to do a regression test and I'm not intimately familiar with the code in question, maybe someone else can find the window, but a potential candidate might be bug# 1050518 as a guess?

Comment 1

[Dave Townsend [:mossop]]

We have some automated tests verifying signature checks so this is surprising and I couldn't reproduce by adding a newline to the defaults/prefs.js file in adblock plus. Can you be more specific about what you changed that didn't get noticed or provide an example XPI?

Comment 2

[Mark Straver]

Attachment: Adblock Plus, author signed, original

This is the original (unedited) Adblock Plus extension that I've used for testing. It installs on older code bases and on FF37

Comment 3

[Mark Straver]

Attachment: Adblock Plus, Author signed, with edited default prefs js

This is the same extension with an added newline at the top of the default preferences file (present in manifest and .sf)

Blocked in older code bases, as expected. FF37 happily installs this one as well without complaining.

Comment 4

[Mark Straver]

Observations:

FF37 on the original file complains:
Signature Verification Error: the signature on this .jar archive is invalid because the digital signature (*.RSA) file is not a valid signature of the signature instruction file (*.SF).

This is incorrect because the RSA is indeed valid for the SF.

Older code base: No error is printed.

So it seems current FF erroneously sees an incorrect RSA file, and as a result allows the installation unchecked (I'd have expected this to be blocked, instead).

If I edit the SF file, the above error is given in both browsers tested, which I would expect, but both allow installation as well (which would be incorrect).

if I edit a listed file in the .sf invalidating the hash, FF37 ignores the error and simply installs. Older code prints the following error:
"Signature Verification Error: the signature on defaults/preferences/adblockplus.js is invalid because the MANIFEST.MF file does not contain a valid hash of the file being verified." which I would expect.

If I edit both SF and the .js mentioned, all browsers print the RSA error and silently allow installation.

So, there's 2 errors here:

1) editing a listed file no longer blocks installation in FF37 while it used to
2) editing the SF and invalidating the signature as a result bypasses further checking completely in all versions of the code with just an informational message printed in the console.

Comment 5

[Dave Townsend [:mossop]]

If I recall correctly there are two different failure cases. If something is wrong with the sf or rsa files then it counts as a bad signature and Firefox should just say that the author is not verified. If the files in the XPI don't match the manifest then we consider the file corrupt and refuse to install. It looks like that latter case has stopped happening and now we just say author not verified for your example add-on in current versions. Not ideal but at least we're not claiming the XPI is signed when something is clearly broken. I'll try to narrow down when this changed.

Comment 6

[Dave Townsend [:mossop]]

The change occurred somewhere in here: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e86b84998b18&tochange=a19e0434ea52

Comment 7

[Dave Townsend [:mossop]]

Bug 1028643 seems a likely candidate

Comment 8

[Daniel Veditz [:dveditz]]

Ugh, that means we now have to worry about bug 1049686 for signed add-ons as well. Before signatures didn't expire, which while not great still meant we could revoke them (at least in the new-signing case). If we don't fix bug 1049686 then previously-good add-ons will fail unless we re-sign and update them before the certs expire -- so we'll have to track expiration time on AMO.

Comment 9

[Mark Straver]

Sorry if I misunderstand the point of having a signature in an extension. If the signature is bad (mismatching SF/RSA), I'd expect the installation of the extension to be prevented, since it's obviously tampered with, corrupt, or damaged in transport, and at the very least give a clear warning in the browser... more than an obscure console message and allowing installation anyway :)

In fact, unless a signed extension passes both the RSA/SF check and the individual file checks, it should be considered corrupt, IMHO, and not installed.

Comment 10

[Daniel Veditz [:dveditz]]

bug 1028643 seems like the only relevant check-in from that window, but are you sure about the window?
1) that was checked into Firefox 34, not 37 as Mark reported
2) that code seems to only check the signature up to the manifest.mf file as expected, it's not checking individual file contents.

Comment 11

[Mark Straver]

(In reply to Daniel Veditz [:dveditz] from comment #10)
> 1) that was checked into Firefox 34, not 37 as Mark reported

Please note that I did not become aware of this problem (because installations succeeded on otherwise assumed validly-signed XPIs) until my very recent foray into this part of extension installations -- it's quite possible that this issue has existed since FF 34, since for a casual observer, all is well. Nobody tends to keep the console open when just installing an add-on to see the message.

What I think happens here is that the checking exits early when the SF/RSA mismatches, and it doesn't get to the file checking at all. That would explain FF37 allowing tampered extensions to be installed as-normal, because it always fails the SF/RSA check.

Comment 12

[Dave Townsend [:mossop]]

(In reply to Daniel Veditz [:dveditz] from comment #10)
> bug 1028643 seems like the only relevant check-in from that window, but are
> you sure about the window?
> 1) that was checked into Firefox 34, not 37 as Mark reported
> 2) that code seems to only check the signature up to the manifest.mf file as
> expected, it's not checking individual file contents.

That's the range I found with mozregression where Firefox switches from calling the file corrupt to just saying it isn't signed, I haven't dug into what is going on specifically.

I don't know if this affects the new signed add-ons or not since the XPI verification code is different but they share some of the certificate checking pieces.

Comment 13

[Dave Townsend [:mossop]]

(In reply to Mark Straver from comment #9)
> Sorry if I misunderstand the point of having a signature in an extension. If
> the signature is bad (mismatching SF/RSA), I'd expect the installation of
> the extension to be prevented, since it's obviously tampered with, corrupt,
> or damaged in transport, and at the very least give a clear warning in the
> browser... more than an obscure console message and allowing installation
> anyway :)
> 
> In fact, unless a signed extension passes both the RSA/SF check and the
> individual file checks, it should be considered corrupt, IMHO, and not
> installed.

The point of having a signature is to tell you that something hasn't been modified since some party you trust signed it. Once there isn't a signature or the signature doesn't match the XPI contents all we know is that we don't know where the code came from, it could have been modified by the original author or another party. If a malicious party is trying to modify a signed extension then all they needed to do is remove the signature and it will install anyway. I'm not sure it makes much difference to the average user to distinguish those cases.

What I'm trying to say is that if an XPI is signed and matches the signature then clearly it should install cleanly and we could tell the user who signed it. If an XPI is unsigned then again it should install and we shouldn't give any assertion that someone signed it. In other cases (broken/untrusted cert, changed/added/removed files) we shouldn't assert that someone signed it but whether it should install or not is a bit of a grey area, I don't think there is a particularly right choice.

We made one choice for that in Firefox 4, partly because of expediency because some of the relevant information wasn't available to JavaScript.

Comment 14

[Daniel Veditz [:dveditz]]

> If an XPI is unsigned then again it should install and we shouldn't give any assertion
> that someone signed it. In other cases (broken/untrusted cert, changed/added/removed
> files) we shouldn't assert that someone signed it but whether it should install or not
> is a bit of a grey area, I don't think there is a particularly right choice.

We made one choice historically (bad cert == unsigned == allow anyway), and we're making a different choice in the future (unsigned == no good cert == NO INSTALL).

I do agree with Mark that this particular change is a bug that should be fixed, and we should test to make sure the new signatures behave "correctly". The grey area is what to do about corrupt metadata; tampered file contents (or missing/added files) is just plain corrupt and should be rejected even if signing itself is optional.

Comment 15

[Mark Straver]

Final decision rests with you of course, but my take on this is:

If metadata is present in the extension, then it should be valid - if it's not, then the extension should be considered corrupt and not allowed to install
Reasoning: The mere fact that metadata is present would imply that the extension is *supposed* to be signed, and failing the sanity/verification check on RSA/SF should therefore automatically classify the extension as corrupt or tampered with.

For the average user I'm sure it would be easy to verify that the extension was signed by someone they trust if it's clearly exposed in the UI (e.g. in the add-on manager).

All that is rather beside the point of this particular bug though.

The main bug here is that you'll always hit a verification error on RSA/SF even if they are matching, so following what I said above, and also in the future case as indicated by Dan, this would make no signed extension installable at all (the current behavior is to allow, which is why this hasn't really come out in the wash until I specifically tested it and found it strange I could just tamper with files of a signed extension and it not complaining at all)

Comment 16

[Dave Townsend [:mossop]]

(In reply to Mark Straver from comment #15)
> Final decision rests with you of course, but my take on this is:
> 
> If metadata is present in the extension, then it should be valid - if it's
> not, then the extension should be considered corrupt and not allowed to
> install
> Reasoning: The mere fact that metadata is present would imply that the
> extension is *supposed* to be signed, and failing the sanity/verification
> check on RSA/SF should therefore automatically classify the extension as
> corrupt or tampered with.
> 
> For the average user I'm sure it would be easy to verify that the extension
> was signed by someone they trust if it's clearly exposed in the UI (e.g. in
> the add-on manager).

Unfortunately we don't expose it clearly in any UI :(

> The main bug here is that you'll always hit a verification error on RSA/SF
> even if they are matching, so following what I said above, and also in the
> future case as indicated by Dan, this would make no signed extension
> installable at all (the current behavior is to allow, which is why this
> hasn't really come out in the wash until I specifically tested it and found
> it strange I could just tamper with files of a signed extension and it not
> complaining at all)

I don't follow this, when correctly signed there is no verification error so we do show the signer in the install UI in current releases, in currently nightlies with AMO signature requirements enabled signature verification seems to work fine.

Comment 17

[Mark Straver]

(In reply to Dave Townsend [:mossop] from comment #16)
> 
> I don't follow this, when correctly signed there is no verification error so
> we do show the signer in the install UI in current releases, in currently
> nightlies with AMO signature requirements enabled signature verification
> seems to work fine.

:) It's not about showing it in the UI. It's about tampered extensions being allowed to install. I'm not sure how many different ways I can try to explain the same thing...

What I noticed is that the SF/RSA check gives an error in the console, even if the files correctly match, on m-r, and as a result, the manifest files aren't checked and you can simply edit files in the signed extension without it refusing to install. The attached edited file should NOT install (since it has an edited file that no longer matches what is signed) but it installs anyway - I just installed nightly to verify this, and it happily accepts the corrupt (edited) extension while displaying the stated (incorrect) SF/RSA mistmatch informational message in the error console. The edited XPI has a properly matching SF/RSA pair and shouldn't give the message in the console, and SHOULD check the manifest files as a result and prevent installation with the edited .js in there, but it does not.

Comment 18

[Daniel Veditz [:dveditz]]

(In reply to Mark Straver from comment #17)
> :) It's not about showing it in the UI. It's about tampered extensions being
> allowed to install. I'm not sure how many different ways I can try to
> explain the same thing...

As far as Firefox is concerned it's an unsigned add-on because the certificate can't be validated. At that point whether any of the files have been tampered with is irrelevant to the behavior you're seeing. We can make a different choice about what to do with bad certs (treat it as a failure rather than "unsigned", which we will do if xpinstall.signatures.required is set to true) or adjust our criteria for bad certs (ignore expiration as we did in the past).

> I just installed nightly to verify this, and it happily accepts the corrupt (edited)
> extension while displaying the stated (incorrect) SF/RSA mistmatch informational
> message in the error console.

No, the message is correct. Valid signatures in Nightly must be issued from the Mozilla addons root. I grant that this has been a controversial decision but it has been clearly communicated. CA-issued signing certs are no longer valid for this purpose as the message says.

> The edited XPI has a properly matching SF/RSA pair and shouldn't give the message
> in the console, and SHOULD check the manifest files as a result and prevent installation
> with the edited .js in there, but it does not.

An invalid certificate cannot "properly match" any SF file.

Comment 19

[Mark Straver]

(In reply to Daniel Veditz [:dveditz] from comment #18)
>  CA-issued signing certs are no longer
> valid for this purpose as the message says.

OK, in that case the error displayed is indeed correct. But don't you think that, in that case, if an XPI includes signing metadata that doesn't match what is expected, that the extension should be blocked from being installed, or at least issue a clear warning that "The extension is signed but not as expected"?

Looking forward, if you are going to make Mozilla-signing compulsory, then either not signed or invalidly (according to your behavior) signed will result in the exact same thing, unless I misunderstand what's going to happen?

This still doesn't take away that for the time being, with signing not being enforced (yet), that tampering of a CA-signed extension will not give any warning at all to the end-user (while it previously did). And that's wrong, and that's why I filed this bug.

I can also imagine that for the (as of yet to me unknown) method required for closed enterprises, where Mozilla-signing isn't an option, this same issue would still apply (if it's not enforced there, then CA-signed checking still needs to work(?), and it currently doesn't) unless you want to enforce that enterprises don't use signed extensions internally anymore...

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Any suggestions of a rating for this issue, Dan?

Comment 21

[Daniel Veditz [:dveditz]]

This is a policy decision rather than a "bug": we should unhide this bug so we can have a proper discussion of it (and it should take place in the addons forum rather than in bugzilla).