896620 - Add marketplace certificate for desktop apps

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Marco Castelluccio [:marco]]

Attachment: add_marketplace_cert

(In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith) from bug 892837 comment #10)
> In order to review this patch I need to know more details about how the
> desktop RT works. Does the desktop RT use a separate Gecko profile for each
> installed app? Will this desktop privileged app support be enabled for
> Firefox itself?

Every application will have its own profile. We want to allow privileged apps from marketplace to be installed through Firefox desktop.

> See bug 892288 for an example of the type of problem that doesn't apply to
> B2G but which probably applies to other products. It seems like, at a
> minimum, we'd need to remove the content handlers for X.509 certificates
> (PSMContentListener.cpp) and do other things to ensure that other code
> signing certs are not added to the certificate datbase in the profile that
> the desktop RT.
> 
> When I wrote the B2G_CERTDATA hack, I was actually expecting that we'd have
> to rewrite packaged app certificate verification completely to avoid these
> issues. The B2G_CERTDATA hack was written the way it was because at the time
> I was told I didn't have time to do it right.

Comment 1

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #0)
> Every application will have its own profile. We want to allow privileged
> apps from marketplace to be installed through Firefox desktop.

Actually we also want marketplace signed apps to be installable through the webapp runtime.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Marco Castelluccio [:marco] from comment #0)
> Every application will have its own profile. We want to allow privileged
> apps from marketplace to be installed through Firefox desktop.

Which thing will be doing the signature checking: the Web RT or Firefox?

Comment 3

[Marco Castelluccio [:marco]]

(In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith) from comment #2)
> Which thing will be doing the signature checking: the Web RT or Firefox?

Both of them. When installing from Firefox, it will be Firefox, when installing from the webapp runtime, it will be the webapp runtime.
Note that the webapp runtime will use the Firefox's shared libraries.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: add marketplace cert and enable test

Comment 5

[Marco Castelluccio [:marco]]

What are the next steps here?

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Marco Castelluccio [:marco] from comment #3)
> (In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith)
> from comment #2)
> > Which thing will be doing the signature checking: the Web RT or Firefox?
> 
> Both of them. When installing from Firefox, it will be Firefox, when
> installing from the webapp runtime, it will be the webapp runtime.
> Note that the webapp runtime will use the Firefox's shared libraries.

Then we will need to rewrite the signature verification code. The MOZ_B2G_CERTDATA=1 hack won't work for desktop.

