Add marketplace certificate for desktop apps

RESOLVED FIXED in Firefox 29

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: marco, Assigned: briansmith)

Tracking

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed)

Details

(Whiteboard: DesktopWebRT2)

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

6 years ago
Posted patch add_marketplace_cert (obsolete) — Splinter Review
(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (: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.
Reporter

Comment 1

6 years ago
(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.
(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?
Reporter

Comment 3

6 years ago
(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (: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.
Whiteboard: DesktopWebRT2
Reporter

Comment 4

6 years ago
Attachment #779336 - Attachment is obsolete: true
Reporter

Comment 5

6 years ago
What are the next steps here?
Flags: needinfo?(brian)
(In reply to Marco Castelluccio [:marco] from comment #3)
> (In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (: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.
Flags: needinfo?(brian)
Reporter

Comment 7

6 years ago
(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (: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 reply to Marco Castelluccio [:marco] from comment #7)
> (In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (: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.
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.
(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.
Keywords: productwanted
Priority: -- → P1
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) 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.
Depends on: 878932
Reporter

Comment 12

6 years ago
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #782822 - Attachment is obsolete: true
Attachment #800540 - Flags: feedback?(brian)

Comment 13

6 years ago
removing the productwanted flag based on comment #11
Keywords: productwanted
What's the status of this?
i am totally fine with brian's solution of having signature check always failing on desktop.
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.
(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?
Flags: needinfo?(brian)
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.
Attachment #800540 - Attachment is obsolete: true
Attachment #800540 - Flags: feedback?(brian)
Attachment #8373179 - Flags: review?(brian)
Flags: needinfo?(brian)
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.
Attachment #8373179 - Flags: review?(brian) → review-
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]
Assignee: nobody → brian
Component: Web Apps → Security: PSM
Product: Firefox → Core
Target Milestone: --- → mozilla30
<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.
Posted patch make-it-work.patch (obsolete) — Splinter Review
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.
Attachment #8373179 - Attachment is obsolete: true
Attachment #8375915 - Flags: review?(dkeeler)
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?
Attachment #8375915 - Flags: review?(dkeeler) → review+
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!
https://hg.mozilla.org/mozilla-central/rev/8cdaaf3da9f8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
[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.
Attachment #8375915 - Attachment is obsolete: true
Attachment #8376612 - Flags: review+
Attachment #8376612 - Flags: approval-mozilla-aurora?

Comment 27

5 years ago
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?
Flags: needinfo?(brian)
(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.
Flags: needinfo?(brian)

Comment 31

5 years ago
Thanks for the super-fast response!
(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

5 years ago
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...
(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

5 years ago
Yeah, not sure what was happening...
Why didn't this receive build peer review?
Flags: needinfo?(brian)
(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.
No longer depends on: 967153
Flags: needinfo?(brian)
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.
For now, I am not sure I should uplift it to aurora. Let me know if / when it is ready.

Comment 40

5 years ago
(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?
(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.
(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.
Flags: needinfo?(sledru)
Attachment #8376612 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(sledru)
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.
You need to log in before you can comment on or make changes to this bug.