1178448 - Tool for creating new signed packages

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect, P2)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Developers will need tools to create the new signed packages. Probably something that is done part of marketplace signing but tracking this separate to make sure we cover all reviewer, partner & beta-testing use cases.

Comment 1

[Jonathan Hao (inactive) [:jhao]]

I'm trying to modify the script fabrice gave me to make it generate signature and calculate the hashes of resources.

https://github.com/johnathan79717/fxos-package-signing-tool

Comment 2

[Jonathan Hao (inactive) [:jhao]]

With my commit today, the packaging tool can calculate hashes and put them in the manifest. Also it will calculate the signature of the manifest and add a header to the package.

Sample package: https://github.com/johnathan79717/fxos-package-signing-tool/blob/dec41ce0c8c640c01d2e3568366339494f008051/my-app.pak

Comment 3

[Jonathan Hao (inactive) [:jhao]]

Usage: also in README.md on github

1. Setup
  * Clone the scripts:

    git clone https://github.com/johnathan79717/fxos-package-signing-tool.git

  * Create certificates and database:

    cd fxos-package-signing-tool
    ./create_test_files.sh --regenerate-test-certs

2. Usage
Each time you want to generate a package, run

    python make_web_package.py <app_folder> <package_name>

Comment 4

[Jonathan Hao (inactive) [:jhao]]

Attachment: Read certificate from /data/local/tmp in developer mode

Comment 5

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8671247 [details] [diff] [review]
Read certificate from /data/local/tmp in developer mode

Hi David,

Could you take a brief look at this WIP patch and tell us if you like this approach? Cause we probably need you to review this. It is based on the discussion last week at MTV with rbarnes.

Instead of letting all verification pass in developer mode, we read a certificate from the device local file system, and make it the trusted root.

The path of the certificate is hardcoded to be in the temporary directory in this WIP patch, but we planned to add another preference that specifies the location.

Also, we may not want to do file I/O every time we open a package, so we should probably just do it once and store the certificate in memory when the PackagedAppService is initialized, and redo it when the preference that indicates the location of the certificate changed.

Thank you.

Comment 6

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

Comment on attachment 8671247 [details] [diff] [review]
Read certificate from /data/local/tmp in developer mode

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

This seems reasonable. It might be nice to have some visual indication when the device is in developer mode (like when you enable debugging on Android and connect to it via USB).

Comment 7

[Jonathan Hao (inactive) [:jhao]]

Attachment: Use imported CA in developer mode

Hi David and Valentin,

Could you both review my patch?

I later decided to store the certificate in an attribute of PackagedAppService
and make it observe the preference change, so I asked Valentin to review as well.

I hope it's not inappropriate to use PackagedAppService this way.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8673014 [details] [diff] [review]
Use imported CA in developer mode

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

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +1029,5 @@
> +  return NS_OK;
> +}
> +
> +void
> +PackagedAppService::ReadCertificate()

Wouldn't this do main thread IO? That would make a lot of people very sad :)

Comment 9

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

Comment on attachment 8673014 [details] [diff] [review]
Use imported CA in developer mode

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

I think this is the right approach, but the main-thread IO is a concern. Also, I think AppTrustDomain should actually own the data for the developer testing root. r- for now.

::: modules/libpref/init/all.js
@@ +1456,3 @@
>  pref("network.http.packaged-apps-developer-mode", false);
> +// The path to your own certificate
> +pref("network.http.packaged-apps-developer-trusted-root", "/data/local/tmp/trusted_ca.der");

This seems to be B2G/android-specific. Should it go in a platform-specific prefs file?

::: netwerk/base/nsIPackagedAppService.idl
@@ +42,5 @@
>    void getResource(in nsIChannel aChannel,
>                     in nsICacheEntryOpenCallback aCallback);
> +
> +  readonly attribute charPtr trustedDERData;
> +  readonly attribute unsigned long trustedDERLength;

I'm not sure "trustedDER" is a good description here, since these are for local development only, right?
Also, I'm thinking the AppTrustDomain should read and own this data, since it's the one actually using it (unless it will be reused elsewhere?)

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +1052,5 @@
> +  }
> +
> +  uint64_t length;
> +  inputStream->Available(&length);
> +  trustedDERData = new char[length];

This probably needs to be thread-safe or at least atomic (and note that both the pointer and the length need to be in sync).

::: security/apps/AppTrustDomain.cpp
@@ +11,5 @@
>  #include "nsIX509CertDB.h"
>  #include "nsNSSCertificate.h"
>  #include "prerror.h"
>  #include "secerr.h"
> +#include "nsIPackagedAppService.h"

nit: these #includes should be sorted