(In reply to Marco Castelluccio [:marco] from comment #5)
> What are the next steps here?

Talk with sstamm about finding somebody who can redo the signature verification code.

Comment 7

[Marco Castelluccio [:marco]]

(In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith) from comment #6)
> Then we will need to rewrite the signature verification code. The
> MOZ_B2G_CERTDATA=1 hack won't work for desktop.

By "won't work" do you mean that it isn't secure?

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Marco Castelluccio [:marco] from comment #7)
> (In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith)
> from comment #6)
> > Then we will need to rewrite the signature verification code. The
> > MOZ_B2G_CERTDATA=1 hack won't work for desktop.
> 
> By "won't work" do you mean that it isn't secure?

In Firefox, we have a bunch of CAs like Verisign, Comodo, etc. that are trusted for creating code-signing certificates. The MOZ_B2G_CERTDATA=1 hack "turns off" the code-signing ability of those CAs and adds a new Mozilla Marketplace CA that is trusted (only) for code-signing. This enforces the rule that only the Mozilla Marketplace is allowed to sign signed packaged apps.

In order to use the MOZ_B2G_CERTDATA=1 hack on Desktop, you'd have to start a discussion on with kwilson and the rest of the CA Policy team and the addons team to see if they will agree to remove the "code signing" capability from the commercial CAs. And, we'd need to have a discussion with Linux vendors (particularly Red Hat and Debian) about *them* also removing that code signing trust bit and/or removing the --use-system-nss build configuration from Firefox. Then we would also need to remove the ability for users to add new code signing certificates to Firefox, and also make a change to NSS and/or PSM to allow us to ignore the user-set code signing trust bits in the user's certificate database.

Otherwise, the alternative is to replace the MOZ_B2G_CERTDATA=1 hack with a proper solution. This quarter, we are working on the basis of that proper solution. Once we have it, it wouldn't be much work to redo the certificate verification code to use that proper solution.

Either path is a nontrivial amount of work. I personally think that the technical solution (the second option) is less risky than the politically-challenging option (the first option). But, regardless, the main issue is prioritizing and assigning the work and dealing with dependencies.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

I wonder if we could make incremental progress here by employing the MOZ_B2G_CERTDATA=1 hack for the desktop runtime such that the Marketplace is able to install privileged apps when running as an app in the runtime, even if can't do so when running as a website in the Firefox browser.  Then would be able to direct users to install the Marketplace app (and thence privileged apps) as a workaround until we get a longer-term solution in place.

But MOZ_B2G_CERTDATA=1 affects the build process for the security/ directory in ways I don't understand, so I don't know if this is a realistic option.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #9)
> I wonder if we could make incremental progress here by employing the
> MOZ_B2G_CERTDATA=1 hack for the desktop runtime such that the Marketplace is
> able to install privileged apps when running as an app in the runtime, even
> if can't do so when running as a website in the Firefox browser.  Then would
> be able to direct users to install the Marketplace app (and thence
> privileged apps) as a workaround until we get a longer-term solution in
> place.
> 
> But MOZ_B2G_CERTDATA=1 affects the build process for the security/ directory
> in ways I don't understand, so I don't know if this is a realistic option.

The data for what root CAs to trust is in certdata.txt. That file gets compiled to C source code that gets compiled into one of the NSS shared libraries. So, MOZ_B2G_CERTDATA=1 directly affects the contents of one of the NSS shared libraries. Additionally, some Linux vendors (e.g. Red Hat/Fedora/CentOS/Oracle, and Debian, at least) substitute their own copy of that DLL for ours.

According to comment 3, the web runtime shares the NSS shared libraries with Firefox. Additionally, according to other comments, Firefox needs to verify signatures on apps before they get installed to the desktop runtime, so Firefox really needs to be doing the right thing too, even if we were to give the desktop runtime its own copies of the shared libraries.

Some things to keep in mind:

1. It would be easy to make it so that signature verification always fails on desktop, so that privileged apps cannot be installed, but non-privileged apps would still work.
2. While people are working on the desktop web runtime, they can build with MOZ_B2G_CERTDATA=1 as long as they don't mind the fact that anybody can use Yahoo's leaked certificate to sign privileged apps that will be trusted on their computer. This will allow everybody on the desktop runtime team to at least get their work done while the certificate issues get sorted out. Just, MOZ_B2G_CERTDATA=1 isn't an acceptable default for m-c.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #8)
> This quarter, we are working on the basis of that proper
> solution. Once we have it, it wouldn't be much work to redo the certificate
> verification code to use that proper solution.

Making bug 878932 block this one since I think that's what you meant, Brian.

Comment 12

[Marco Castelluccio [:marco]]

Attachment: Patch

To support app reviewers needs, at least until we come up with a proper solution, we could verify the signature with the new method and use the old method as a fallback (only for b2g and android).

After this patch most of the tests in test_signed_apps.js are failing because the app they use isn't signed with the marketplace certificate.

Comment 13

[vkrishnamoorthy]

removing the productwanted flag based on comment #11

Comment 14

[Christopher Van Wiemeersch [:cvan]]

What's the status of this?

Comment 15

[Raymond Forbes[:rforbes]]

i am totally fine with brian's solution of having signature check always failing on desktop.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 800540 [details] [diff] [review]
Patch

Review of attachment 800540 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about the delays here. This looks pretty good. Today (Thursday) during the day, I will review the patch and I will describe a workaround that I think we can use until the insanity::pkix library is ready.

Comment 17

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #16)
> Comment on attachment 800540 [details] [diff] [review]
> Patch
> 
> Review of attachment 800540 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry about the delays here. This looks pretty good. Today (Thursday) during
> the day, I will review the patch and I will describe a workaround that I
> think we can use until the insanity::pkix library is ready.

Brian, any update?

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Marco's patch, updated based on the current state of insanity::pkix

https://tbpl.mozilla.org/?tree=Try&rev=81613871c83d

xpcshell test test_signed_apps.js will fail because we don't have a solution for testing with alternate certificates yet. Will chat with the people on the email thread about this tomorrow.

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8373179 [details] [diff] [review]
Marco's patch, updated based on the current state of insanity::pkix

Review of attachment 8373179 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/AppsTrustDomain.cpp
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

please add the emacs/vim modelines.

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "AppsTrustDomain.h"

blank line here.

@@ +6,5 @@
> +#include "insanity/pkix.h"
> +#include "certdb.h"
> +#include "secerr.h"
> +#include "cms.h"
> +#include "marketplaceCert.h"

s/marketplaceCert/AppsRootCerts.h

Sort these includes as specified in the style guide.

@@ +10,5 @@
> +#include "marketplaceCert.h"
> +
> +using namespace insanity::pkix;
> +
> +#ifdef MOZ_LOGGING

This should be PR_LOGGING.

@@ +16,5 @@
> +#endif
> +
> +namespace mozilla { namespace psm {
> +
> +AppsTrustDomain::AppsTrustDomain(void * pinArg)

Please adjust whitespace according to the coding style guide (e.g. "void*" instead of "void *"), here and elsewhere.

@@ +46,5 @@
> +  if (!candidateCert || !trustLevel) {
> +    PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +    return SECFailure;
> +  }
> +

We need to add a comment here explaining that we want to use the NSS certificate database for DISTRUST but we will use our own thing for TRUST.

@@ +66,5 @@
> +  CERTCertificate* marketplaceCert =
> +    CERT_DecodeCertFromPackage((char*)marketplaceCertData,
> +                               sizeof(marketplaceCertData));
> +
> +  if (CERT_CompareCerts(marketplaceCert, candidateCert)) {

You don't need to call CERT_DecodeCertFromPackage and you don't need to use CERT_CompareCerts here. You can just use memcmp to compare (marketplaceCertData, sizeof(marketplaceCertData)) to (candidateCert->derCert.data, candidateCert->derCert.len).

@@ +84,5 @@
> +}
> +
> +nsresult
> +verifyAppSignature(SECItem* buffer, SECItem* detachedDigest,
> +                   CERTCertificate** signerCert) {

Capitalize the function name.

Check that none of the given parameters are NULL.

I was expecting you to copy SEC_PKCS7VerifyDetachedSignatureAtTime, substituting a call to BuildCertChain for the call to CERT_VerifyCert. However, I can see from reading SEC_PKCS7VerifyDetachedSignatureAtTime why you went with this simpler solution. I need to cross-check this implementation with SEC_PKCS7VerifyDetachedSignatureAtTime tomorrow.

Change signerCert to "insanity::pkix::ScopedCERTCertList& builtChain" instead. See my other comments.

@@ +86,5 @@
> +nsresult
> +verifyAppSignature(SECItem* buffer, SECItem* detachedDigest,
> +                   CERTCertificate** signerCert) {
> +  // Decode CMS message.
> +  NSSCMSMessage* cmsMsg = NSS_CMSMessage_CreateFromDER(buffer, 0, 0, 0, 0, 0, 0);

Need to check the return value.

We are leaking this, right?

@@ +92,5 @@
> +  // Check if the message is signed and get its data.
> +  if (!NSS_CMSMessage_IsSigned(cmsMsg)) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CMS message isn't signed"));
> +    return NS_ERROR_CMS_VERIFY_NOT_SIGNED;
> +  } 

trailing whitespace here and elsewhere.

@@ +112,5 @@
> +
> +  // Temporarily import certificates.
> +  if (NSS_CMSSignedData_ImportCerts(sigd, CERT_GetDefaultCertDB(),
> +                                    certUsageObjectSigner,
> +                                    false) != SECSuccess) {

I will make sure this is the right thing to do tomorrow.

@@ +130,5 @@
> +                     MustBeEndEntity, KU_DIGITAL_SIGNATURE,
> +                     SEC_OID_EXT_KEY_USAGE_CODE_SIGN,
> +                     builtChain) != SECSuccess) {
> +    return NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY,
> +                                     -1 * SEC_ERROR_UNKNOWN_ISSUER);

Use PRErrorCode_to_nsresult from ScopedNSSTypes.h

::: security/manager/ssl/src/JARSignatureVerification.cpp
@@ +582,5 @@
>    if (NS_FAILED(rv)) {
>      return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
>    }
>  
> +  CERTCertificate *rawSignerCert;

Make this the ScopedCERTCertList so VierfyAppSignature will return the entire chain. (See the comment below about that being a good idea.)

Note that making this change will also fix a potential use-after-free, AFAICT.

@@ +705,3 @@
>      NS_ENSURE_TRUE(rawSignerCert, NS_ERROR_UNEXPECTED);
>  
>      nsCOMPtr<nsIX509Cert3> signerCert = nsNSSCertificate::Create(rawSignerCert);

To get the certificate from the cert chain, you can use CERT_LIST_HEAD(builtChain).

::: security/nss/lib/smime/smime.def
@@ +24,5 @@
>  NSS_CMSContentInfo_GetBulkKeySize;
>  NSS_CMSContentInfo_GetContent;
>  NSS_CMSContentInfo_GetContentEncAlgTag;
>  NSS_CMSContentInfo_GetContentTypeTag;
> +NSS_CMSContentInfo_GetContentTypeOID;

I will re-review this tomorrow. For now, please split this change to NSS into a separate file, because it would need to be checked in separately.

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8373179 [details] [diff] [review]
Marco's patch, updated based on the current state of insanity::pkix

Review of attachment 8373179 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/JARSignatureVerification.cpp
@@ -575,5 @@
> -  ScopedSEC_PKCS7ContentInfo p7_info(SEC_PKCS7DecodeItem(&sigBuffer,
> -                                        ContentCallback, nullptr,
> -                                        GetPasswordKeyCallback, nullptr,
> -                                        GetDecryptKeyCallback, nullptr,
> -                                        DecryptionAllowedCallback));

Now we get build failures due to the callbacks being unused. The callback functions need to be deleted. (This didn't result in build failure previously because we only added WARNINGS_AS_ERRORS to PSM recently.)

../../../../../security/manager/ssl/src/JARSignatureVerification.cpp:524:1: error: unused function 'ContentCallback' [-Werror,-Wunused-function]
../../../../../security/manager/ssl/src/JARSignatureVerification.cpp:528:1: error: unused function 'GetDecryptKeyCallback' [-Werror,-Wunused-function]
../../../../../security/manager/ssl/src/JARSignatureVerification.cpp:533:1: error: unused function 'DecryptionAllowedCallback' [-Werror,-Wunused-function]
../../../../../security/manager/ssl/src/JARSignatureVerification.cpp:538:1: error: unused function 'GetPasswordKeyCallback' [-Werror,-Wunused-function]

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

<briansmith> It seems like Gekco 28 is 1.3 and Gecko 30 is 1.4, so Gecko 29 is nothing as far as B2G is concerned?
<fabrice> briansmith: correct
<briansmith> fabrice: Yes, I'm just trying to figure out if I make a big change to the app signature verification logic, if it would add risk to B2G to uplift to 29
<fabrice> briansmith: that's fine
<fabrice> go ahead
<briansmith> Thanks.

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: make-it-work.patch

David, I tried to make this work by copying PKCS7_VerifyDetachedSignatureAtTime and replacing the call to CERT_VerifyCert with a call to BuildCertChain. However, PKCS7_VerifyDetachedSignatureAtTime ends up calling a lot of internal pkcs7* functions and copying all of those would be a LOT of code. Marco's way is a better way of doing it.

In attempting to modify Marco's patch, I was trying to avoid the need to export NSS_CMSSignerInfo_Verify from NSS. But, I will just export that from NSS.

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8375915 [details] [diff] [review]
make-it-work.patch

Review of attachment 8375915 [details] [diff] [review]:
-----------------------------------------------------------------

No major concerns - just a bunch of nits. r=me with those addressed.

::: security/apps/AppSignatureVerification.cpp
@@ +574,5 @@
> +      if (cert) {
> +        if (CERT_AddCertToListTail(certs.get(), cert.get()) == SECSuccess) {
> +          cert.release(); // ownership transfered
> +        } else {
> +          MOZ_CRASH("wut?");

Shouldn't allocation failures already be fatal? I don't think we need to handle this case specifically here.

@@ +623,4 @@
>  }
>  
>  NS_IMETHODIMP
> +OpenSignedJARFile(AppTrustedRoot trustedRoot, nsIFile* aJarFile,

nit: aTrustedRoot to be consistent

@@ +664,5 @@
>    if (NS_FAILED(rv)) {
>      return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
>    }
>  
> +  insanity::pkix::ScopedCERTCertList certChain;

doesn't look like this is used

@@ +796,5 @@
>  
>  class OpenSignedJARFileTask MOZ_FINAL : public CryptoTask
>  {
>  public:
> +  OpenSignedJARFileTask(AppTrustedRoot trustedRoot, nsIFile* aJarFile,

nit: aTrustedRoot to be consistent

::: security/apps/AppTrustDomain.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

nit: modeline

@@ +13,5 @@
> +#include "nsIX509CertDB.h"
> +#include "prerror.h"
> +#include "secerr.h"
> +
> +#include "marketplace-prod-public.inc"

Maybe add a comment about how these are programmatically generated.

@@ +134,5 @@
> +    // CERTDB_TERMINAL_RECORD means "stop trying to inherit trust" so if the
> +    // relevant trust bit isn't set then that means the cert must be considered
> +    // distrusted.
> +    PRUint32 relevantTrustBit = endEntityOrCA == MustBeCA ? CERTDB_TRUSTED_CA
> +      : CERTDB_TRUSTED;

nit: indentation

@@ +136,5 @@
> +    // distrusted.
> +    PRUint32 relevantTrustBit = endEntityOrCA == MustBeCA ? CERTDB_TRUSTED_CA
> +      : CERTDB_TRUSTED;
> +    if (((flags & (relevantTrustBit | CERTDB_TERMINAL_RECORD)))
> +      == CERTDB_TERMINAL_RECORD) {

nit: indentation

::: security/apps/AppTrustDomain.h
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

nit: modeline

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef _APPSTRUSTDOMAIN_H_

According to https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#C.2FC.2B.2B_practices this should be mozilla_psm_AppsTrustDomain_h

@@ +28,5 @@
> +  SECStatus VerifySignedData(const CERTSignedData* signedData,
> +                             const CERTCertificate* cert) MOZ_OVERRIDE;
> +
> +private:
> +  void * mPinArg; // non-owning!

nit: void*

@@ +34,5 @@
> +};
> +
> +} } // namespace mozilla::psm
> +
> +#endif

nit: #endif // mozilla_psm_AppsTrustDomain_h

::: security/apps/Makefile.in
@@ +1,1 @@
> +#

unnecessary line?

::: security/apps/gen_cert_header.py
@@ +1,1 @@
> +import sys

boilerplate mode/license

::: security/apps/moz.build
@@ +24,5 @@
> +    DEFINES["MOZ_B2G_CERTDATA"] = 'True'
> +
> +DEFINES['NSS_ENABLE_ECC'] = 'True'
> +for var in ('DLL_PREFIX', 'DLL_SUFFIX'):
> +    DEFINES[var] = '"%s"' % CONFIG[var]

Are these necessary for this directory?

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +61,5 @@
>    if (!results) {
> +    // NSS sometimes returns this unhelpful error code upon failing to find any
> +    // candidate certificates.
> +    if (PR_GetError() == SEC_ERROR_BAD_DATABASE) {
> +      PR_SetError(SEC_ERROR_UNKNOWN_ISSUER, 0);

Is this change just to be consistent with AppTrustDomain?

::: security/manager/ssl/public/nsIX509CertDB.idl
@@ +309,1 @@
>                                in nsIOpenSignedJARFileCallback callback);

You've changed some names to not say JAR anymore, but not all of them, and now it's inconsistent - do we care? Should it be unified in a follow-up?

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2390,2 @@
>    if (caPubs) {
> +    int32_t numCAs = nsCertListCount(caPubs.get());

This seems unrelated to the rest of this patch.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +21,5 @@
>  const SEC_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE;
>  
>  // Sort in numerical order
>  const SEC_ERROR_REVOKED_CERTIFICATE                     = SEC_ERROR_BASE +  12;
> +const SEC_ERROR_UNKNOWN_ISSUER                          = SEC_ERROR_BASE +  13;

If the patch from bug 967975 doesn't bounce from inbound, you won't need this change.

::: security/manager/ssl/tests/unit/test_signed_apps.js
@@ +163,2 @@
>      check_open_result("unknown_issuer",
>                        /*Cr.*/NS_ERROR_SEC_ERROR_UNKNOWN_ISSUER));

Looks like you can change this to also use the definition in head_psm.js like in test_signed_apps_marketplace.js, right?

Comment 24

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8375915 [details] [diff] [review]
make-it-work.patch

Review of attachment 8375915 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/apps/AppSignatureVerification.cpp
@@ +574,5 @@
> +      if (cert) {
> +        if (CERT_AddCertToListTail(certs.get(), cert.get()) == SECSuccess) {
> +          cert.release(); // ownership transfered
> +        } else {
> +          MOZ_CRASH("wut?");

I removed the MOZ_CRASH.

I know that C++ "operator new" allocations will never return null in Gecko, but I'm not sure what the current state of allocation failure via malloc is. Generally we treat all NSS functions that may allocate as potentially failing. If you've written or reviewed code that makes different assumptions then let's file a bug or bugs to resolve those cases.

::: security/apps/AppTrustDomain.cpp
@@ +13,5 @@
> +#include "nsIX509CertDB.h"
> +#include "prerror.h"
> +#include "secerr.h"
> +
> +#include "marketplace-prod-public.inc"

done.

::: security/apps/moz.build
@@ +24,5 @@
> +    DEFINES["MOZ_B2G_CERTDATA"] = 'True'
> +
> +DEFINES['NSS_ENABLE_ECC'] = 'True'
> +for var in ('DLL_PREFIX', 'DLL_SUFFIX'):
> +    DEFINES[var] = '"%s"' % CONFIG[var]

Not sure. Cargo culting.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +61,5 @@
>    if (!results) {
> +    // NSS sometimes returns this unhelpful error code upon failing to find any
> +    // candidate certificates.
> +    if (PR_GetError() == SEC_ERROR_BAD_DATABASE) {
> +      PR_SetError(SEC_ERROR_UNKNOWN_ISSUER, 0);

Yes, it seems like it will be a problem in both cases.

::: security/manager/ssl/public/nsIX509CertDB.idl
@@ +309,1 @@
>                                in nsIOpenSignedJARFileCallback callback);

I s/JAR/App/.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2390,2 @@
>    if (caPubs) {
> +    int32_t numCAs = nsCertListCount(caPubs.get());

This is due to unified builds and disambiguating insanity::pkix::ScopedCERTCertList and mozilla::psm::ScopedCERTCertList.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +21,5 @@
>  const SEC_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE;
>  
>  // Sort in numerical order
>  const SEC_ERROR_REVOKED_CERTIFICATE                     = SEC_ERROR_BASE +  12;
> +const SEC_ERROR_UNKNOWN_ISSUER                          = SEC_ERROR_BASE +  13;

Thanks!

Comment 25

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/mozilla-central/rev/8cdaaf3da9f8

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: make-it-work-2.patch

[Approval Request Comment]
sledru, lsblakk: This is the other uplift I discussed with you earlier this week. Please let me know if you have any questions or concerns and I will address them right away.

User impact if declined: Cannot install packaged apps in Desktop Firefox.

Testing completed (on m-c, etc.): Just landed on m-c. There are automated tests in security/manager/ssl/tests/unit/test_signed_apps.js and security/manager/ssl/tests/unit/test_signed_apps-marketplace.js

Risk to taking this patch (and alternatives if risky): Little risk. There are prefs that can be used to make this functionality inaccessible. Also, we can disable packaged apps for desktop on the server side. For B2G, there is basically no risk to uplifting this because B2G 1.3 is based on Gecko 28 and B2G 1.4 is based on Gecko 30.

String or IDL/UUID changes made by this patch: UUIDs of nsIX509CertDB and an interface specific to this feature were changed.

Comment 27

[(no longer active)]

I did a try push tonight <> and got this valgrind build error:

https://tbpl.mozilla.org/php/getParsedLog.php?id=34721957&tree=Try&full=1

The patch for this bug was also in this push.  Could this have been caused by this patch?

Comment 28

[(no longer active)]

Link to the try push: https://tbpl.mozilla.org/?tree=Try&rev=3d69ab340c2f

Comment 29

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #27)
> I did a try push tonight <> and got this valgrind build error:
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=34721957&tree=Try&full=1
> 
> The patch for this bug was also in this push.  Could this have been caused
> by this patch?

Yes, I will revert the deletion of that file immediately.

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/mozilla-central/rev/b80f7eece913

This will be done in bug 972201.

Comment 31

[(no longer active)]

Thanks for the super-fast response!

Comment 32

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #31)
> Thanks for the super-fast response!

Ehsan, I think there is something very strange happening with your build. I reverted the deletion of b2g-app-root-cert.der as a knee-jerk reaction, but actually a missing b2g-app-root-cert.der shouldn't cause any build failure. Also, your error message about "AppProductionRoot.inc" indicates that your build is somehow referencing older versions of my patch, not the one I checked into m-c! AppProductionRoot.inc is an artifact from a previous version of my patch that isn't part of the current patch. You can search the tree and see that "AppProductionRoot" appears *nowhere*.

Also, my try run of my changeset passed:
https://tbpl.mozilla.org/?tree=Try&rev=8bdc7170c008

Comment 33

[(no longer active)]

Hmm, looking at my try changeset <https://hg.mozilla.org/try/rev/c2af2e22ef8c>, the parent changeset is 8cdaaf3da9f8, which I think is the latest changeset in this bug...  Not sure what else could be wrong.  I don't think our patches should interact with each other in any way.  Retriggered the run a few times...

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #28)
> Link to the try push: https://tbpl.mozilla.org/?tree=Try&rev=3d69ab340c2f

Ehsan, it looks like it was a one-off problem. The retriggerings all succeeded.

Comment 35

[(no longer active)]

Yeah, not sure what was happening...

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Why didn't this receive build peer review?

Comment 37

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to :Ms2ger from comment #36)
> Why didn't this receive build peer review?

All the build system changes were trivial. If there is an issue with the build changes, please let me know.

Also, I'm removing the dependency on bug 967153 since enough of bug 967153 landed for this to get done.

Comment 38

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The policy is that all changes to makefiles, excluding those that only add or remove values from a list, need build peer review. security/apps/Makefile.in doesn't seem to fall under that exclusion.

Comment 39

[Sylvestre Ledru [:Sylvestre]]

For now, I am not sure I should uplift it to aurora. Let me know if / when it is ready.

Comment 40

[(no longer active)]

(In reply to comment #38)
> The policy is that all changes to makefiles, excluding those that only add or
> remove values from a list, need build peer review. security/apps/Makefile.in
> doesn't seem to fall under that exclusion.

Sigh, yeah, that needs to be rewritten in moz.build for the purposes of bug 928196. :-(

We should really stop adding more of these to the build system.  Can you please move that stuff to moz.build in a follow-up?

Comment 41

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #40)
> We should really stop adding more of these to the build system.  Can you
> please move that stuff to moz.build in a follow-up?

Yes, I filed the follow-up as bug 973708 and I will work on it today. Sorry about the misunderstanding regarding the review requirements for Makefile.in. I now understand why we changed that.

Comment 42

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #39)
> For now, I am not sure I should uplift it to aurora. Let me know if / when
> it is ready.

Sylvestre, the patch can be uplifted to Aurora 29 as-is. I am not going to request that the follow-up work I do in bug 973708 be uplifted. The build system issue isn't a correctness issue in that everything works fine; it just gets in the way of upcoming (Gecko 30+) important build system improvements. It would be great to uplift the patch soon so we can demo it working in Fx29 ASAP.

Comment 43

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0f5364659f88
On this uplift commit, I remembered to change the author to be Marco, which I forgot to do on my commit to m-c.