ACE, Access control enforcer will need to use WebCrypto to verify signed data, and to compute the digest has for the developer certificate. Required: 1) Developer public certificate. 2) Signed application guid. 3) Working WebCrypto in the device specific image. The current vendor specific devic image for Flame (Fx02 v2.1) is missing new WebCrypto changes that allows the import of certificates used for signature verification. Mainline WebCrypto itself already can verify and calculate dev certificate hashes in both release and nightly desktop firefox builds.

^ Correction to bug opening notes: Webcrypto can only verify with keys, not certs. External/extra PKI code appears to be needed in WebCrypto to pull the key out of the actual certificate for signature verification. It gets an error otherwise upon importKey (which retruns a CryptoKey). 'DataError: Data provided to an operation does not meet requirements'. Richard, is PKI considered a feature outside WebCrypto (we're calling from gecko chrome code), or should it be part of some future version of WebCrypto?

Initial crypto verification.

Redirecting needInfo to alternate as requested.

> Richard, is PKI considered a feature outside WebCrypto (we're calling from > gecko chrome code), or should it be part of some future version of WebCrypto? The former. PKI is an application of crypto, so it's separate from WebCrypto. That said, WebCrypto can import a public key in the format used by certificates, so you just need to extract the subjectPublicKeyInfo structure from the certificate and call crypto.subtle.importKey() with "spki" as the format. There are a few good libraries for parsing certificates, e.g.: http://digitalbazaar.com/forge/ http://pkijs.org/

Rebase and update (nothing new).

Comment on attachment 8595634 [details] [diff] [review] Part 1 - Developer signature for apps on the marketplace Should someone on the security team be reviewing/giving feedback?

Comment on attachment 8595634 [details] [diff] [review] Part 1 - Developer signature for apps on the marketplace Review of attachment 8595634 [details] [diff] [review]: ----------------------------------------------------------------- not my expertise.

Comment on attachment 8595634 [details] [diff] [review] Part 1 - Developer signature for apps on the marketplace Review of attachment 8595634 [details] [diff] [review]: ----------------------------------------------------------------- Im not a webcrypto expert but this looks good to me. ::: dom/secureelement/gonk/ACEService.js @@ +241,5 @@ > + return Promise.reject(Error("Certificate, guid signature, and guid " + > + "all must be instances of Uint8Array")); > + } > + > + let alg = { name: "RSASSA-PKCS1-v1_5", hash: "SHA-1" }; Just checking - is the SHA-1 as opposed to something stronger a limitation of webcrypto (I think so iirc, but just checking)

(In reply to Paul Theriault [:pauljt] from comment #9) > Comment on attachment 8595634 [details] [diff] [review] > Part 1 - Developer signature for apps on the marketplace > > Review of attachment 8595634 [details] [diff] [review]: > ----------------------------------------------------------------- > > Im not a webcrypto expert but this looks good to me. > > ::: dom/secureelement/gonk/ACEService.js > @@ +241,5 @@ > > + return Promise.reject(Error("Certificate, guid signature, and guid " + > > + "all must be instances of Uint8Array")); > > + } > > + > > + let alg = { name: "RSASSA-PKCS1-v1_5", hash: "SHA-1" }; > > Just checking - is the SHA-1 as opposed to something stronger a limitation > of webcrypto (I think so iirc, but just checking) It's a limitation of the UICC SE storage space (no room for full dev certificates), and compatibility with other OSes, I believe. A FxOS marketplace verified app would help with the known SHA-1 limitations (See also: Bug 1146624).

SHA-1 is also suggested by GPD spec (GlobalPlatform Device Technology Secure Element Access Control Version 1.0).

Updated. This is the device side check, according to the Marketplace side change: https://bugzilla.mozilla.org/show_bug.cgi?id=1146624#c16 If the signature is to be verified in Marketplace, just checking for guid (or dev signed guid signature) existence client side doesn't actually buy much extra security, so I think we can remove it here. It's in this patch though.

(In reply to Garner Lee from comment #12) > If the signature is to be verified in Marketplace, just checking for guid > (or dev signed guid signature) existence client side doesn't actually buy > much extra security, so I think we can remove it here. It's in this patch > though. Agree on that, we can remove it an keep it all simple. Guid signature should be checked on marketplace side, we only need to compute the hash of the cert and compare it with ACE.

Removed guid/guid_sig check, which will be done in Marketplace.

Why did you set sec-approval? Why did you do so but not fill in the template that goes into the comment when you set it?

(In reply to Al Billings [:abillings] from comment #15) > Why did you set sec-approval? Why did you do so but not fill in the template > that goes into the comment when you set it? I think garner meant sec-review - steph is on it.

Just a note, this bug is implemented according to the dev doc: https://docs.google.com/document/d/1nUtG205Fv6UhOh9cPDmQ19H5Gww7wCDfgwFNhK9SfrQ/edit?usp=sharing

Comment on attachment 8613748 [details] [diff] [review] Part 1 - Verify developer certificate for apps on the marketplace. Review of attachment 8613748 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me

Per new dev document, ACE now also checks application manifest for the relative location of the developer certificate inside the archive. The certificate must still be in DER format. "dev_certificate": "/cert/certfile.der",

Comment on attachment 8617676 [details] [diff] [review] Part 1 - Verify developer certificate for apps on the marketplace. Review of attachment 8617676 [details] [diff] [review]: ----------------------------------------------------------------- comment 8

^^ Ok. I mentioned that to Wesley earlier too.

Hi Stephanie, can you do the review for this patch (or suggest an appropriate reviewer for this security feature?).

Hi Garner, I can take care of the security review, but the code review should be taken care of by the module owner & peers.

Comment on attachment 8617676 [details] [diff] [review] Part 1 - Verify developer certificate for apps on the marketplace. Per IRC, at least initially.

Comment on attachment 8617676 [details] [diff] [review] Part 1 - Verify developer certificate for apps on the marketplace. Review of attachment 8617676 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/SEUtils.jsm @@ +53,5 @@ > + } > + > + let array = new Uint8Array(str.length); > + for (let i = 0; i < str.length; i++) { > + array[i] = str.charCodeAt(i); This will end badly. string.charCodeAt() returns a sixteen-bit value. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/charCodeAt Even if you're using this to read a string that has 8-bit character values, you should assert that here. ::: dom/secureelement/gonk/ACEService.js @@ +181,5 @@ > + throw Error("App not found."); > + } > + let appDir = null; > + // Pre-installed app location, if using non-engineering app builds > + appDir = FileUtils.getDir("coreAppsDir", ["webapps", app.id], false, true); Nit: You could combine this line with the above. @@ +221,5 @@ > + > + return crypto.subtle.digest("SHA-1", devCert) > + .then(function(arrayBuffer) { > + DEBUG && debug("Got Sha1: " + > + JSON.stringify(new Uint8Array(arrayBuffer))); This might not give you what you expect. > JSON.stringify(new Uint8Array([0,1,2,3])) "{"0":0,"1":1,"2":2,"3":3}"

Comment on attachment 8617676 [details] [diff] [review] Part 1 - Verify developer certificate for apps on the marketplace. Review of attachment 8617676 [details] [diff] [review]: ----------------------------------------------------------------- Bunch of minor issues here, so r- for now. The bigger question is: How do we know that the certificate included in the manifest/app is actually related to the app? Are we assuming that this is part of the review process? ::: dom/secureelement/SEUtils.jsm @@ +32,5 @@ > > return array; > }, > > + hexStringToUint8Array: function hexStringToUint8Array(hexStr) { Looks like you don't need this any more? ::: dom/secureelement/gonk/ACEService.js @@ +170,5 @@ > + .then((rules) => { > + let decision = new GPAccessDecision(rules, certHash, aid); > + return Promise.resolve(decision.isAccessAllowed()); > + }); > + }); Don't you need a catch() clause here in case there are errors?

Thanks for the review. Removed extra/obsolete SEUtils function. The SEUtils (that expected raw bytes in the string) function has been renamed "rawByteStringToUint8Array" and throws if it finds the string arg wasn't the default raw bytes from NetUtils. Fixed nit, and replaced the Sha1 JSON.stringify with the more human readable SEUtils.byteArrayToHexString(). Added a catch, though it just prints and forwards to the outer catch. Extra debug added if decision.isAccessAllowed() return false (non-catch scenario).

Test new throw error case.

Trying: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06cb03c826e1

Garner, is this this work still happening?

(In reply to Richard Barnes [:rbarnes] from comment #30) > Garner, is this this work still happening? Good question. Speaking strictly about functionality, it's needed for proper SE support (as well as a way to either include (by marketplace extracting it), or provide a way to extract the key on the device). About the work happening...it's hard to say. There needs to be vendor support for any of this to truly be used. We used one of the Flame mobile devices with a special Qualcomm firmware bulid (2.1?). There hasn't been an image update in quite some time. There's a corresponding change in marketplace as well in Bug 1146624 to verify on submission https://bugzilla.mozilla.org/show_bug.cgi?id=1146624#c24