Closed Bug 1059208 Opened 8 years ago Closed 7 years ago

Sign manifest files of Trusted Hosted Apps

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S5 (26sep)
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: mattias.ostergren, Assigned: vlatko.markovic)

References

Details

Attachments

(1 file, 10 obsolete files)

25.30 KB, patch
robin.thunell
: review+
Details | Diff | Splinter Review
Manifests of the trusted hosted apps have to be signed and their signature verified on installation.
Assignee: nobody → vlatko.markovic
Blocks: 1016421
Whiteboard: [2.1-feature-qa+]
Target Milestone: --- → 2.1 S4 (12sep)
Whiteboard: [2.1-feature-qa+]
Attachment #8480663 - Flags: review?(dveditz)
Attachment #8480663 - Attachment is obsolete: true
Attachment #8480663 - Flags: review?(dveditz)
Comment on attachment 8482310 [details] [diff] [review]
Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

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

I don't have the appropriate context for this patch. What will be using each of the manifest-signing-*.h files? Why are some of them essentially empty? (If it's because we're waiting on some production data, they should be checked in later, separately.)

::: security/manager/ssl/tests/unit/test_signed_manifest/README.md
@@ +5,5 @@
> +
> +* NSS 3.4 or higher.
> +* Python 2.7 (should work with 2.6 also)
> +* Bash
> +* Device with FX OS running

Also openssl, it appears. Also, is it actually necessary to have the device to re-generate/sign the test files?

@@ +14,5 @@
> +  I) ./create_test_files.sh --regenerate-certs
> +
> +The new test files will be created. At the ./testValidSignedManifest and
> +./testInvalidSignedManifest directories there will be manifests signed with
> +valid and invalid certificate respectively.

nit: "certificates,"

@@ +16,5 @@
> +The new test files will be created. At the ./testValidSignedManifest and
> +./testInvalidSignedManifest directories there will be manifests signed with
> +valid and invalid certificate respectively.
> +
> +If it is the first time that script is run it should be executed with 1 as only

nit: now the parameter is "--regenerate-certs", right?

::: security/manager/ssl/tests/unit/test_signed_manifest/create_test_files.sh
@@ +6,5 @@
> +# 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/.
> +
> +
> +export NSS_DEFAULT_DB_TYPE=sql

This isn't used - should it be?

@@ +9,5 @@
> +
> +export NSS_DEFAULT_DB_TYPE=sql
> +
> +export BASE_PATH=`dirname $0`
> +export SIGN_SCR_LOC=.

The variable name is unclear - what does it stand for?