@@ +111,5 @@
> +      trustedDER.data = reinterpret_cast<uint8_t*>(data);
> +      pkgSvc->GetTrustedDERLength(&trustedDER.len);
> +
> +      if (!trustedDER.data) {
> +        PR_SetError(SEC_ERROR_INVALID_ARGS, 0);

I'm not sure INVALID_ARGS is the most appropriate error code here. Maybe SEC_ERROR_BAD_DATA?

::: security/manager/ssl/nsIX509CertDB.idl
@@ +318,5 @@
>    const AppTrustedRoot AppXPCShellRoot = 6;
>    const AppTrustedRoot AddonsPublicRoot = 7;
>    const AppTrustedRoot AddonsStageRoot = 8;
>    const AppTrustedRoot PrivilegedPackageRoot = 9;
> +  const AppTrustedRoot DeveloperTestingRoot = 10;

We should document that if this root is used, the certificate will be read from whatever file is specified in the pref.

Comment 10

[Andrew Williamson [:eviljeff]]

Moving as this bug isn't about Marketplace implementation anymore.  A new bug can be created for any server-side work needed.

Comment 11

[Mahendra Potharaju [:mahe]]

[Tracking Requested - why for this release]:

This bug being part of New Security Model shouldn’t be a 2.5 blocker as New Sec is not part of 2.5.

Removing 2.5 blocker flag.

Comment 12

[Jonathan Hao (inactive) [:jhao]]

Attachment: Use imported CA in developer mode

Hi David and Valentin,

Please review this revised patch again.

1. I've moved the logic back to AppTrustDomain. The special case we added in SetTrustedRoot
   will only be called by VerifySignedManifestAsync, which is not on main thread.s
   I also added mutex lock to ensure thread-safety.

2. I think SEC_ERROR_IO is probably best to describe the failure in SetTrustedRoot.

3. I think developers could also use desktop nightly to test their packages if they want,
   so I kept the pref in all.js. Is it OK?

There's not much change on necko, except adding a parameter aIsDeveloperMode to VerifyManifest

Thank you!

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8674236 [details] [diff] [review]
Use imported CA in developer mode

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

The packaged app bits are OK.

Comment 14

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

Comment on attachment 8674236 [details] [diff] [review]
Use imported CA in developer mode

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

LGTM. Just make that the return value from every library call that can fail is checked. Also, we should make sure those static variables are initialized to something reasonable (the C++ language definition may ensure they're initialized, but I don't remember the details). Finally, see the comment about how this code doesn't observe the pref for the location of the root. That can be a follow-up, though, I guess.

::: security/apps/AppTrustDomain.cpp
@@ +44,4 @@
>  
>  namespace mozilla { namespace psm {
>  
> +Mutex AppTrustDomain::sMutex("AppTrustDomain::mMutex");

I think we want a StaticMutex here.

@@ +44,5 @@
>  
>  namespace mozilla { namespace psm {
>  
> +Mutex AppTrustDomain::sMutex("AppTrustDomain::mMutex");
> +nsAutoArrayPtr<unsigned char> AppTrustDomain::sDevImportedDERData;

This should probably be initialized to nullptr, if it isn't already?

@@ +45,5 @@
>  namespace mozilla { namespace psm {
>  
> +Mutex AppTrustDomain::sMutex("AppTrustDomain::mMutex");
> +nsAutoArrayPtr<unsigned char> AppTrustDomain::sDevImportedDERData;
> +unsigned int AppTrustDomain::sDevImportedDERLen;

Similarly, this should be initialized to 0.

@@ +112,5 @@
>        trustedDER.len = mozilla::ArrayLength(privilegedPackageRoot);
>        break;
>  
> +    case nsIX509CertDB::DeveloperImportedRoot: {
> +      MutexAutoLock lock(sMutex);

(StaticMutexAutoLock for the StaticMutex)

@@ +120,5 @@
> +        if (!file) {
> +          PR_SetError(SEC_ERROR_IO, 0);
> +          return SECFailure;
> +        }
> +        file->InitWithNativePath(

nit: check the return value for this call

@@ +132,5 @@
> +          return SECFailure;
> +        }
> +
> +        uint64_t length;
> +        inputStream->Available(&length);

nit: check return value

@@ +135,5 @@
> +        uint64_t length;
> +        inputStream->Available(&length);
> +
> +        char* data = new char[length];
> +        inputStream->Read(data, length, &sDevImportedDERLen);

nit: check return value

@@ +137,5 @@
> +
> +        char* data = new char[length];
> +        inputStream->Read(data, length, &sDevImportedDERLen);
> +        MOZ_ASSERT(length == sDevImportedDERLen);
> +        sDevImportedDERData = reinterpret_cast<unsigned char*>(data);

Do we want to observe the pref and free this memory when it changes, so subsequent calls will use the new location? (Similarly, it seems unfortunate that if the contents of the file changes, there's not a straight-forward way to get the new data.)

Comment 15

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d62b0e9d96b8

Comment 16

[Paul Theriault [:pauljt] (no longer reading bugmail)]

This should block as without it devs can't sign their own packages. (They could disable signing for testing purposes, but given that we have verification support we should prioritize landing this).

Comment 17

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1562125bbde

Comment 18

[Jonathan Hao (inactive) [:jhao]]

Attachment: Use imported CA in developer mode (r+'d)

Fix the nits mentioned by David, and I filed bug 1215408 for observing the pref.

There are tests failed in Comment 15 because now the behaviour of developer mode
has change. I'm not sure how to test current behaviour automatically. I'll disable
the tests that enables developer mode for now.

Comment 19

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fa9f0cecaad

Comment 20

[Jonathan Hao (inactive) [:jhao]]

Attachment: Use imported CA in developer mode (r+'d, fixed testcases)

Since we don't bypass verification now in developer mode, I fixed some testcases
that don't pass now.

Hi Valentin,
I'm not sure if this needs your review again.

Thank you!

Comment 21

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jonathan Hao [:jhao] from comment #20)
> Created attachment 8674837 [details] [diff] [review]
> Use imported CA in developer mode (r+'d, fixed testcases)
> 
> Since we don't bypass verification now in developer mode, I fixed some
> testcases
> that don't pass now.
> 
> Hi Valentin,
> I'm not sure if this needs your review again.

Actually, I'm having second thoughts about this.
Not having a way to bypass signature verification would make it a real pain to write unit tests for signed packages with handlers that change package contents on-the-fly.
Also, changing tests becomes much harder, since you now need to generate signatures for them using PrivilegedPackageRoot.

Unless you have a better idea, I'd suggest making the developer-mode pref skip verification, and if the developer-trusted-root pref is set to anything, using that as a root instead.
What do you think?

Comment 22

[Jonathan Hao (inactive) [:jhao]]

(In reply to Valentin Gosu [:valentin] from comment #21)
> Actually, I'm having second thoughts about this.
> Not having a way to bypass signature verification would make it a real pain
> to write unit tests for signed packages with handlers that change package
> contents on-the-fly.
> Also, changing tests becomes much harder, since you now need to generate
> signatures for them using PrivilegedPackageRoot.
> 
> Unless you have a better idea, I'd suggest making the developer-mode pref
> skip verification, and if the developer-trusted-root pref is set to
> anything, using that as a root instead.
> What do you think?

Paul, Henry and I think that a preference bypassing verification for all packages would make developer's phone easily owned by malicious links.

To make testing easier, we propose to add a preference that bypass verification for only a specified origin, say "localhost" or "mochi.test", etc.

Also, we would like to take this chance to rename and sort out previous pref's.

Proposal:

1. network.http.signed-packages.enabled
  - This boolean value enables signed packages.
2. network.http.signed-packages.developer-root
  - This is the path to the developer's own root, say "/data/local/tmp/dev_root.der
3. network.http.signed-packages.trusted-origin
  - Testcases can put the origin they test in this pref, say "localhost".
  - The packages from this origin will pass verification even with incorrect signature.

What do you think?

Comment 23

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jonathan Hao [:jhao] from comment #22)
> (In reply to Valentin Gosu [:valentin] from comment #21)
> > Actually, I'm having second thoughts about this.
> > Not having a way to bypass signature verification would make it a real pain
> > to write unit tests for signed packages with handlers that change package
> > contents on-the-fly.
> > Also, changing tests becomes much harder, since you now need to generate
> > signatures for them using PrivilegedPackageRoot.
> > 
> > Unless you have a better idea, I'd suggest making the developer-mode pref
> > skip verification, and if the developer-trusted-root pref is set to
> > anything, using that as a root instead.
> > What do you think?
> 
> Paul, Henry and I think that a preference bypassing verification for all
> packages would make developer's phone easily owned by malicious links.
> 
> To make testing easier, we propose to add a preference that bypass
> verification for only a specified origin, say "localhost" or "mochi.test",
> etc.
> 
> Also, we would like to take this chance to rename and sort out previous
> pref's.
> 
> Proposal:
> 
> 1. network.http.signed-packages.enabled
>   - This boolean value enables signed packages.
> 2. network.http.signed-packages.developer-root
>   - This is the path to the developer's own root, say
> "/data/local/tmp/dev_root.der
> 3. network.http.signed-packages.trusted-origin
>   - Testcases can put the origin they test in this pref, say "localhost".
>   - The packages from this origin will pass verification even with incorrect
> signature.
> 
> What do you think?

Sounds great! r=me

Comment 24

[David Scravaglieri [:scravag]]

This is not a 2.5 blocker as Nsec not part of 2.5, removing blocker

Comment 25

[Jonathan Hao (inactive) [:jhao]]

Attachment: Use imported CA in developer mode (r+'d, just rebased)

This patch is rebased. I filed bug 1216469 to track the verification bypassing.

Comment 26

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a62bac34486d

Comment 27

[Jonathan Hao (inactive) [:jhao]]

Attachment: Use imported CA in developer mode (r+'d)

More rebasing.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/a2335e0a53d9

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 30

[Ethan Tseng [:ethan]]

Good job Jonathan!

Comment 31

[Ethan Tseng [:ethan]]

Follow-up filed:
Bug 1218757 - [NSec] Add a repo under B2G for package signing tool