1059208 - Sign manifest files of Trusted Hosted Apps

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Mattias Östergren]

Manifests of the trusted hosted apps have to be signed and their signature verified on installation.

Comment 1

[Vlatko Markovic]

Attachment: Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

Comment 2

[Robin Thunell]

Attachment: Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

Comment 3

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

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)

Comment 4

[Robin Thunell]

Attachment: Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

Updated according to review comments.

Comment 5

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

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).

Comment 6

[Robin Thunell]

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.

Comment 7

[Robin Thunell]

Attachment: Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

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.

Comment 8

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

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

Comment 9

[Robin Thunell]

Attachment: Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

Updated according to review comments plus an option is added to generate test certificate and test pinning header files at build time.

Comment 10

[Robin Thunell]

Attachment: Bug_1059208-Build-update-to-generate-manifest-signat.patch

Enables the option to generate test certificate and test pinning header files at build time.

Comment 11

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

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

Comment 12

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

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?

Comment 13

[Robin Thunell]

Attachment: Bug_1059208-Add-scripts-for-signing-manifest-files-o.patch

Updated according to review comments.

Comment 14

[Robin Thunell]

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 15

[Zoran Jovanovic]

Attachment: Bug-1059208-Add-scripts-for-signing-manifest-files.patch

Comment 16

[Robin Thunell]

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.

Comment 17

[Zoran Jovanovic]

Attachment: 0001-Bug-1059208-Add-scripts-for-signing-manifest-files-o.patch

clean up of create_test_files.sh

Comment 18

[Robin Thunell]

Attachment: 0001-Bug-1059208-Add-scripts-for-signing-manifest-files-o.patch

Removal of signature pinning files, certs, and signature files.

Comment 19

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

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

Comment 20

[Richard Barnes [:rbarnes]]

Comment on attachment 8492204 [details] [diff] [review]
0001-Bug-1059208-Add-scripts-for-signing-manifest-files-o.patch

Keeler's review is sufficient here.

Comment 21

[Zoran Jovanovic]

(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.

Comment 22

[Robin Thunell]

Attachment: 0001-Bug-1059208-Add-scripts-for-signing-manifest-files-o.patch

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.

Comment 23

[Robin Thunell]

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

Comment 24

[Zoran Jovanovic]

Blocks bug 1059216

Comment 25

[[:fabrice] Fabrice Desré]

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

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f4035ff1165

Comment 28

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d883d8b4bfd6