@@ +14,5 @@
> +
> +# Function to create a signing database
> +# Parameters:
> +# $1: Output directory (where the DB will be created)
> +createDb() {

nit: maybe "createDB"

@@ +16,5 @@
> +# Parameters:
> +# $1: Output directory (where the DB will be created)
> +createDb() {
> +
> +  db=$1

This variable is used pseudo-globally - either make it more clear that this is intentional by calling it "DB" or don't use it globally.

@@ +21,5 @@
> +
> +  mkdir -p $db
> +
> +  # Insecure by design, so... please don't use this for anything serious
> +  passwordfile=$db/passwordfile

This is used globally - same deal.

@@ +53,5 @@
> +  echo n >  $ee_responses # Is this a CA?
> +  echo >>   $ee_responses # Accept default path length constraint (no constraint)
> +  echo y >> $ee_responses # Is this a critical constraint?
> +
> +  make_cert="certutil -d $db -f $passwordfile -S -g 2048 -Z SHA256 \

This function assumes db and passwordfile are already set, when they may not be (which is a bit confusing since the documentation indicates that the db argument is supposed to be passed-in).

@@ +55,5 @@
> +  echo y >> $ee_responses # Is this a critical constraint?
> +
> +  make_cert="certutil -d $db -f $passwordfile -S -g 2048 -Z SHA256 \
> +                    -z $noisefile -y 3 -2 --extKeyUsage critical,codeSigning"
> +  $make_cert -v 480 -n ${4}        -m 1 -s "$ca_subj" \

nit: unnecessary whitespace within this line

@@ +58,5 @@
> +                    -z $noisefile -y 3 -2 --extKeyUsage critical,codeSigning"
> +  $make_cert -v 480 -n ${4}        -m 1 -s "$ca_subj" \
> +      --keyUsage critical,certSigning      -t ",,CTu" -x < $ca_responses 2>&1 >/dev/null
> +  $make_cert -v 240 -n ${5} -c ${4} -m 2 -s "$ee_subj" \
> +      --keyUsage critical,digitalSignature -t ",,,"      < $ee_responses 2>&1 >/dev/null

nit: unnecessary whitespace within this line

@@ +60,5 @@
> +      --keyUsage critical,certSigning      -t ",,CTu" -x < $ca_responses 2>&1 >/dev/null
> +  $make_cert -v 240 -n ${5} -c ${4} -m 2 -s "$ee_subj" \
> +      --keyUsage critical,digitalSignature -t ",,,"      < $ee_responses 2>&1 >/dev/null
> +
> +  # In case we want to inspect the generated certs

Not sure what this comment is for.

@@ +62,5 @@
> +      --keyUsage critical,digitalSignature -t ",,,"      < $ee_responses 2>&1 >/dev/null
> +
> +  # In case we want to inspect the generated certs
> +
> +  # Also, we'll need this one later on

nit: maybe "we'll need both of these later on" (or no comment at all - it doesn't seem necessary)

@@ +94,5 @@
> +usage() {
> +  echo "Usage:"
> +  echo "   $1 [--regenerate-certs]"
> +  echo "params:"
> +  echo "   --regenerate-certs (Optional) - Generate the Certificates Database"

The usage info could have a bit more detail (e.g. what happens if --regenerate-certs isn't specified?)

@@ +106,5 @@
> +TRUSTED_EE=trusted_ee1
> +UNTRUSTED_EE=untrusted_ee1
> +TRUSTED_CA=trusted_ca1
> +UNTRUSTED_CA=untrusted_ca1
> +SPKI_DIGEST=

Probably isn't necessary to pre-declare SPKI_DIGEST

@@ +147,5 @@
> +  fi
> +fi
> +
> +# generate signer cert CA header file
> +python $GEN_CERT_HEADER_DIR/gen_cert_header.py trustedAppTestRoot $DB_PATH/${TRUSTED_CA}.der > $GEN_CERT_HEADER_DIR/manifest-signing-test-root.h

With other, similar files, this gets run as part of the build process. Why does this need to happen here? (And, that way, we don't have to check in the file manifest-signing-test-root.h)

::: security/manager/ssl/tests/unit/test_signed_manifest/manifest.webapp
@@ +3,5 @@
> +  "type": "trusted",
> +  "launch_path": "/index.html",
> +  "icons": { "128" : "icon-128.png" },
> +  "version": 1,
> +  "csp" : "script-src https://www.some.domain.exam.pl; style-src https://www.some.domain.exam.pl",

Use example.com as the example domain (exam.pl is an actual domain someone owns)
Attachment #8482310 - Flags: review?(dkeeler) → review-
Updated according to review comments.
Attachment #8482310 - Attachment is obsolete: true
Attachment #8486487 - Flags: review?(dkeeler)
Comment on attachment 8486487 [details] [diff] [review]
Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

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

I would still like answers to the questions I asked in my previous review (specifically, in the first part and the second-to-last part of comment 3).
Attachment #8486487 - Flags: review?(dkeeler)
Sorry for the delayed answers on you questions.
Regarding the empty header files they are removed now.
Regarding generating the cert header files it's going to be part of the build process.
Both updates are under implementation and are coming in the next patch upload.
The test pinning header files are removed.
create_test_files.sh had a makeover including preparation for build system integration by adding an option to specify test of public pinning header files.
Not integrated into the build system yet.
Attachment #8486487 - Attachment is obsolete: true
Attachment #8489487 - Flags: review?(dkeeler)
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
Comment on attachment 8489487 [details] [diff] [review]
Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

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

This looks fine. I'm still a little confused on why manifest-signing-cert-pinning.h and manifest-signing-root.h are in this patch, though.

::: security/apps/manifest-signing-cert-pinning.h
@@ +1,1 @@
> +	const char trustedAppSignerSpkiDigest[] = ""; 

I'm still a little confused - is this file generated as part of the build process? If so, it probably doesn't need to be checked in, right? Also, there's a trailing space at the end of this line, and there probably doesn't need to be a blank second line.

::: security/apps/manifest-signing-root.h
@@ +1,1 @@
> +const uint8_t trustedAppPublicRoot[] = "";

Same question as with manifest-signing-cert-pinning.h, and there probably doesn't need to be the second blank line.

::: security/manager/ssl/tests/unit/test_signed_manifest/nss_ctypes.py
@@ +3,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/.
> +

nit: unnecessary extra blank line
Attachment #8489487 - Flags: review?(dkeeler) → review+
Updated according to review comments plus an option is added to generate test certificate and test pinning header files at build time.
Attachment #8489487 - Attachment is obsolete: true
Attachment #8490783 - Flags: review?(dkeeler)
Enables the option to generate test certificate and test pinning header files at build time.
Attachment #8490784 - Flags: review?(dkeeler)
Comment on attachment 8490783 [details] [diff] [review]
Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

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

I think a better approach to automating this as part of the build process would be to do what security/apps/Makefile.in has done.

::: client.mk
@@ +165,5 @@
>  
>  # The default rule is build
>  build::
> +ifeq ($(MOZ_GEN_CERT),1)
> +	$(MAKE) -C $(TOPSRCDIR)/security/manager/ssl/tests/unit/test_signed_manifest -f generate_cert.mk CERT_FLAG=$(MOZ_GEN_CERT)

This should probably go in security/apps/Makefile.in (see what's already there that generates some files similarly to this)

::: security/apps/manifest-signing-cert-pinning.h
@@ +1,1 @@
> +const char trustedAppSignerSpkiDigest[] = "";

If this file is auto-generated during build, it doesn't need to be checked in to the tree. Then again, if it isn't auto-generated, it should have a value, right?

::: security/apps/manifest-signing-root.h
@@ +1,1 @@
> +const uint8_t trustedAppPublicRoot[] = "";

Same here.

::: security/manager/ssl/tests/unit/test_signed_manifest/generate_cert.mk
@@ +1,1 @@
> +.PHONY: generate-cert generate-cert-test

I'm fairly sure this isn't the approach we want to take

@@ +1,2 @@
> +.PHONY: generate-cert generate-cert-test
> +# The option to generate public certificate and 

nit: trailing space
Attachment #8490783 - Flags: review?(dkeeler) → review-
Comment on attachment 8490784 [details] [diff] [review]
Bug_1059208-Build-update-to-generate-manifest-signat.patch

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

This shouldn't be necessary if the auto-generation happens in security/apps/Makefile.in, right?
Attachment #8490784 - Flags: review?(dkeeler) → review-
Updated according to review comments.
Attachment #8490783 - Attachment is obsolete: true
Attachment #8490784 - Attachment is obsolete: true
Attachment #8491700 - Flags: review?(dkeeler)
Yes your right, all the header files are now generated from security/apps/Makefile.in,
and since we don't have the public certificates yet the public header files are generated
with empty arrays.
To keep the makefile a bit simpler a new script is added to generated the digest
header file.
Comment on attachment 8491700 [details] [diff] [review]
Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

New update in https://bugzilla.mozilla.org/show_bug.cgi?id=1059216 required a new patch here.
Attachment #8491700 - Attachment is obsolete: true
Attachment #8491700 - Flags: review?(dkeeler)
Attachment #8491734 - Flags: review?(dkeeler) → review?(rlb)
clean up of create_test_files.sh
Attachment #8491899 - Flags: review?(rlb)
Attachment #8491734 - Attachment is obsolete: true
Attachment #8491734 - Flags: review?(rlb)
Attachment #8491899 - Flags: review?(rlb) → review?(dkeeler)
Removal of signature pinning files, certs, and signature files.
Attachment #8492204 - Flags: review?(rlb)
Attachment #8492204 - Flags: review?(dkeeler)
Flags: needinfo?(rlb)
Attachment #8491899 - Attachment is obsolete: true
Attachment #8491899 - Flags: review?(dkeeler)
Flags: needinfo?(rlb)
Comment on attachment 8492204 [details] [diff] [review]
0001-Bug-1059208-Add-scripts-for-signing-manifest-files-o.patch

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

I think this is looking good, with some caveats. The first is it turns out we actually need a copy of trusted_ca1.der to successfully build, and we don't want to run create_test_files.sh as part of the build, so we should just run it once and include that file in this patch (this is something we commonly do with other tests that require generation scripts). The second is that it would be really nice to actually have the trusted hosted app public root. That way, we don't have to make changes to gen_cert_header.py or have the additional logic in Makefile.in. Do we have a timeline for when it will be available? Can we just use a placeholder in the meantime? r=me with those issues and the minor nits (below) addressed.

Also, when I apply this patch, I get what I think are python linting warnings:

security/manager/ssl/tests/unit/test_signed_manifest/nss_ctypes.py:9:1: F403 'from ctypes import *' used; unable to detect undefined names
security/manager/ssl/tests/unit/test_signed_manifest/sign_b2g_manifest.py:12:1: F401 'ctypes' imported but unused

::: security/apps/Makefile.in
@@ +20,4 @@
>  marketplace-stage.inc: marketplace-stage.crt $(GEN_CERT_HEADER)
>  	$(PYTHON) $(GEN_CERT_HEADER) marketplaceStageRoot $< > $@
>  
> +ifeq ($(shell test -s trusted-app-public.der; echo $$?),0)

Why don't we have the trusted hosted app public root yet? If we had it, we wouldn't need to do this.

@@ +28,5 @@
> +
> +manifest-signing-root.inc: $(TRUSTED_APP_PUBLIC) $(GEN_CERT_HEADER)
> +	$(PYTHON) $(GEN_CERT_HEADER) trustedAppPublicRoot $(TRUSTED_APP_PUBLIC) > $@
> +
> +manifest-signing-test-root.inc: $(srcdir)/../manager/ssl/tests/unit/test_signed_manifest/trusted_ca1.der $(GEN_CERT_HEADER)

Since trusted_ca1.der is part of the build now, it looks like we need create_test_files.sh to be run at least once so we can check that file in and have it available.

@@ +46,2 @@
>    xpcshell.inc \
> +  $(NULL)
\ No newline at end of file

This file is missing a newline at the end of the file.

::: security/apps/gen_cert_header.py
@@ +22,4 @@
>    print "};"
>    return 0
>  
> +def create_empty_header(array_name):

Again, my understanding is if we had the trusted hosted app public root, this wouldn't be necessary.

::: security/manager/ssl/tests/unit/test_signed_manifest/create_test_files.sh
@@ +70,5 @@
> +# Function to create a signing database
> +# Parameters:
> +# $1: Output directory (where the DB will be created)
> +# $2: Password file
> +createDB() {

nit: let's be consistent about whether braces for functions go on the next line (like for usage()) or on the same line like here.

@@ +128,5 @@
> +# $3: Signed manifest file path
> +# $4: Nickname of the signing certificate
> +# $5: Password file
> +signManifest() {
> +

nit: unnecessary blank line

::: security/manager/ssl/tests/unit/test_signed_manifest/nss_ctypes.py
@@ +4,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/.
> +
> +

nit: unnecessary blank line
Attachment #8492204 - Flags: review?(dkeeler) → review+
Comment on attachment 8492204 [details] [diff] [review]
0001-Bug-1059208-Add-scripts-for-signing-manifest-files-o.patch

Keeler's review is sufficient here.
Attachment #8492204 - Flags: review?(rlb)
(In reply to David Keeler (:keeler) [use needinfo?] from comment #19)
> Comment on attachment 8492204 [details] [diff] [review]
> 0001-Bug-1059208-Add-scripts-for-signing-manifest-files-o.patch
> 
> Review of attachment 8492204 [details] [diff] [review]:
> -----------------------------------------------------------------
> ... 
> The second is
> that it would be really nice to actually have the trusted hosted app public
> root. That way, we don't have to make changes to gen_cert_header.py or have
> the additional logic in Makefile.in. Do we have a timeline for when it will
> be available? Can we just use a placeholder in the meantime? r=me with those
> issues and the minor nits (below) addressed.
> ...
>
> ::: security/apps/Makefile.in
> @@ +20,4 @@
> >  marketplace-stage.inc: marketplace-stage.crt $(GEN_CERT_HEADER)
> >  	$(PYTHON) $(GEN_CERT_HEADER) marketplaceStageRoot $< > $@
> >  
> > +ifeq ($(shell test -s trusted-app-public.der; echo $$?),0)
> 
> Why don't we have the trusted hosted app public root yet? If we had it, we
> wouldn't need to do this.
> 
> ::: security/apps/gen_cert_header.py
> @@ +22,4 @@
> >    print "};"
> >    return 0
> >  
> > +def create_empty_header(array_name):
> 
> Again, my understanding is if we had the trusted hosted app public root,
> this wouldn't be necessary.
Since for now only OEM would use trusted hosted apps, it would be most appropriate for Mozilla to create a public root for public repository (which would then require that Mozilla signs manifests for the devices that use that particular root), while OEMs should overwrite it for their devices if they have trusted hosted apps. In the future we could think of multiple roots to allow for multiple curators of trusted hosted apps.
Updates according to comments except the ones regarding the public root.
Regarding the new line at the end of Makefile.in it's maybe a little bit confusing but it's actually there in the file but the Bugzilla diff and the patch tool seems to filter it away.
Attachment #8492204 - Attachment is obsolete: true
Attachment #8492578 - Flags: review?(rlb)
Attachment #8492578 - Flags: review?(dkeeler)
Comment on attachment 8492578 [details] [diff] [review]
0001-Bug-1059208-Add-scripts-for-signing-manifest-files-o.patch

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

r+ from keeler
Attachment #8492578 - Flags: review?(rlb)
Attachment #8492578 - Flags: review?(dkeeler)
Attachment #8492578 - Flags: review+
Blocks bug 1059216
Comment on attachment 8492578 [details] [diff] [review]
0001-Bug-1059208-Add-scripts-for-signing-manifest-files-o.patch

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

got a=bajaj over email.
Attachment #8492578 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/5f4035ff1165
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.