Closed Bug 1178448 Opened 6 years ago Closed 6 years ago

Tool for creating new signed packages

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(tracking-b2g:backlog, firefox44 fixed)

RESOLVED FIXED
FxOS-S10 (30Oct)
tracking-b2g backlog
Tracking Status
firefox44 --- fixed

People

(Reporter: pauljt, Assigned: jhao)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Component: Gaia → Reviewer Tools
Product: Firefox OS → Marketplace
Version: unspecified → Avenir
Priority: -- → P1
Priority: P1 → P2
blocking-b2g: --- → 2.5+
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
Assignee: nobody → 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
Target Milestone: --- → 2015-10-06
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>
Priority: P2 → P3
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.
Attachment #8671247 - Flags: feedback?(dkeeler)
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).
Attachment #8671247 - Flags: feedback?(dkeeler) → feedback+
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.
Attachment #8673014 - Flags: review?(valentin.gosu)
Attachment #8673014 - Flags: review?(dkeeler)
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 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.
Attachment #8673014 - Flags: review?(dkeeler) → review-
Moving as this bug isn't about Marketplace implementation anymore.  A new bug can be created for any server-side work needed.
Component: Reviewer Tools → Gaia
Product: Marketplace → Firefox OS
Target Milestone: 2015-10-06 → ---
Version: Avenir → unspecified
[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.
blocking-b2g: 2.5+ → ---
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!
Attachment #8674236 - Flags: review?(valentin.gosu)
Attachment #8674236 - Flags: review?(dkeeler)
Attachment #8673014 - Attachment is obsolete: true
Attachment #8673014 - Flags: review?(valentin.gosu)
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.
Attachment #8674236 - Flags: review?(valentin.gosu) → review+
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.)
Attachment #8674236 - Flags: review?(dkeeler) → review+
See Also: → 1215408
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).
blocking-b2g: --- → 2.5+
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.
Attachment #8674236 - Attachment is obsolete: true
Attachment #8671247 - Attachment is obsolete: true
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!
Attachment #8674837 - Flags: review?(valentin.gosu)
Attachment #8674758 - Attachment is obsolete: true
Summary: Developer/Reviewer tools for creating new signed packages → Tool for creating new signed packages
(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?
Flags: needinfo?(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?
Flags: needinfo?(jhao) → needinfo?(valentin.gosu)
(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
Flags: needinfo?(valentin.gosu)
Attachment #8674837 - Flags: review?(valentin.gosu) → review+
Priority: P3 → P2
Target Milestone: --- → FxOS-S10 (30Oct)
This is not a 2.5 blocker as Nsec not part of 2.5, removing blocker
blocking-b2g: 2.5+ → ---
This patch is rebased. I filed bug 1216469 to track the verification bypassing.
Attachment #8674837 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8676128 - Attachment is obsolete: true
Good job Jonathan!
QA Whiteboard: [COM=NSec]
Follow-up filed:
Bug 1218757 - [NSec] Add a repo under B2G for package signing tool
See Also: → 1218757
See Also: → 1224084
You need to log in before you can comment on or make changes to this bug.