User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/536.11 (KHTML, like Gecko) Ubuntu/12.04 Chromium/20.0.1132.47 Chrome/20.0.1132.47 Safari/536.11 Steps to reproduce: No Nfc Access support

we should not mark feature work with blocking-b2g flag.

This diagram is based on "GP Secure Element Access Control Version 1.0", which defines a generic mechanism for Secure Element access control in Firefox OS, usable for any kind of Secure Element (e.g. embedded SE, microSD card with security controller, UICC, etc.). It supports application management by multiple entities and allows each entity to set the access rules for its card applications. Secure Element access rule data is stored in the Secure Element (SE) and used by an Access Control enforcer on the device. The Access Control enforcer shall retrieve the access rules from the Secure Element and apply those rules to restrict device application access to the various Secure Element applications.

This diagram describes how the Access Control enforcer on the device shall behave when evaluating rules from ARA-M to be compliant with the SE access control policy. There are two distinct ways that the device can use the access control mechanisms on the Secure Element. Either the device fetches all the rules in advance from the Secure Element, or it queries the Secure Element application each time access is requested. The following sections explain each of those usage scenarios.

Needs complete overhaul. See: https://bugzilla.mozilla.org/show_bug.cgi?id=879861

Quick Note: Currently all NFC payment user stories are covered under: Bug 979152, Bug 979154, Bug 979157, Bug 979158, 884478. All the dependencies for those should be marked in those meta bugs.

Please have a look at: https://bug879861.bugzilla.mozilla.org/attachment.cgi?id=8456179. Ming prepared this diagram to explain the relationship between compontents for access control. If I understand you correctly Ken, the concern is that ACE (Access Control Element) should be a separate module on its own (as opposed to be tied to SE API implementation) and reason for that is that it's used by both NFC stack and SE stack? It makes sense IMO, and I'm fine with doing this properly even for 2.2. I suppose it's easier then to refactor later. Any other comments/questions for this current diagram?

(In reply to Kamil Leszczuk [:kamituel] from comment #6) > Please have a look at: > https://bug879861.bugzilla.mozilla.org/attachment.cgi?id=8456179. > Ming prepared this diagram to explain the relationship between compontents > for access control. > > If I understand you correctly Ken, the concern is that ACE (Access Control > Element) should be a separate module on its own (as opposed to be tied to SE > API implementation) and reason for that is that it's used by both NFC stack > and SE stack? Yes, it is what I was talking. But I was not saying we have to do that, but just saying we need to think about this. > > It makes sense IMO, and I'm fine with doing this properly even for 2.2. I > suppose it's easier then to refactor later. It is fine that we don't have this in 2.2. I think we need time to discuss more details for ACF design. > > Any other comments/questions for this current diagram? Current diagram is good. but if we can know the relations and interfaces among components, it will be better. Here is a example, a RIL developer had written the following wiki for Multi-SIM before she really implemented. And this make everyone clear to know what she wants to do. https://wiki.mozilla.org/WebAPI/WebTelephony/Multi-SIM

Ken, as next, we'd want to cover following issues: - detailed interface description of ACE; - agreed interface between ACE and RIL; - e2e example call flow for sim access; Thanks for the multi-sim wiki, we could create the similar one for SE development, including use cases,requirements, architecture, detailed design of key component/interface, implementation plan, etc.

ACE is a component which should be unit tested, because it can be easily isolated from the underlying RIL (whose simple interface can be simply mocked). In addition to that, to have a good coverage for ACE, we'd need quite a lot of test cases (for various format/combinations of access control rules), which will be pretty painful to write and slow to run using Marionette/Mochitests. Fabrice, in your comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=979767#c52 you mentioned about unit tests for the Gecko JS code. Can you point me to some example on how we can do that? (what runner is used, how to launch them, etc)? I can't really find anything on MDN regarding that.

(In reply to Kamil Leszczuk [:kamituel] from comment #9) > Fabrice, in your comment here: > https://bugzilla.mozilla.org/show_bug.cgi?id=979767#c52 you mentioned about > unit tests for the Gecko JS code. Can you point me to some example on how we > can do that? (what runner is used, how to launch them, etc)? I can't really > find anything on MDN regarding that. If your component doesn't need any UI or web content, you can write an xpcshell test (see https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests). If you need a web page context to be available, you'll need a mochitest, see for instance the ones we have in dom/apps/tests.

Although maybe not of my business I don't see how you could make compelling applications using the SE interface since the channel is between the server and SE. Can you do something like this: https://mobilepki.org/WebCryptoPlusPlus http://webpki.org/papers/PKI/EMV-Tokenization-SET-3DSecure-WebCryptoPlusPlus-combo.pdf#page=4 IMO, the SIM-vendors should consider aligning their products with WebCrypto.Next because WebCrypto operates at a level where the rest of the world will be (already is).

Hey Kamil, Would you want to attach patch for review soon?

This contains the ACE implementation as in GPD spec. The implementation is almost final. The only big thing missing is the developer signature support, which is waiting for the bug 973823. Other small changes needed in the future could be because of the potential changes in IccProvider interface. I've unit tested it using all 3 GPD examples from the spec and the contents from the real SIM card from our partner. Asking or feedback for now, if it can be merged without the dev signature support (so without filling up the _getDevCertHashForApp method) I'll ask for review as well.

Those are the unit tests I've been using during my development and testing. All the code is tested and running properly on the Flame with latest IccProvider from Sid. During devleopment, I've beed using mocks (see attached gecko-mocks.file) so I could develop it in the Firefox desktop for the most part. It speed up my workflow significantly. In case the overall ACE is fit for merging, we could work on moving those unit tests from Mocha to xpcshell. For now I opted for Mocha because they are like 1000 times faster ;)

Comment on attachment 8536803 [details] [diff] [review] ace.patch Review of attachment 8536803 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand why using a JSM? specially when you also do some IPC in this JSM. Also caller part is missing, for example how SE will call your ACE. ::: b2g/chrome/content/shell.js @@ +6,5 @@ > > Cu.import('resource://gre/modules/ContactService.jsm'); > Cu.import('resource://gre/modules/SettingsRequestManager.jsm'); > Cu.import('resource://gre/modules/DataStoreChangeNotifier.jsm'); > +Cu.import('resource://gre/modules/ACEService.jsm'); what's this? ::: dom/secureelement/ACEService.jsm @@ +9,5 @@ > + dump("ACEservice: " + aStr + "\n"); > + } > +} > + > +this.EXPORTED_SYMBOLS = ["ACEService"]; Usually put this in the last line. @@ +20,5 @@ > +XPCOMUtils.defineLazyServiceGetter(this, "ppmm", > + "@mozilla.org/parentprocessmessagemanager;1", > + "nsIMessageListenerManager"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "iccProvider", Shouldn't you use some API from SecureElement? @@ +28,5 @@ > +XPCOMUtils.defineLazyModuleGetter(this, "DOMApplicationRegistry", > + "resource://gre/modules/Webapps.jsm"); > + > + > +let Utils = { Discuss with Sidd to see how you'd share this util function. @@ +151,5 @@ > + return apdu; > + } > +}; > + > +function GPAccessRulesManager() {} This should implement some nsIAccessRulesManager, and perhaps put it in a different JS. @@ +154,5 @@ > + > +function GPAccessRulesManager() {} > + > +GPAccessRulesManager.prototype = { > + PKCS_AID: "a000000063504b43532d3135", add documentation for where it came from. @@ +164,5 @@ > + > + SELECT_BY_DF: [0x00, 0xA4, 0x00, 0x04, 0x02], > + SELECT_ODF: [0x00, 0xA4, 0x00, 0x04, 0x02, 0x50, 0x31], > + > + simSlot: 0, Discuss with Sid for this. SE API should abstract the SIM slot. @@ +187,5 @@ > + let odf = yield this._readODF(); > + let dodf = yield this._readDODF(odf); > + > + let acmf = yield this._readACMF(dodf); > + let refreshTag = acmf[0x30][0x04]; const these magic numbers. @@ +215,5 @@ > + done(); > + } > + }), > + > + _openChannel: function _openChannel() { I won't check these APIs which should be done by SE. Please discuss with Sid how you can call his interfaces. @@ +496,5 @@ > + * applications is denied unless a specific rule explicitly exists for > + * this other device application." > + */ > + let isAidBlocked = !!this.rules.find(this._oneAppletAnyApp.bind(this)); > + remove extra line @@ +570,5 @@ > + > + this._messages = ["ACEService:IsAccessAllowed", "ACEService:ReadRules"]; > + this._messages.forEach((msgName) => { > + ppmm.addMessageListener(msgName, this); > + }); implement some shutdown function. @@ +573,5 @@ > + ppmm.addMessageListener(msgName, this); > + }); > + > + this._ruleManager = new GPAccessRulesManager(); > + debug("Not attempting to read rules on boot."); There are somethings missing here, default should try ARA-M first, then fallback to ARF if ARA-M doesn't exist. @@ +579,5 @@ > + > + isAccessAllowed: function isAccessAllowed(manifestURL, aid) { > + let promiseInit = (resolve, reject) => { > + debug("isAccessAllowed for " + manifestURL + " to " + > + JSON.stringify(aid)); why using stringify on aid? @@ +586,5 @@ > + if (!app) { > + debug("No app found for " + manifestURL); > + return reject(Error("No app found for manifest " + manifestURL)); > + } > + debug("App is: " + JSON.stringify(app)); using if (DEBUG) to guard this. stringify is expensive. @@ +598,5 @@ > + debug("Setting test dev cert hash A683...2129"); > + app.dev_cert_hash = "A683A44507D67C5A58D23BCF2DCBABED9AEC2129"; > + } else { > + debug("Not setting test dev cert hash"); > + } Shouldn't these also move to this_getDevCertHashForApp?

Comment on attachment 8536807 [details] [diff] [review] Tests for the ACE. Review of attachment 8536807 [details] [diff] [review]: ----------------------------------------------------------------- You can do your local test whatever you want. But if you mean to integrate this into m-c there might need a seperate bug for this.

> > Also caller part is missing, for example how SE will call your ACE. SE <-> ACE integration is left out from this. Together with Sid we'll do that after we land both ACE and SE, if that's okay. > > @@ +20,5 @@ > > +XPCOMUtils.defineLazyServiceGetter(this, "ppmm", > > + "@mozilla.org/parentprocessmessagemanager;1", > > + "nsIMessageListenerManager"); > > + > > +XPCOMUtils.defineLazyServiceGetter(this, "iccProvider", > > Shouldn't you use some API from SecureElement? SE API works in a way that it receives a DOM request, then queries ACE, and then (in case of positive response), uses iccProvider to open a channel for an app. So ACE is used by SE, and both are clients of iccProvider. The iccProvider is a single point of contact with SIM card. Does that make sense? > > + _openChannel: function _openChannel() { > > I won't check these APIs which should be done by SE. > > Please discuss with Sid how you can call his interfaces. Same as above. > > There are somethings missing here, default should try ARA-M first, then > fallback to ARF if ARA-M doesn't exist. > We haven't seen any ARA-M implementation in the wild. No existing UICC products support this interface. Even worse, we couldn't obtain any (even non commercial) SIM card which would contain it, so it'd be impossible to actually test it. That's why it has been left out of current implementation. Of course, in case it's needed, we would implement that in the future. All other nits, I'll fix. Thanks for comments Yoshi!

(In reply to Kamil Leszczuk [:kamituel] from comment #17) > > > > Also caller part is missing, for example how SE will call your ACE. > > SE <-> ACE integration is left out from this. Together with Sid we'll do > that after we land both ACE and SE, if that's okay. > Landing a patch which will not be used, is NOT okay. You could implement the minimal feature you needed, that's okay for me. But if the feature is not used at all, it looks to me we should not waste time on this. > > > > @@ +20,5 @@ > > > +XPCOMUtils.defineLazyServiceGetter(this, "ppmm", > > > + "@mozilla.org/parentprocessmessagemanager;1", > > > + "nsIMessageListenerManager"); > > > + > > > +XPCOMUtils.defineLazyServiceGetter(this, "iccProvider", > > > > Shouldn't you use some API from SecureElement? > > SE API works in a way that it receives a DOM request, then queries ACE, and > then (in case of positive response), uses iccProvider to open a channel for > an app. So ACE is used by SE, and both are clients of iccProvider. The > iccProvider is a single point of contact with SIM card. Does that make sense? > What about accessing rules from other SE? like eSE, ASSD? From the spec it mentions some command like STORE_DATA, ... SELECT, if you are implementing that spec you should use those interface as the abstraction to access SE, not using ICC Provider directly. > > > > There are somethings missing here, default should try ARA-M first, then > > fallback to ARF if ARA-M doesn't exist. > > > > We haven't seen any ARA-M implementation in the wild. No existing UICC > products support this interface. Even worse, we couldn't obtain any (even > non commercial) SIM card which would contain it, so it'd be impossible to > actually test it. That's why it has been left out of current implementation. > Of course, in case it's needed, we would implement that in the future. > Then put a TODO or file a bug for this. Because that's mentioned on the spec and we're implementing it.

Can we update the target milestone and ETA? Thanks!

Fixed all nits. This patch applies onto Sid's patch (SE impl.) with SEUtils. Also, to clarify - the IPC is there for test purposes only. We'll remove it once done. In the end, the DOM SE will include ACE and call |.isAccessAllowed| method.

Comment on attachment 8542969 [details] [diff] [review] ACE for SE. Review of attachment 8542969 [details] [diff] [review]: ----------------------------------------------------------------- I don't see my previous comments addressed at all, except a JS RuleManager is added. ::: b2g/chrome/content/shell.js @@ +6,5 @@ > > Cu.import('resource://gre/modules/ContactService.jsm'); > Cu.import('resource://gre/modules/SettingsRequestManager.jsm'); > Cu.import('resource://gre/modules/DataStoreChangeNotifier.jsm'); > +Cu.import('resource://gre/modules/ACEService.jsm'); ? ::: dom/secureelement/ACEService.jsm @@ +1,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/. */ > + > +/* Copyright © 2015, Deutsche Telekom, Inc. */ Like I said before who will use this jsm? @@ +88,5 @@ > + > + return false; > + }, > + > + _applicationMatches: function applicationMatches(appArray) { function name doesn't have _ prefix. @@ +98,5 @@ > + return SEUtils.arraysEqual(hash, this.certHash); > + })); > + }, > + > + _oneAppletAnyApp: function(rule) { no function name. Use consistent style for function name. @@ +187,5 @@ > + // Handle messages from SE/NFC DOM impl. We might want it to be a regular > + // interface, not message-based in case both SE/NFC DOM impl. and ACEService > + // are living in the same process. > + // Should be exposed as service to be used by SE parent and HCIConfigurator > + receiveMessage: function receiveMessage(aMessage) { See my previous comment this should NOT be done in a JSM ::: dom/secureelement/GPAccessRulesManager.js @@ +24,5 @@ > +Cu.import("resource://gre/modules/Task.jsm"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "iccProvider", > + "@mozilla.org/ril/content-helper;1", > + "nsIIccProvider"); See my previous comment this should be done in SE.

[updating target milestone] Considering review lead time and PTOs, I'm resetting the target milestone to Feb6.

This is the main ACE component. ACEService.js implements nsIAccessControlEnforcer.idl which defines isAccessAllowed method. ACEService.js will retrieve parse rules from SIM card via GPAccessRulesManager (will upload in next patch). IPC calls are removed now.

GPAccessRulesManager.js is a component responsible for reading and parsing the rules from the SE. It implements nsIAccessRulesManager.idl, it uses UiccConnector to access the SIM card ARF structure (as defined in GlobalPlatform Device Technology Secure Element Access Control section 7). It does not support ARA-M currently.

This is the initial integration between SE parent process and ACE. SE parent process implementation calls ACE.isAccessAllowed before opening the channel to SE. I'm not sure if this should be landed in this bug or in 1118098.

Comment on attachment 8550369 [details] [diff] [review] Part 1: ACEService.js and nsIAccessControlEnforcer.idl Review of attachment 8550369 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/ACEService.js @@ +8,5 @@ > + > +/* globals dump, Components, XPCOMUtils, ppmm, DOMApplicationRegistry, > + SEUtils */ > + > +const DEBUG = true; default to false. @@ +34,5 @@ > + * algorithm does not support that either (should be straightforward to add > + * later, though). > + */ > +function GPAccessDecision(rules, certHash, aid) { > + this.rules = rules; I cannot find where 'rules' came from. I think the order of these patches are wrong. @@ +52,5 @@ > + > + /* > + * Implements a following check (GPD 3.2.1): > + * "If a specific rule associates the hash of a device application with > + * the AID of an SE application, then access to all the other device why indent?

Comment on attachment 8550375 [details] [diff] [review] Part 2: GPAccessRulesManager.js and nsIAccessRulesManager.idl Review of attachment 8550375 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/GPAccessRulesManager.js @@ +8,5 @@ > + > +/* globals Components, dump, XPCOMUtils, Promise, Task, SEUtils, libcutils, > + UiccConnector */ > + > +let DEBUG = true; ^^^ @@ +25,5 @@ > +Cu.import("resource://gre/modules/systemlibs.js"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "UiccConnector", > + "@mozilla.org/secureelement/connector;1", > + "nsISecureElementConnector"); I have mentioned in the SE API bug this is a wrong contractId. @@ +36,5 @@ > + > +/* > + * Based on [1] - "GlobalPlatform Device Technology > + * Secure Element Access Control Version 1.0". > + * GPAccessRulesManager reads and parses access rules from SIM card file system It should be 'Secure Element' and not SIM-specific. @@ +52,5 @@ > + > + // APDUs (ISO 7816-4) for accessing rules on SIM card file system > + // see for more details: http://www.cardwerk.com/smartcards/ > + // smartcard_standard_ISO7816-4_6_basic_interindustry_commands.aspx > + READ_BINARY: [0x00, 0xB0, 0x00, 0x00], Why stored this as array and covert this to an object in byteToAPDU? Why not store this as { cla: ins: ... }; @@ +60,5 @@ > + > + // Non-null if there is a channel open > + channel: null, > + > + REFRESH_TAG_PATH: [0x30, 0x04], what does this come from @@ +63,5 @@ > + > + REFRESH_TAG_PATH: [0x30, 0x04], > + refreshTag: null, > + > + // Contains rules as read from the UICC UICC? @@ +91,5 @@ > + let dodf = yield this._readDODF(odf); > + > + let acmf = yield this._readACMF(dodf); > + let refreshTag = acmf[this.REFRESH_TAG_PATH[0]] > + [this.REFRESH_TAG_PATH[1]]; What's the return type of acmf and is this a 2D array or 2-level object property access? @@ +107,5 @@ > + let condFiles = yield this._readACConditions(acrf); > + this.rules = yield this._parseRules(acrf, condFiles); > + > + debug("_readAccessRules: done reading rules"); > + debug("_readAccessRules: " + JSON.stringify(this.rules, 0, 2)); use DEBUG to wrap JSON.stringify @@ +216,5 @@ > + > + _selectAndRead: function _selectAndRead(apdu, cb) { > + return this._exchangeAPDU(apdu) > + .then((resp) => this._readBinaryFile(resp)); > + }, The function name doesn't match what it does here. if the name is selectAndRead it should look like selectAndRead: function(id, cb) { select(id).then(resp => this.readBinary(resp)); } select: function(id) { // wrap it as some Select APDU let selectAPDU = ....; return this.exchangeAPDU(selectAPDU); } @@ +226,5 @@ > + > + _readDODF: function _readDODF(odfFile) { > + debug("_readDODF, ODF file: " + odfFile); > + > + let DODF_DF = odfFile[0xA7][0x30][0x04]; What's the return type of odfFile? @@ +246,5 @@ > + 0x81, 0x48, // 129 > + 0x01, // 1 > + 0x01]; // 1 > + > + let records = this._ensureIsArray(dodfFile[0xA1]); what's 0xA1? @@ +323,5 @@ > + let df = ruleEntry[0x30][0x04]; > + let condition = conditionFiles[df]; > + > + let oneApplet = ruleEntry[0xA0]; > + let allApplets = ruleEntry[0x82]; Where do these numbers come from ::: dom/secureelement/gonk/nsIAccessRulesManager.idl @@ +15,5 @@ > + * @return Promise which is resolved if init is successful or rejected > + * otherwise > + */ > + jsval init(); > + For IDL please use some function callback to do asynchrouns operation. @@ +18,5 @@ > + jsval init(); > + > + /** > + * Retrieves all access rules > + * @return Promise which resolves with Array containing parsed access rules That should be some IDL or documenation to describe the rules. @@ +20,5 @@ > + /** > + * Retrieves all access rules > + * @return Promise which resolves with Array containing parsed access rules > + */ > + jsval getAccessRules(); I cannot find any code calling this.

Comment on attachment 8550381 [details] [diff] [review] Part 4: initial SE integration with ACE Review of attachment 8550381 [details] [diff] [review]: ----------------------------------------------------------------- wait for SE ready then send r? I cannot comment anything base on a r- codebase.

Comment on attachment 8550369 [details] [diff] [review] Part 1: ACEService.js and nsIAccessControlEnforcer.idl Review of attachment 8550369 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl @@ +9,5 @@ > +[scriptable, uuid(68c358a6-441c-4823-b8c7-93b9dda376ce)] > +interface nsIAccessControlEnforcer : nsISupports > +{ > + jsval isAccessAllowed(in unsigned long localId, in DOMString seName, > + in jsval aid); Please explain the arguments here. What's localId? What's seName? is it 'eSE' or 'uicc'? And why aid is type of jsval? And what's the return type?

Comment on attachment 8550375 [details] [diff] [review] Part 2: GPAccessRulesManager.js and nsIAccessRulesManager.idl Review of attachment 8550375 [details] [diff] [review]: ----------------------------------------------------------------- r- for the previous unclear IDL.

Comment on attachment 8550377 [details] [diff] [review] Part 3: build support for ACE components Review of attachment 8550377 [details] [diff] [review]: ----------------------------------------------------------------- looks okay, but I prefer feedback on this when Part 1 and Part 2 are r+.

Comment on attachment 8550381 [details] [diff] [review] Part 4: initial SE integration with ACE Review of attachment 8550381 [details] [diff] [review]: ----------------------------------------------------------------- r- for this has the same problem with SE. ::: dom/secureelement/gonk/SecureElement.js @@ +251,5 @@ > this.handlers["SE:TransmitAPDU"] = this.handleTransmit; > > Services.obs.addObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false); > + > + this.accessControlEnforcer = shorter name, ace or aceService, or acEnforcer @@ +370,4 @@ > > + let connector = getConnector(msg.type); > + connector.openChannel(PREFERRED_UICC_CLIENTID, > + SEUtils.byteArrayToHexString(msg.aid), { This patch has the same problem with SE. The connect should be a generic connector, it could be UICC, could be eSE. Why does the first agrument still bind to the UICC clientId?

Comment on attachment 8550375 [details] [diff] [review] Part 2: GPAccessRulesManager.js and nsIAccessRulesManager.idl Review of attachment 8550375 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/GPAccessRulesManager.js @@ +25,5 @@ > +Cu.import("resource://gre/modules/systemlibs.js"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "UiccConnector", > + "@mozilla.org/secureelement/connector;1", > + "nsISecureElementConnector"); Also I've mentioned to Sidd. Please make your architecture extensible, here you just use Uicc. What if we plan to support eSE, ASSD in the future? Try to make the interface generic. And from the specification it mentions 'Secure Element', not 'UICC' nor 'embeddedSE' So if your implementation is programming on certain implementation, which will be wrong.

(In reply to Yoshi Huang[:allstars.chh] from comment #33) > r- for this has the same problem with SE. > This patch builds on the latest patches for SE. SE changes to use proper contractID are WIP [1], and I hope to upload them today in SE patch. Since I wanted this codebase to be actually running on the device I've used patches provided by Sid and asked for feedback. The version which will be uploaded for your review will of curse use the UiccConnector which will be r+'ed. [1] - https://github.com/svic/SecureElement-v0.1/commit/34c36c129b6af0cc7ff2950fdfe2c8706e1955b9

Sorry I have to fix blockers and still have to review patches from tauzen and Dimi now, please send your next patch to Vincent/Dimi.

Dimi can help to provide feedback while Yoshi is busy on fixing blockers this week. We still need Yoshi to stamp on the patch, and he will be back soon. :-)

(In reply to Yoshi Huang[:allstars.chh] from comment #27) > @@ +8,5 @@ > > + > > +/* globals dump, Components, XPCOMUtils, ppmm, DOMApplicationRegistry, > > + SEUtils */ > > + > > +const DEBUG = true; > > default to false. Fixed. > > @@ +34,5 @@ > > + * algorithm does not support that either (should be straightforward to add > > + * later, though). > > + */ > > +function GPAccessDecision(rules, certHash, aid) { > > + this.rules = rules; > > I cannot find where 'rules' came from. > I think the order of these patches are wrong. Those rules are supplied by ACEService. ACEservice would query GPAccessRulesManager for all the rules, and then pass those rules to GPAccessDecision which uses them to make a decision to allow or deny access to SE. > > @@ +52,5 @@ > > + > > + /* > > + * Implements a following check (GPD 3.2.1): > > + * "If a specific rule associates the hash of a device application with > > + * the AID of an SE application, then access to all the other device > > why indent? Fixed (In reply to Yoshi Huang[:allstars.chh] from comment #28) > > +/* globals Components, dump, XPCOMUtils, Promise, Task, SEUtils, libcutils, > > + UiccConnector */ > > + > > +let DEBUG = true; > Fixed. > > +XPCOMUtils.defineLazyServiceGetter(this, "UiccConnector", > > + "@mozilla.org/secureelement/connector;1", > > + "nsISecureElementConnector"); > > I have mentioned in the SE API bug this is a wrong contractId. > Fixed. > > + * Based on [1] - "GlobalPlatform Device Technology > > + * Secure Element Access Control Version 1.0". > > + * GPAccessRulesManager reads and parses access rules from SIM card file system > > It should be 'Secure Element' and not SIM-specific. Yes, we tend to mix those two terms, sorry. Fixed. > > > + READ_BINARY: [0x00, 0xB0, 0x00, 0x00], > > Why stored this as array and covert this to an object in byteToAPDU? > Why not store this as > { > cla: > ins: > ... > }; The helper function (_bytesToAPDU) is useful anyway, because it helps out with data reformatting as well. So, having it, storing commands in an array (as opposed to an object) is more compact and looks more like examples from the spec. > > > + > > + REFRESH_TAG_PATH: [0x30, 0x04], > > what does this come from Added a comment explaining that. It comes from GPD spec, sections 7.1.5 and examples (i.e. C.1). > > @@ +63,5 @@ > > + > > + REFRESH_TAG_PATH: [0x30, 0x04], > > + refreshTag: null, > > + > > + // Contains rules as read from the UICC > > UICC? SE of course. Fixed. > > + let acmf = yield this._readACMF(dodf); > > + let refreshTag = acmf[this.REFRESH_TAG_PATH[0]] > > + [this.REFRESH_TAG_PATH[1]]; > > What's the return type of acmf and is this a 2D array or 2-level object > property access? We're reaching for a refresh tag here. ACMF file (and every other TLV filed used by GPAccessRulesManager) are parsed by _parseTLV(). For a example of ACMF file, see the GPD spec, or this attachment: https://bugzilla.mozilla.org/attachment.cgi?id=8536807. > > > + debug("_readAccessRules: " + JSON.stringify(this.rules, 0, 2)); > > use DEBUG to wrap JSON.stringify Fixed for all occurences. > > @@ +216,5 @@ > > + > > + _selectAndRead: function _selectAndRead(apdu, cb) { > > + return this._exchangeAPDU(apdu) > > + .then((resp) => this._readBinaryFile(resp)); > > + }, > > The function name doesn't match what it does here. > > if the name is selectAndRead > it should look like > > selectAndRead: function(id, cb) { > select(id).then(resp => this.readBinary(resp)); > } > > select: function(id) { > // wrap it as some Select APDU > let selectAPDU = ....; > return this.exchangeAPDU(selectAPDU); > } Ah, you're right. I changed it a bit, so now selectAndRead() accepts not the whole APDU, but rather only file's DF. It's cleaner this way, because the whole APDU doesn't need to be assembled for each file (ACMF, ODF, DODF, ...) in many places, but only here. Thanks for pointing that out. > > @@ +226,5 @@ > > + > > + _readDODF: function _readDODF(odfFile) { > > + debug("_readDODF, ODF file: " + odfFile); > > + > > + let DODF_DF = odfFile[0xA7][0x30][0x04]; > > What's the return type of odfFile? Same as above, it's parsed by _parseTLV(). Description of a format either in the attachment, or in GPD spec. > > + > > + let records = this._ensureIsArray(dodfFile[0xA1]); > > what's 0xA1? It's a generic tag which means that we're reaching into a "subdocument". It's meaningful only in relation to this particular place in this particular file - so this tag doesn't fully describe the contents, i.e. in this case it doesn't tell that TLV with tag 0xA1 contains records. That's why it's not worth const'ing it. > > @@ +323,5 @@ > > + let df = ruleEntry[0x30][0x04]; > > + let condition = conditionFiles[df]; > > + > > + let oneApplet = ruleEntry[0xA0]; > > + let allApplets = ruleEntry[0x82]; > > Where do these numbers come from Added a comment explaining. They come from GPD spec. > > ::: dom/secureelement/gonk/nsIAccessRulesManager.idl > @@ +15,5 @@ > > + * @return Promise which is resolved if init is successful or rejected > > + * otherwise > > + */ > > + jsval init(); > > + > > For IDL please use some function callback to do asynchrouns operation. As Krzysztof agreed with you on IRC after last round of feedback, we'll be using promises here. > > > + > > + /** > > + * Retrieves all access rules > > + * @return Promise which resolves with Array containing parsed access rules > > That should be some IDL or documenation to describe the rules. Added documentation. > > @@ +20,5 @@ > > + /** > > + * Retrieves all access rules > > + * @return Promise which resolves with Array containing parsed access rules > > + */ > > + jsval getAccessRules(); > > I cannot find any code calling this. ACEservice is calling this each time it's making an access decision. (In reply to Yoshi Huang[:allstars.chh] from comment #30) > ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl > @@ +9,5 @@ > > +[scriptable, uuid(68c358a6-441c-4823-b8c7-93b9dda376ce)] > > +interface nsIAccessControlEnforcer : nsISupports > > +{ > > + jsval isAccessAllowed(in unsigned long localId, in DOMString seName, > > + in jsval aid); > > Please explain the arguments here. > What's localId? > What's seName? is it 'eSE' or 'uicc'? > And why aid is type of jsval? > > And what's the return type? Added comment in IDL file. All above fixes is in patches I'll upload shortly. Thanks for the feedback!

Addressing Yoshi's comments and feedback (see comment above).

Same as above, fixes nits raised by Yoshi. Also, regarding parsing TLV documents. Here's the rationale for doing this the way it's currently implemented (so nested objects): - this way code using this data structure can be simple, i.e. response[tag1][tag2]. - this document format maps closely to the way those files are structured, so it's easier to reason about them and read the code. Of course, without first reading GPD and some PKCS specs, the tags used are cryptic, but changing the data structure wouldn't solve that ;) - number of documents created isn't that high, because most of the time GP files (ACMF, ACRF, ...) are small. - we're not creating those documents when not needed - only when some application requests opening a channel to the SE applet. I.e. we're not doing that periodically. - the TLV parser used in ril_worker.js would create similar number of objects as far as I understand it. And using document returned by it is actually more complex, i.e. you'd need to write some loops to get to most values. Having that said, I'd prefer to keep the current TLV structure. We've also added some tags explanations, along with the specs where they're defined, in the _parseTLV() comments.

Comment on attachment 8554878 [details] [diff] [review] Part 1: ACEService.js and nsIAccessControlEnforcer.idl Review of attachment 8554878 [details] [diff] [review]: ----------------------------------------------------------------- feedback- because i would like to see comment addressed ::: dom/secureelement/gonk/ACEService.js @@ +47,5 @@ > + // GPD 4.2.3 A > + decision = this.rules.filter(this._oneAppletOneApp.bind(this)); > + if (decision.length === 1) { > + return true; > + } How about else case ? or |if(!!decision)| is enough same as GPD 4.2.3 C @@ +65,5 @@ > + // GPD 4.2.3 B. No need to merge rules, because we do not implement > + // APDU filters for now. > + decision = this.rules.filter(this._oneAppletAllApps.bind(this)); > + if (decision.length > 0) { > + return decision[0].application === "allowed-all" ? true : false; can just use |decision[0].application === "allowed-all"| same as GPD 4.2.3 D. @@ +106,5 @@ > + }, > + > + _oneAppletAllApps: function _oneAppletAllApps(rule) { > + let appMatches = rule.application === "allowed-all" || > + rule.application === "denied-all"; Please define "allow-all" and "denied-all" in nsIAccessRulesManager Then use Ci.nsIAccessRulesManager.ALLOW_ALL & Ci.nsIAccessRulesManager.DENIED_ALL @@ +111,5 @@ > + return SEUtils.arraysEqual(rule.applet, this.aid) && appMatches; > + }, > + > + _allAppletsOneApp: function _allAppletsOneApp(rule) { > + return rule.applet === "all" && this._applicationMatches(rule.application); define like "all" in nsIAccessRulesManager.idl Ex. const DOMString ALL_APPLET = "all"; @@ +118,5 @@ > + _allAppletsAllApps: function _allAppletsAllApps(rule) { > + let appMatches = rule.application === "allowed-all" || > + rule.application === "denied-all"; > + return rule.applet === "all" && appMatches; > + } I wrote a pseudo code for the GPD 4.2.3. it combine 4 _xxxAppletsxxxApps function and may just need to call |this.rules.filter| once (Trying to reduce that because each array filter call will create a new array). Please check if it could improve the code here. // GPD 4.2.3 Algorithm for applying rules let decision = this.rules.filter(this._searchForRules.bind(this)); // I do not quite understand the logic here( > 0 and === 1) but just follow the original implemetation if (decision.length > 0 && decision[0].application === Ci.nsIAcessRulesManager.ALLOW_ALL) { return true; } else if (decision.length === 1 && Array.isArray(decision[0].application)) { return true; } _searchForRules: function _searchForRules(rule) { let appMatched; let appletMatched; // 4.2.3 A, 4.2.3 C if (Array.isArray(rule.application)) { appMatched = this._applicationMatched(rule.application); // 4.2.3 B, 4.2.3 D } else { appMatched = [Ci.nsIAcessRulesManager.ALLOW_ALL, Ci.nsIAcessRulesManager.DENIED_ALL] .indexOf(rule.application) != -1; } // 4.2.3 A, 4.2.3 B if (Array.isArray(rule.applet)) { appletMatched = SEUtils.arraysEqual(rule.applet, this.aid); // 4.2.3 C, 4.2.3 D } else { appletMatched = rule.applet === Ci.nsIAcessRulesManager.ALL_APPLET; } return appMatched && appletMatched; } @@ +122,5 @@ > + } > +}; > + > +function ACEService() { > + this._ruleManager = Nits. sync naming, _rulesManager @@ +136,5 @@ > + > + isAccessAllowed: function isAccessAllowed(localId, seName, aid) { > + let manifestURL = DOMApplicationRegistry.getManifestURLByLocalId(localId); > + if (!manifestURL) { > + return Promise.reject("Missing manifest for app: " + localId); Promise.reject(new Error("Missing manifest for app: " + localId)); @@ +139,5 @@ > + if (!manifestURL) { > + return Promise.reject("Missing manifest for app: " + localId); > + } > + > + let promiseInit = (resolve, reject) => { Usually we declare function directly inside |new Promise|. so return new Promise((resolve, reject) => { debug("isAccessAllowed for " + manifestURL + " to " + aid); ... }); @@ +183,5 @@ > + return ""; > + }); > + }, > + > + classID: Components.ID("{882a7463-2ca7-4d61-a89a-10eb6fd70478}"), Add contractID: ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl @@ +17,5 @@ > + * @param localId ID of an application accessing SE > + * @param seName Name of the SE. > + * @param aid AID of a SE applet > + * @return Promise which is resolved to true if access should be allowed, > + * false otherwise. 1.Nits. Align description 2.Add comments when Promise is rejected @@ +20,5 @@ > + * @return Promise which is resolved to true if access should be allowed, > + * false otherwise. > + */ > + jsval isAccessAllowed(in unsigned long localId, in DOMString seName, > + in jsval aid); 1.Nits. Align parameter (in unsigned long localId, in DOMString seName, in jsval aid); 2.If my understanding is correct, Aid is Uint8Array passed from |openLogicalChannel| in SecureElement.webidl in idl we can use nsIVariant instead of jsval, following is an example: https://dxr.mozilla.org/comm-central/source/mozilla/dom/nfc/nsINfcContentHelper.idl#18 3.Use seType instead of seName to sync with |interface SEReader|

Comment on attachment 8554878 [details] [diff] [review] Part 1: ACEService.js and nsIAccessControlEnforcer.idl Review of attachment 8554878 [details] [diff] [review]: ----------------------------------------------------------------- Additional comment for part1 ::: dom/secureelement/gonk/ACEService.js @@ +133,5 @@ > + > +ACEService.prototype = { > + _ruleManager: null, > + > + isAccessAllowed: function isAccessAllowed(localId, seName, aid) { will seName be used in the future ? If yes, please create a follow up bug for that and add todo and bug number on comment @@ +174,5 @@ > + // TODO verify GUID signature > + // TODO compute the hash of the cert and possibly store it for future use > + > + // right now we have the cert hash included in manifest file > + // TODO remove this once we have fixed all the todos Please add bug number for todo here

Comment on attachment 8554886 [details] [diff] [review] Part 2: GPAccessRulesManager.js and nsIAccessRulesManager.idl Review of attachment 8554886 [details] [diff] [review]: ----------------------------------------------------------------- feeback- mainly because of two reason. 1. Lack of generic connector design. There should be one as describe in https://bug879861.bugzilla.mozilla.org/attachment.cgi?id=8456179 2. Too many magic number in this patch, try to document them and use definition as many as possible. ::: dom/secureelement/gonk/GPAccessRulesManager.js @@ +25,5 @@ > +Cu.import("resource://gre/modules/systemlibs.js"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "UiccConnector", > + "@mozilla.org/secureelement/connector/uicc;1", > + "nsISecureElementConnector"); Not address yoshi's comment in previous patch. The connector could be connected to eSE, Uicc, ASSD. But from this patch current architecture is not extensible if we want to support different type secure element. @@ +52,5 @@ > + // see for more details: http://www.cardwerk.com/smartcards/ > + // smartcard_standard_ISO7816-4_6_basic_interindustry_commands.aspx > + READ_BINARY: [0x00, 0xB0, 0x00, 0x00], > + GET_RESPONSE: [0x00, 0xC0, 0x00, 0x00], > + SELECT_BY_DF: [0x00, 0xA4, 0x00, 0x04, 0x02], If my understanding is correct, these value comes from ISO 7816-4 6.11.3 Command message 0xA4 is in Table 57, 0x00 is in table 58, 0x04 is in Table 59, I would suggest we add definition of these values somewhere in this file or create another to define this(personally prefer this way). Ex. const CLA_SM = 0x00 // Secure messaging const LC_EMPTY = 0x00 // ISO 7816-4 6.11.3 Command message table 57 - table 59 const INS_SF = 0xA4; const P1_SF_DF = 0x00; const P2_SF_FR = 0x04; // First Record ??? so SELECT_BY_DF: [CLA_SM, CLA_SF, P1_SF_DF, P2_SF_FR, 0x02], if 0x02 is calculated based on ODF_DF = [0x50, 0x31]; , please do concat in code Also for READ_BINARY, GET_RESPONSE @@ +198,5 @@ > + ); > + }); > + }, > + > + _readBinaryFile: function _readBinaryFile(selectResponse) { _readBinaryFile looks like a general util function, is the naming of |selectResponse| also general? @@ +216,5 @@ > + _selectAndRead: function _selectAndRead(df, cb) { > + return this._exchangeAPDU(this.SELECT_BY_DF.concat(df)) > + .then((resp) => this._readBinaryFile(resp)); > + }, > + Could you add some comments about what is ODF and DODF, thanks Ex. // Object Directory File (ODF) is an elementary file, which contains pointers to other EFs // Detail is specified in PKCS#15 section 6.6. @@ +220,5 @@ > + > + _readODF: function _readODF() { > + debug("_readODF"); > + // ODF DF, as specified in PKCS#15 section 6.6. > + let ODF_DF = [0x50, 0x31]; How does this come from , from PKCS#15 6.7 File Identifier ? If yes, please also define it in this same place as SELECT_BY_DF and add comments // PKCS#15 6.7 File Identifiers const ODF_DF = [0x50, 0x31] @@ +227,5 @@ > + > + _readDODF: function _readDODF(odfFile) { > + debug("_readDODF, ODF file: " + odfFile); > + > + let DODF_DF = odfFile[0xA7][0x30][0x04]; where does [0xA7][0x30][0x04] come from ? same as below like record[0xA1][0x30][0x06], gpdRecords[0][0xA1][0x30][0x30][0x04]; acMainFile[0x30][0x30][0x04]; could you document where does this magic number is defined and if possible please use a definition for it @@ +278,5 @@ > + > + let acRules = this._ensureIsArray(acRulesFile[0x30]); > + if (acRules.length === 0) { > + debug("No rules found in ACRules file."); > + return; Promise.reject ? @@ +291,5 @@ > + acRules.forEach((ruleEntry) => { > + let df = ruleEntry[0x30][0x04]; > + > + acReadQueue = acReadQueue.then((conditionFiles) => { > + if (df in conditionFiles) { use |if(conditionFiles[df] !== undefined)| for better performance @@ +334,5 @@ > + throw Error("Unknown applet definition"); > + } > + > + if (condition === null) { > + rule.application = "denied-all"; Use definition in IDL mentioned in patch part1 @@ +437,5 @@ > + apdu.data = (data.length) ? SEUtils.byteArrayToHexString(data) : null; > + return apdu; > + }, > + > + classID: Components.ID("{3e046b4b-9e66-439a-97e0-98a69f39f55f}"), Add contact ID

Hi Kamil, I will take PTO until Feb.2, Vincent will help review patches.

Comment on attachment 8554886 [details] [diff] [review] Part 2: GPAccessRulesManager.js and nsIAccessRulesManager.idl Review of attachment 8554886 [details] [diff] [review]: ----------------------------------------------------------------- Another question about patch part2. Currently we are reading rules from ARF, but is it possible we will have to support ARA-M in the future. If it is possible, please also make the architecture extensible for retrieving rule from ARA-M.

(In reply to Dimi Lee[:dimi][:dlee] from comment #45) > Comment on attachment 8554886 [details] [diff] [review] > Part 2: GPAccessRulesManager.js and nsIAccessRulesManager.idl > > Review of attachment 8554886 [details] [diff] [review]: > ----------------------------------------------------------------- > > Another question about patch part2. > Currently we are reading rules from ARF, but is it possible we will have to > support ARA-M in the future. > If it is possible, please also make the architecture extensible for > retrieving rule from ARA-M. Hi Dimi, Based on the existing SE implementation architecture(see attachment: https://bug884594.bugzilla.mozilla.org/attachment.cgi?id=8555765), the SE stack should be easily extendable to support the new access rule retrieval interface, such as ARA/M. The new implementation shall follow the agreed abstract interface "nsIAccessRuleManager.idl". With the help of two abstraction interfaces, namely, nsIAccessRuleManager and nsISecureElementConnector.idl, we can on the one hand integrate different SE access and rule access approaches; on the other hand, the SE parts(both in content/parent) keep unchanged, thanks to the 3rd. abstraction interface - nsIAccessControlEnforcer.idl. Ming

(In reply to Ming Yin from comment #47) > (In reply to Dimi Lee[:dimi][:dlee] from comment #45) > > Comment on attachment 8554886 [details] [diff] [review] > > Part 2: GPAccessRulesManager.js and nsIAccessRulesManager.idl > > > > Review of attachment 8554886 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Another question about patch part2. > > Currently we are reading rules from ARF, but is it possible we will have to > > support ARA-M in the future. > > If it is possible, please also make the architecture extensible for > > retrieving rule from ARA-M. > > Hi Dimi, > > Based on the existing SE implementation architecture(see attachment: > https://bug884594.bugzilla.mozilla.org/attachment.cgi?id=8555765), the SE > stack should be easily extendable to support the new access rule retrieval > interface, such as ARA/M. The new implementation shall follow the agreed > abstract interface "nsIAccessRuleManager.idl". > > With the help of two abstraction interfaces, namely, nsIAccessRuleManager > and nsISecureElementConnector.idl, we can on the one hand integrate > different SE access and rule access approaches; on the other hand, the SE > parts(both in content/parent) keep unchanged, thanks to the 3rd. abstraction > interface - nsIAccessControlEnforcer.idl. > Ming Hi Ming, Thanks for the diagram, the abstract interface for different access rule is nsIAccessRuleManager and its implementation is ini GPAccessRulesManager.js. What i would like to address in Comment 45 is about implementation, implementation in GPAccessRulesManager.js should be extensible.

Great review, very informative - thanks Dimi! (In reply to Dimi Lee[:dimi][:dlee] from comment #41) > > + let appMatches = rule.application === "allowed-all" || > > + rule.application === "denied-all"; > > Please define "allow-all" and "denied-all" in nsIAccessRulesManager > Then use Ci.nsIAccessRulesManager.ALLOW_ALL & > Ci.nsIAccessRulesManager.DENIED_ALL Done. I just had to use "unsigned short" type instead of DOMString, as the latter caused the build errors. It seems like const's cannot be of the DOMString type. > > I wrote a pseudo code for the GPD 4.2.3. > it combine 4 _xxxAppletsxxxApps function and may just need to call > |this.rules.filter| once > (Trying to reduce that because each array filter call will create a new > array). > Please check if it could improve the code here. > > // GPD 4.2.3 Algorithm for applying rules > let decision = this.rules.filter(this._searchForRules.bind(this)); > // I do not quite understand the logic here( > 0 and === 1) but just follow > the original implemetation > if (decision.length > 0 && > decision[0].application === Ci.nsIAcessRulesManager.ALLOW_ALL) { > return true; > } else if (decision.length === 1 && > Array.isArray(decision[0].application)) { > return true; > } > > _searchForRules: function _searchForRules(rule) { > let appMatched; > let appletMatched; > > // 4.2.3 A, 4.2.3 C > if (Array.isArray(rule.application)) { > appMatched = this._applicationMatched(rule.application); > // 4.2.3 B, 4.2.3 D > } else { > appMatched = [Ci.nsIAcessRulesManager.ALLOW_ALL, > Ci.nsIAcessRulesManager.DENIED_ALL] > .indexOf(rule.application) != -1; > } > > // 4.2.3 A, 4.2.3 B > if (Array.isArray(rule.applet)) { > appletMatched = SEUtils.arraysEqual(rule.applet, this.aid); > // 4.2.3 C, 4.2.3 D > } else { > appletMatched = rule.applet === Ci.nsIAcessRulesManager.ALL_APPLET; > } > > return appMatched && appletMatched; > } Neat! I used that, and ran it against all my unit tests - and all passed :) So your algorithm is included in the next patch. > > +function ACEService() { > > + this._ruleManager = > > Nits. sync naming, _rulesManager Done. > > @@ +136,5 @@ > > + > > + isAccessAllowed: function isAccessAllowed(localId, seName, aid) { > > + let manifestURL = DOMApplicationRegistry.getManifestURLByLocalId(localId); > > + if (!manifestURL) { > > + return Promise.reject("Missing manifest for app: " + localId); > > Promise.reject(new Error("Missing manifest for app: " + localId)); Done. > > Usually we declare function directly inside |new Promise|. > so > > return new Promise((resolve, reject) => { > debug("isAccessAllowed for " + manifestURL + " to " + aid); > ... > }); Done. > > + > > + classID: Components.ID("{882a7463-2ca7-4d61-a89a-10eb6fd70478}"), > > Add contractID: Done. > > ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl > @@ +17,5 @@ > > + * @param localId ID of an application accessing SE > > + * @param seName Name of the SE. > > + * @param aid AID of a SE applet > > + * @return Promise which is resolved to true if access should be allowed, > > + * false otherwise. > > 1.Nits. Align description > 2.Add comments when Promise is rejected Done. > > @@ +20,5 @@ > > + * @return Promise which is resolved to true if access should be allowed, > > + * false otherwise. > > + */ > > + jsval isAccessAllowed(in unsigned long localId, in DOMString seName, > > + in jsval aid); > > 1.Nits. Align parameter > (in unsigned long localId, > in DOMString seName, > in jsval aid); Done. > > 2.If my understanding is correct, Aid is Uint8Array passed from > |openLogicalChannel| in SecureElement.webidl > in idl we can use nsIVariant instead of jsval, following is an example: > > https://dxr.mozilla.org/comm-central/source/mozilla/dom/nfc/ > nsINfcContentHelper.idl#18 Done. > > 3.Use seType instead of seName to sync with |interface SEReader| Done. > @@ +133,5 @@ > > + > > +ACEService.prototype = { > > + _ruleManager: null, > > + > > + isAccessAllowed: function isAccessAllowed(localId, seName, aid) { > > will seName be used in the future ? > If yes, please create a follow up bug for that and add todo and bug number > on comment I left this one as a next step for now, as it's related to the "extensibility" you and Yoshi mentioned. Will comment on that in a second (below). > > @@ +174,5 @@ > > + // TODO verify GUID signature > > + // TODO compute the hash of the cert and possibly store it for future use > > + > > + // right now we have the cert hash included in manifest file > > + // TODO remove this once we have fixed all the todos > > Please add bug number for todo here Done. > > Not address yoshi's comment in previous patch. > The connector could be connected to eSE, Uicc, ASSD. But from this patch > current architecture is not > extensible if we want to support different type secure element. Will address that in a second (below). > > @@ +52,5 @@ > > + // see for more details: http://www.cardwerk.com/smartcards/ > > + // smartcard_standard_ISO7816-4_6_basic_interindustry_commands.aspx > > + READ_BINARY: [0x00, 0xB0, 0x00, 0x00], > > + GET_RESPONSE: [0x00, 0xC0, 0x00, 0x00], > > + SELECT_BY_DF: [0x00, 0xA4, 0x00, 0x04, 0x02], > > If my understanding is correct, these value comes from ISO 7816-4 6.11.3 > Command message > 0xA4 is in Table 57, 0x00 is in table 58, 0x04 is in Table 59, > > I would suggest we add definition of these values somewhere in this file or > create another to define this(personally prefer this way). > Ex. > > const CLA_SM = 0x00 // Secure messaging > const LC_EMPTY = 0x00 > > // ISO 7816-4 6.11.3 Command message table 57 - table 59 > const INS_SF = 0xA4; > const P1_SF_DF = 0x00; > const P2_SF_FR = 0x04; // First Record ??? > so > SELECT_BY_DF: [CLA_SM, CLA_SF, P1_SF_DF, P2_SF_FR, 0x02], > > if 0x02 is calculated based on ODF_DF = [0x50, 0x31]; , please do concat in > code > > Also for READ_BINARY, GET_RESPONSE Done. I opted for storing them in the same file, as I don't think they'll be useful outside of it. > > @@ +198,5 @@ > > + ); > > + }); > > + }, > > + > > + _readBinaryFile: function _readBinaryFile(selectResponse) { > > _readBinaryFile looks like a general util function, is the naming of > |selectResponse| also general? Yes. The way it works is that first we SELECT a file (by DF) and then use the response we get to read the whole file. > > @@ +216,5 @@ > > + _selectAndRead: function _selectAndRead(df, cb) { > > + return this._exchangeAPDU(this.SELECT_BY_DF.concat(df)) > > + .then((resp) => this._readBinaryFile(resp)); > > + }, > > + > > Could you add some comments about what is ODF and DODF, thanks > Ex. > // Object Directory File (ODF) is an elementary file, which contains > pointers to other EFs > // Detail is specified in PKCS#15 section 6.6. I added some extensive comments for ODF, DODF and ACMF as well. Those comments include examples of those files, properly formatted as a nested TLV structure. This also explains the "magic" numbers we have. > > @@ +220,5 @@ > > + > > + _readODF: function _readODF() { > > + debug("_readODF"); > > + // ODF DF, as specified in PKCS#15 section 6.6. > > + let ODF_DF = [0x50, 0x31]; > > How does this come from , from PKCS#15 6.7 File Identifier ? > If yes, please also define it in this same place as SELECT_BY_DF and add > comments > > // PKCS#15 6.7 File Identifiers > const ODF_DF = [0x50, 0x31] Done. > > @@ +227,5 @@ > > + > > + _readDODF: function _readDODF(odfFile) { > > + debug("_readDODF, ODF file: " + odfFile); > > + > > + let DODF_DF = odfFile[0xA7][0x30][0x04]; > > where does [0xA7][0x30][0x04] come from ? > same as below like > record[0xA1][0x30][0x06], > gpdRecords[0][0xA1][0x30][0x30][0x04]; > acMainFile[0x30][0x30][0x04]; > > could you document where does this magic number is defined and if possible > please use a definition for it I opted not for const'ing them because each of those is used only in one place. Instead, I included examples of ODF, DODF and ACMF files with some comments, so the meaning of those tags should be clear now. > > @@ +278,5 @@ > > + > > + let acRules = this._ensureIsArray(acRulesFile[0x30]); > > + if (acRules.length === 0) { > > + debug("No rules found in ACRules file."); > > + return; > > Promise.reject ? Done. > > @@ +291,5 @@ > > + acRules.forEach((ruleEntry) => { > > + let df = ruleEntry[0x30][0x04]; > > + > > + acReadQueue = acReadQueue.then((conditionFiles) => { > > + if (df in conditionFiles) { > > use |if(conditionFiles[df] !== undefined)| for better performance Done. > > @@ +334,5 @@ > > + throw Error("Unknown applet definition"); > > + } > > + > > + if (condition === null) { > > + rule.application = "denied-all"; > > Use definition in IDL mentioned in patch part1 Done. > > @@ +437,5 @@ > > + apdu.data = (data.length) ? SEUtils.byteArrayToHexString(data) : null; > > + return apdu; > > + }, > > + > > + classID: Components.ID("{3e046b4b-9e66-439a-97e0-98a69f39f55f}"), > > Add contact ID Done. The one comment from you and Yoshi not yet addressed is the extensibility over other SE types. In the next round of feedback I'll include that - I plan to do that similarly to what is being done in SE implementation, which is to use seType to use the correct implementation. As for ARA-M, there's very little to do in terms of making GPAccessRulesManager ready for supporting it. ARA-M support will mean exchanging some more APDU's at the beggining. It should be trivial, and those steps should be added to _readAccessRules() - which is using promises and generators, so very easy to modify and reason about. I'll upload patches in a minute.

For the Dimi's and Yoshi's absence, I'd like to ask you Vincent for feedback. All nits should be addresses besides one - the extensibility over other SE types. I'll work on that next. Asking for feedback now to speed up the process. Thanks!

The GPAccessRulesManager. See comment above.

Comment on attachment 8556772 [details] [diff] [review] Part 1: ACEService.js and nsIAccessControlEnforcer.idl Review of attachment 8556772 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/ACEService.js @@ +42,5 @@ > + > +GPAccessDecision.prototype = { > + isAccessAllowed: function isAccessAllowed() { > + let decision = this.rules.filter(this._searchForRules.bind(this)); > + In your previous patch you have implemented a check according to (GPD 3.2.1) and that check is between 4.2.3 A and 4.2.3 B. I think you may still need check it and does that check MUST be between 4.2.3 A and B? If it does, we may need another way to handle it here.

Comment on attachment 8556772 [details] [diff] [review] Part 1: ACEService.js and nsIAccessControlEnforcer.idl Review of attachment 8556772 [details] [diff] [review]: ----------------------------------------------------------------- Hi Kamil, thanks for your new patches. Overall looks good, I saw most of the comments are addressed. ::: dom/secureelement/gonk/ACEService.js @@ +5,5 @@ > +/* Copyright © 2015, Deutsche Telekom, Inc. */ > + > +"use strict"; > + > +/* globals dump, Components, XPCOMUtils, ppmm, DOMApplicationRegistry, Nit: Capital letter in the beginning of a sentence. I think we can remove this comment. @@ +31,5 @@ > + * mostly in 3.1, 3.2 and 4.2.3. > + * > + * Since GPAccessRulesManager does not support APDU filters, decision making > + * algorithm does not support that either (should be straightforward to add > + * later, though). What is the APDU filter stands for? Do we need a follow-up bug for that? @@ +47,5 @@ > + if (!!decision.length && > + decision[0].application === Ci.nsIAccessRulesManager.ALLOW_ALL) { > + return true; > + } else if (!!decision.length && Array.isArray(decision[0].application)) { > + return true; Can we move this logic to _searchForRules()? We only need to check arrary length of decision, right? @@ +117,5 @@ > + return reject(Error("No developer certificate found.")); > + } > + > + this._rulesManager.getAccessRules() > + .then((rules) => { Check if we are failed to get access rules file or is there any error when trying to get access rules file. Is it possible that we get empty rules file? We can do early return in these cases. ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl @@ +18,5 @@ > + * > + * @param localId > + * ID of an application accessing SE > + * @param seName > + * Name of the SE. Nit: this should be seType, right?

Comment on attachment 8556773 [details] [diff] [review] Part 2: GPAccessRulesManager.js and nsIAccessRulesManager.idl Review of attachment 8556773 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/GPAccessRulesManager.js @@ +6,5 @@ > + > +"use strict"; > + > +/* globals Components, dump, XPCOMUtils, Promise, Task, SEUtils, libcutils, > + UiccConnector */ The comment is not necessary. @@ +63,5 @@ > + * as defined in section #7 of [1]: "Structure of Access Rule Files (ARF)". > + * Rules retrieval from ARA-M applet is not implmented due to lack of > + * commercial implemenations of ARA-M. > + * @todo implement ARA-M support according to section #4 of [1] > + * @todo const used tags Please open the follow-up bugs for TODO items. @@ +103,5 @@ > + > + return new Promise((resolve, reject) => { > + this._readAccessRules(() => resolve(this.rules)); > + }); > + }, Do we really need to call "init" function to get ARF? Why do we need to have "init" and "getAccessRules" doing similar things? The only difference is "init" doesn't pass "this.rules" in "resolve" while "getAccessRules" does. Not sure if we can cache the ARF in "this.rules", and we can update "this.rules" whenever we receive the notification from security element that ARF is modified. Is it possible to do that? @@ +105,5 @@ > + this._readAccessRules(() => resolve(this.rules)); > + }); > + }, > + > + _readAccessRules: Task.async(function*(done) { Using Task.async and promise are really fantastic. It makes implementation more readable. Since this part is more Global Platform specific. I will let Yoshi to review it. However, it would be good if we can abstract a little bit more in send/response and parser things.

Hey, How's it going? I haven't seen update for quite a while.

This bug will need to be separated into something that fully supports WebCrypto and just the bits that do work without full support. The OEM needs to update the device image, although the basic ACE impl can probably still land (including digest calculation once the rule is retrieved from the SIM), minus developer signature verification.

per discussion in the weekly meeting, we're not confident to pass review and land before FL. Because of this consensus, I'm resetting the feature-b2g to 3.0?. We'll keep finishing the works on master/central.

Bug 1113054 is landed. We could move the development of ACE to 2.2/m-c on Nexus 5 L. Alison gives it a shot today and result looks good. Have time to try it?

Garner is working on this.

(In reply to Vincent Chang[:vchang] from comment #58) > Bug 1113054 is landed. We could move the development of ACE to 2.2/m-c on > Nexus 5 L. Alison gives it a shot today and result looks good. Have time to > try it? This is really good news. I'll try to prepare the build and do the testing with Nexus 5 early next week and let you know about the results.

This is not an xpcshell test. This patch has unit tests that runs in a normal browser until certs and signed guids are available.

(In reply to Vincent Chang[:vchang] from comment #53) > Comment on attachment 8556772 [details] [diff] [review] > Part 1: ACEService.js and nsIAccessControlEnforcer.idl > > Review of attachment 8556772 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hi Kamil, thanks for your new patches. Overall looks good, I saw most of the > comments are addressed. > > ::: dom/secureelement/gonk/ACEService.js > @@ +5,5 @@ > > +/* Copyright © 2015, Deutsche Telekom, Inc. */ > > + > > +"use strict"; > > + > > +/* globals dump, Components, XPCOMUtils, ppmm, DOMApplicationRegistry, > > Nit: Capital letter in the beginning of a sentence. > I think we can remove this comment. > Removed. > @@ +31,5 @@ > > + * mostly in 3.1, 3.2 and 4.2.3. > > + * > > + * Since GPAccessRulesManager does not support APDU filters, decision making > > + * algorithm does not support that either (should be straightforward to add > > + * later, though). > > What is the APDU filter stands for? Do we need a follow-up bug for that? Application Protocol Data Unit. [Kamil] APDU filters are defined in GPD spec. Rules which contain filter mean, that those rules are applicable only to the subset of APDU's exchanged. Filtering criteria is based on CLA, INS, P1, P2 and a payload. If an exchanged APDU is matching the filter, rule applies. Otherwise, rule is ignored for that APDU. See GPD spec 7.1.7 for instance. Having that said, APDU filters are meaningful when applying access control for exchange APDU operation. What we're doing now, is that we're only applying ACE for openLogicalChannel operation. The follow up bug for that is a good idea. > > @@ +47,5 @@ > > + if (!!decision.length && > > + decision[0].application === Ci.nsIAccessRulesManager.ALLOW_ALL) { > > + return true; > > + } else if (!!decision.length && Array.isArray(decision[0].application)) { > > + return true; > > Can we move this logic to _searchForRules()? We only need to check arrary > length of decision, right? Modified, and moved to _searchForRules. > > @@ +117,5 @@ > > + return reject(Error("No developer certificate found.")); > > + } > > + > > + this._rulesManager.getAccessRules() > > + .then((rules) => { > > Check if we are failed to get access rules file or is there any error when > trying to get access rules file. > Is it possible that we get empty rules file? We can do early return in these > cases. [Kamil] No, it shouldn't happen. According to GPD spec, if the files found on the SE (such as ACMF, ACRF etc) are found to be missing, invalid or incorrectly formatted, or if those files could not be reached, access should be denied. GPAccessRulesManager behaves in this exact way, and if any unexpected thing (like mentioned above) happen, GPAccessRulesManager's "rules" property will be empty, thus effectively making access denied. > > ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl > @@ +18,5 @@ > > + * > > + * @param localId > > + * ID of an application accessing SE > > + * @param seName > > + * Name of the SE. > > Nit: this should be seType, right? Updated.

(In reply to Vincent Chang[:vchang] from comment #54) > Comment on attachment 8556773 [details] [diff] [review] > Part 2: GPAccessRulesManager.js and nsIAccessRulesManager.idl > > Review of attachment 8556773 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/secureelement/gonk/GPAccessRulesManager.js > @@ +6,5 @@ > > + > > +"use strict"; > > + > > +/* globals Components, dump, XPCOMUtils, Promise, Task, SEUtils, libcutils, > > + UiccConnector */ > > The comment is not necessary. Gone. > > @@ +63,5 @@ > > + * as defined in section #7 of [1]: "Structure of Access Rule Files (ARF)". > > + * Rules retrieval from ARA-M applet is not implmented due to lack of > > + * commercial implemenations of ARA-M. > > + * @todo implement ARA-M support according to section #4 of [1] > > + * @todo const used tags > > Please open the follow-up bugs for TODO items. Will open. > > @@ +103,5 @@ > > + > > + return new Promise((resolve, reject) => { > > + this._readAccessRules(() => resolve(this.rules)); > > + }); > > + }, > > Do we really need to call "init" function to get ARF? Why do we need to have > "init" and "getAccessRules" doing similar things? The only difference is > "init" doesn't pass "this.rules" in "resolve" while "getAccessRules" does. > Not sure if we can cache the ARF in "this.rules", and we can update > "this.rules" whenever we receive the notification from security element that > ARF is modified. > Is it possible to do that? [Kamil] The only reason to call .init() is to pre-fetch rules. This is beneficial, because we then discover the value of the refresh tag. So if some application will actually attempt to open a logical channel, the time of doing so will be shorter, because we will only read ODF, DODF and ACMF, instead of reading ACRF and ACCondition files. This could reduce the ACE overhead from about 500ms to 300ms (on our test SIM card).

Comment on attachment 8570215 [details] [diff] [review] Part 2 - GPAccessRulesManager and nsIAccessRulesManager.idl Review of attachment 8570215 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/GPAccessRulesManager.js @@ +192,5 @@ > + debug("Channel not opened"); > + return Promise.reject(); > + } > + > + debug("apdu " + JSON.stringify(bytes)); Add DEBUG @@ +224,5 @@ > + }, > + > + _readBinaryFile: function _readBinaryFile(selectResponse) { > + DEBUG && debug("Select response: " + JSON.stringify(selectResponse)); > + let fileLength = selectResponse[0x62][0x80]; Could you specify where 0x62, 0x80 comes from ? @@ +236,5 @@ > + // let readApdu = this.READ_BINARY.concat(fileLength); > + return this._exchangeAPDU(this.READ_BINARY); > + }, > + > + _selectAndRead: function _selectAndRead(df, cb) { Do we use |cb| parameter ? @@ +256,5 @@ > + // A7 06 > + // 30 04 > + // 04 02 XY WZ > + // where [0xXY, 0xWZ] is a DF of DODF file. > + let DODF_DF = odfFile[0xA7][0x30][0x04]; If i understand correctly , I assume those number like 0xA7, 0x30, 0x04, 0xA1, 0x82 ...etc occured in *odfFile[0xA7][0x30][0x04]; *dodfFile[0xA1] *acMainFile[0x30][0x30][0x04]; *ruleEntry[0x82]; ...etc are all defined "Tag" value mentioned in following code let containerTags = [ // ASN.1 sequence 0x30, // File control parameters (FCP) - ISO 7816 0x62, // AID in the EF-ACRules - GPD spec. 0xA0, // External data objects - PKCS#15 0xA1, // Indirect value. 0xA5, // EF_ODF data objects. 0xA7 ]; So could we define these tags and add comment to address where does it come from like what we already did in the beginning of the file? Ex. /* ISO 7816-4: get response, 7.1.3 table 74 */ const P1_GR = 0x00; const P2_GR = 0x00; @@ +472,5 @@ > + } else if (Array.isArray(result[tag])) { > + result[tag].push(parsed); > + } else { > + result[tag] = [result[tag], parsed]; > + } I am confused about the |else if| and |else| case here, could you explain a little bit here ? thanks ! Why do we push |parsed| to the value of an parsed tag and what does the |[result[tag], parsed]| stand for ?

Comment on attachment 8570205 [details] [diff] [review] Part 1 - ACEService and nsIAccessControlEnforcer.idl Review of attachment 8570205 [details] [diff] [review]: ----------------------------------------------------------------- f+, but you will still need review from yoshi

Comment on attachment 8570215 [details] [diff] [review] Part 2 - GPAccessRulesManager and nsIAccessRulesManager.idl f- because i would like to see comment addressed or explained

Comment on attachment 8570220 [details] [diff] [review] Part 4 - Initial integration with ACE Review of attachment 8570220 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, f? to me again when comment is addressed. ::: dom/secureelement/gonk/SecureElement.js @@ +194,5 @@ > this._registerMessageListeners(); > Services.obs.addObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false); > + this._accessControlEnforcer = > + Cc["@mozilla.org/secureelement/access-control/ace;1"] > + .getService(Ci.nsIAccessControlEnforcer); reset |this._accessControlEnforcer| in _shutdown @@ +209,5 @@ > interfaces: [Ci.nsIMessageListener, > Ci.nsIObserver] > }), > > + _accessControlEnforcer: null, shorter name, ace or aceService, or acEnforcer @@ +273,5 @@ > > + this._accessControlEnforcer.isAccessAllowed(msg.appId, msg.type, msg.aid) > + .then((allowed) => { > + if (!allowed) { > + if (callback) { We didn't check |callback| in the other place of this function, I think we can either check |callback| in the beginning or skip check here @@ +279,5 @@ > } > + return; > + } > + let connector = getConnector(msg.type); > + if (!connector) { Nits. Not align

Comment on attachment 8570218 [details] [diff] [review] Part 3 - Build support for ACE components Fix bad description.

Reminder for updating MDN for app dev actions if they want to access SE.

(In reply to Dimi Lee[:dimi][:dlee] from comment #68) > Comment on attachment 8570215 [details] [diff] [review] > Part 2 - GPAccessRulesManager and nsIAccessRulesManager.idl > > Review of attachment 8570215 [details] [diff] [review]: > I am confused about the |else if| and |else| case here, could you explain a > little bit here ? thanks ! > Why do we push |parsed| to the value of an parsed tag and what does the > |[result[tag], parsed]| stand for ? I believe the main reason for |else if| (append) was to allow a "flatter" internal parsed structure that's easier to use. Some rules have more than one array entry per tag, but the generic _parseTLV is used for more than just rules. See _readACMF.

Comment on attachment 8577029 [details] [diff] [review] Bug 884594: Part 1 - ACEService and nsIAccessControlEnforcer.idl Review of attachment 8577029 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/ACEService.js @@ +48,5 @@ > + let decision = this.rules.some(this._searchForRules.bind(this)); > + return decision; > + }, > + > + _searchForRules: function _searchForRules(rule) { We should rename this function because the return value now is decision instead of rule ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl @@ +22,5 @@ > + * Type of the SE. > + * @param aid > + * AID of a SE applet > + * @return Promise which is resolved to true if access should be allowed, > + * false otherwise, and rejected if application contain no developer contains

Comment on attachment 8577033 [details] [diff] [review] Bug 884594: Part 4 - Initial integration with ACE Review of attachment 8577033 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/SecureElement.js @@ +296,5 @@ > > + notifyError: (reason) => { > + debug("Failed to open the channel to AID : " + > + SEUtils.byteArrayToHexString(msg.aid) + > + ", Rejected with Reason : " + reason); Align

Comment on attachment 8577031 [details] [diff] [review] Bug 884594: Part 2 - GPAccessRulesManager and nsIAccessRulesManager.idl Review of attachment 8577031 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good to me, thanks Please fix nits and ask yoshi to review again ::: dom/secureelement/gonk/GPAccessRulesManager.js @@ +155,5 @@ > + this.refreshTag = refreshTag; > + debug("_readAccessRules: refresh tag saved: " + this.refreshTag); > + > + let acrf = yield this._readACRules(acmf); > + let condFiles = yield this._readACConditions(acrf); I think this is Access Control Conditions File ? If yes, rename to accf @@ +173,5 @@ > + }), > + > + _openChannel: function _openChannel() { > + if (this.channel !== null) { > + DEBUG && debug("_openChannel: Channel already opened, rejecting."); why add DEBUG here ? @@ +196,5 @@ > + }, > + > + _closeChannel: function _closeChannel() { > + if (this.channel === null) { > + DEBUG && debug("_closeChannel: Channel not opened, rejecting."); ditto. @@ +219,5 @@ > + }, > + > + _exchangeAPDU: function _exchangeAPDU(bytes) { > + if (this.channel === null) { > + DEBUG && debug("Channel not opened"); ditto. @@ +236,5 @@ > + debug("APDU response is " + sw1.toString(16) + sw2.toString(16) + > + " data: " + data); > + > + // 90 00 is "success" > + if (sw1 !== 0x90 || sw2 !== 0x00) { extra space @@ +405,5 @@ > + > + return acReadQueue; > + }, > + > + _parseRules: function _parseRules(acRulesFile, conditionFiles) { acConditionsFile or acConditionsFiles to sync the naming policy @@ +416,5 @@ > + DEBUG && debug("Parsing one rule: " + JSON.stringify(ruleEntry)); > + let rule = {}; > + > + let df = ruleEntry[TAG_SEQUENCE][TAG_OCTETSTRING]; > + let condition = conditionFiles[df]; Declare |df| and |condition| when it is used @@ +430,5 @@ > + } else if (allApplets) { > + rule.applet = Ci.nsIAccessRulesManager.ALL_APPLET; > + } else { > + throw Error("Unknown applet definition"); > + } Is it possible that both |oneApplet| and |allApplets| exist, if yes, do we handle it correctly now ?

(In reply to Dimi Lee[:dimi][:dlee] from comment #82) > Comment on attachment 8577031 [details] [diff] [review] > Bug 884594: Part 2 - GPAccessRulesManager and nsIAccessRulesManager.idl > > Review of attachment 8577031 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +430,5 @@ > > + } else if (allApplets) { > > + rule.applet = Ci.nsIAccessRulesManager.ALL_APPLET; > > + } else { > > + throw Error("Unknown applet definition"); > > + } > > Is it possible that both |oneApplet| and |allApplets| exist, if yes, do we > handle it correctly now ? Thanks for the feeedback. According to the GPD SE Access Control v1.1, section C,2 and C.3, the ruleset is parsed one condition entry line at a time (there's no room for 2 at a time), then pushed to the "rules" array.

Fix feedback comments: Renamed _searchForRules to _decideAppAccess, and fixed spelling.

Nits fixed. Renamed "condFiles" to accf (Access Control Conditions File).

Alignment nit fixed.

Comment on attachment 8577512 [details] [diff] [review] Part 1 - ACEService and nsIAccessControlEnforcer.idl Review of attachment 8577512 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/ACEService.js @@ +29,5 @@ > + } > +} > + > +/** > + * Implements decision making algorithm as described in GPD specification, /** * @@ +35,5 @@ > + * > + * Since GPAccessRulesManager does not support APDU filters, decision making > + * algorithm does not support that either (should be straightforward to add > + * later, though). > + * TODO: Bug 1137533 Cut this paragraph and make it shorter, like TODO: Bug 1137533 : Add support for APDU fillter. @@ +45,5 @@ > +} > + > +GPAccessDecision.prototype = { > + isAccessAllowed: function isAccessAllowed() { > + let decision = this.rules.some(this._decideAppAccess.bind(this)); some? @@ +52,5 @@ > + > + _decideAppAccess: function _decideAppAccess(rule) { > + let appMatched, appletMatched, ruleAllows; > + > + // GPD 4.2.3 A and 4.2.3 C Add a simple explanation for this. @@ +68,5 @@ > + appletMatched = SEUtils.arraysEqual(rule.applet, this.aid); > + // GPD 4.2.3 C and 4.2.3 D > + } else { > + appletMatched = rule.applet === Ci.nsIAccessRulesManager.ALL_APPLET; > + } Those checks above seems strange, The checks are different when rule.apple/rule.application is array or not. @@ +71,5 @@ > + appletMatched = rule.applet === Ci.nsIAccessRulesManager.ALL_APPLET; > + } > + > + // Access is allowed, if rule does not deny access to an app. > + ruleAllows = rule.application !== Ci.nsIAccessRulesManager.DENY_ALL; in line 61 you already checked rule.application, now you check it again. @@ +76,5 @@ > + > + return appMatched && appletMatched && ruleAllows; > + }, > + > + _applicationMatches: function _applicationMatches(appArray) { _isHashMatched @@ +122,5 @@ > + return reject(Error("No developer certificate found")); > + } > + > + let rulesManager = this._rulesManagers.get(seType); > + if (!rulesManager) { this could move out of this Promise block. @@ +139,5 @@ > + }); > + }, > + > + _getDevCertHashForApp: function getDevCertHashForApp(manifestURL) { > + return DOMApplicationRegistry.getManifestFor(manifestURL) shouldn't this function return promise? return new Promise((resolver, reject) => { DOMApplicationRegistry.getManifestFor(manifestURL).... .. @@ +155,5 @@ > + }) > + .catch((error) => { > + debug("Not able to retrieve cert hash: " + error); > + return ""; > + }); Whether it is succeed or not this function will always return ""; @@ +159,5 @@ > + }); > + }, > + > + classID: Components.ID("{882a7463-2ca7-4d61-a89a-10eb6fd70478}"), > + contractID: "@mozilla.org/secureelement/access-control/ace;1", Unless you still have plan to add more contractIds, otherwise I think "@mozilla.org/secureelement/access-control;1; or "@mozilla.org/secureelement/ace;1 is fine. ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl @@ +27,5 @@ > + * no developer certificate. > + */ > + jsval isAccessAllowed(in unsigned long localId, > + in DOMString seType, > + in nsIVariant aid); SE API uses DOMString, why do you change to use nsIVariant?

Comment on attachment 8577513 [details] [diff] [review] Part 2 - GPAccessRulesManager and nsIAccessRulesManager.idl Review of attachment 8577513 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/GPAccessRulesManager.js @@ +123,5 @@ > + debug("init"); > + > + return new Promise((resolve, reject) => { > + this._readAccessRules(resolve); > + }); this function looks like not doing 'init' it is reading the rules already. @@ +126,5 @@ > + this._readAccessRules(resolve); > + }); > + }, > + > + getAccessRules: function getAccessRules() { Why don't you use readAccessRules directly? either remove this function, or unless you plan to cache the rules, then add a TODO @@ +136,5 @@ > + }, > + > + _readAccessRules: Task.async(function*(done) { > + try { > + yield this._openChannel(); this.PKCS_AID should be provided here. yield this._openChannel(this.PKCS_AID); @@ +145,5 @@ > + let acmf = yield this._readACMF(dodf); > + let refreshTag = acmf[this.REFRESH_TAG_PATH[0]] > + [this.REFRESH_TAG_PATH[1]]; > + > + if (SEUtils.arraysEqual(this.refreshTag, refreshTag)) { Will the refreshTag change by itself, without notifying gecko? Also it seems refreshTag is only saved, without doing furthur things. @@ +164,5 @@ > + > + yield this._closeChannel(); > + done(); > + } catch (error) { > + debug("Error: " + (error ? error.message : "undefined")); error? looks weird. will it report error but without any error message? @@ +236,5 @@ > + debug("APDU response is " + sw1.toString(16) + sw2.toString(16) + > + " data: " + data); > + > + // 90 00 is "success" > + if (sw1 !== 0x90 || sw2 !== 0x00) { Are you sure you need || here ? or you want && ? @@ +256,5 @@ > + }, > + > + _readBinaryFile: function _readBinaryFile(selectResponse) { > + DEBUG && debug("Select response: " + JSON.stringify(selectResponse)); > + /* 0x80 tag parameter - get the elementary file (EF) length // @@ +400,5 @@ > + acConditionFiles[df] = acConditionFileContents; > + return acConditionFiles; > + }); > + }); > + }); I saw three levels of clousures here I'd suggest you make it more flat or make it more easier to understand. @@ +412,5 @@ > + let rules = []; > + > + let acRules = this._ensureIsArray(acRulesFile[TAG_SEQUENCE]); > + acRules.forEach((ruleEntry) => { > + DEBUG && debug("Parsing one rule: " + JSON.stringify(ruleEntry)); You already do the debug() in line 410, now you did it again, there is too much log here, so either remove this debug or line 410. @@ +419,5 @@ > + // 0xA0 and 0x82 tags as per GPD spec sections C.1 - C.3. 0xA0 means > + // that rule describes access to one SE applet only (and its AID is > + // given). 0x82 means that rule describes acccess to all SE applets. > + let oneApplet = ruleEntry[TAG_GPD_AID]; > + let allApplets = ruleEntry[0x82]; Why did you const TAG_GPD_AID but not for 0x82? @@ +456,5 @@ > + return rules; > + }, > + > + /* Parse "Type Length Value" (TLV) from bytes. */ > + _parseTLV: function _parseTLV(bytes) { move these utils to SEUtils.jsm?

Comment on attachment 8577515 [details] [diff] [review] Part 4 - Initial integration with ACE Review of attachment 8577515 [details] [diff] [review]: ----------------------------------------------------------------- cancel r? for Part 1 and Part 2 need to be updated. ::: dom/secureelement/gonk/SecureElement.js @@ +274,5 @@ > + if (!connector) { > + debug("No SE connector available"); > + callback({ error: SE.ERROR_NOTPRESENT }); > + return; > + } this should be moved outside isAccessAllowed check

^ Thanks Yoshi! I'm working on fixing the commented areas.

(In reply to Yoshi Huang[:allstars.chh] from comment #88) > Comment on attachment 8577512 [details] [diff] [review] > Part 1 - ACEService and nsIAccessControlEnforcer.idl > > Review of attachment 8577512 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +159,5 @@ > > + }); > > + }, > > + > > + classID: Components.ID("{882a7463-2ca7-4d61-a89a-10eb6fd70478}"), > > + contractID: "@mozilla.org/secureelement/access-control/ace;1", > > Unless you still have plan to add more contractIds, > otherwise I think > "@mozilla.org/secureelement/access-control;1; or > "@mozilla.org/secureelement/ace;1 > is fine. I believe we agreed it will be fine as is. There's other contract ids with the same base. > > ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl > @@ +27,5 @@ > > + * no developer certificate. > > + */ > > + jsval isAccessAllowed(in unsigned long localId, > > + in DOMString seType, > > + in nsIVariant aid); > > SE API uses DOMString, why do you change to use nsIVariant? Sid, comments? It's not the actual SE content DOM interface (which is Uint8Array), but DOMString is part of the nsIIccProvider.idl interface below it (gecko parent, IPC'ed). It's nice to keep it asUint8Array though.

Hi Garner, Not sure if I understand the entire context here! But here is the flow. Hope it is useful! - Gaia Apps --> Gecko DOM ( Use Uint8Array via openLogicalChannel(...) in SecureElement.webidl) - Gecko DOM --> SecureElement Parent process (gonk) ( Continue to use 'Uint8Array') - SE Parent process --> UiccConnector (Converts Uint8Array to HexString) The last step where conversion from Uint8Array to a HexString is because of the interface definition in nsISecureElementConnector.idl 's openChannel(..). This interface was defined in this way to keep it consistent with 'nsIIccProvider.idl's iccOpenChannel(..) definition which accepts string for the 'aid'. (Note that eventually 'UiccConnector' talk to RILContentHelper that implements nsIIccProvider.idl interface)

Fixed review comments.

Removed init, which is the pre-cache function is done by ACEService directly. The "refreshTag" will have a separate message coming from the SE/SIM will likely need to be to be handled elsewhere. Here, the rules will be re-read completely only if the refreshTag check doesn't match. Bug 1119152 (isSEPresent) is open to track these state changes in a new IDL. Some other scenarios would be SE access rules updated OTA, and session closed (for whatever reason). Mildly flattened the acConditions function for more clarity (promise.then() until finished reading access control condition files). Moved the semi-generic parseTLV to SEUtils.

Move connector code outside Promise.

Comment on attachment 8584883 [details] [diff] [review] Part 1 - ACEService and nsIAccessControlEnforcer.idl Review of attachment 8584883 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/ACEService.js @@ +43,5 @@ > + > +GPAccessDecision.prototype = { > + isAccessAllowed: function isAccessAllowed() { > + // GPD SE Access Control v1.1, 3.4.1, Table 3-2 > + let decision = this.rules.some(this._decideAppAccess.bind(this)); what does some mean? @@ +53,5 @@ > + > + // GPD SE AC 4.2.3: Algorithm for Applying Rules > + // Specific rule overrides global rule. > + // > + // DeviceAppID is the application, and the SE app id is AID: I think you mean DeviceAppID is the ...., and AID is the .... @@ +72,5 @@ > + } > + // GPD SE AC 4.2.3 B and 4.2.3 D (All Device Applications) > + else { > + appMatched = rule.application === Ci.nsIAccessRulesManager.ALLOW_ALL; > + } appMatch = Array.isArray(rule.application) ? ... : ...; @@ +82,5 @@ > + } > + // GPD SE AC 4.2.3 C and 4.2.3 D (All AID) > + else { > + appletMatched = rule.applet === Ci.nsIAccessRulesManager.ALL_APPLET; > + } ditto @@ +84,5 @@ > + else { > + appletMatched = rule.applet === Ci.nsIAccessRulesManager.ALL_APPLET; > + } > + > + return appMatched && appletMatched; so if the appMatched is false you don't have to check rule.applet. bail out early. @@ +92,5 @@ > + if (!Array.isArray(appArray)) { > + return false; > + } > + > + return !!(appArray.find((hash) => { the argument of the callback in find should be the element of the array, which should be an instance of an 'app', however it's 'hash', what's in the appArray anyway? ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl @@ +27,5 @@ > + * no developer certificate. > + */ > + jsval isAccessAllowed(in unsigned long localId, > + in DOMString seType, > + in nsIVariant aid); aid is not of DOMString?

Comment on attachment 8584894 [details] [diff] [review] Part 2 - GPAccessRulesManager and nsIAccessRulesManager.idl Review of attachment 8584894 [details] [diff] [review]: ----------------------------------------------------------------- move SEUtils.jsm to another part with test cases included. ::: dom/secureelement/gonk/GPAccessRulesManager.js @@ +155,5 @@ > + let acrf = yield this._readACRules(acmf); > + let accf = yield this._readACConditions(acrf); > + this.rules = yield this._parseRules(acrf, accf); > + > + debug("_readAccessRules: done reading rules"); remove this debug you already do debug next line @@ +172,5 @@ > + _openChannel: function _openChannel(aid) { > + if (this.channel !== null) { > + debug("_openChannel: Channel already opened, rejecting."); > + return Promise.reject(Error("")); > + } try to check this.channel earlier so you don't have to check it in EVERY FUNCTION. @@ +228,5 @@ > + UiccConnector.exchangeAPDU(this.channel, apdu.cla, > + apdu.ins, apdu.p1, apdu.p2, apdu.data, apdu.le, > + { > + notifyExchangeAPDUResponse: (sw1, sw2, data) => { > + debug("_exchangeAPDU/notifyExchangeAPDUResponse"); remove @@ +466,5 @@ > + ]; > + return SEUtils.parseTLV(bytes, containerTags); > + }, > + > + _ensureIsArray: function _ensureIsArray(obj) { why not move to SEUtils?

Addressed comments. Also, AID is now a DOMString instead of Uint8Array.

Moved additional function to SEUtils.jsm. As for checking the channel, I've removed one from the multiple _exchangeAPDU calls only. Open and close are matched pairs otherwise. If removing even those two, it can potentially default what the lower layers return as and error if that is what you are requesting.

New xpcshell tests for SEUtils: parseTLV and ensureIsArray.

Comment on attachment 8591172 [details] [diff] [review] (v8) Bug 884594: Part 1 - ACEService and nsIAccessControlEnforcer.idl Review of attachment 8591172 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/gonk/ACEService.js @@ +55,5 @@ > + return decision; > + }, > + > + _decideAppAccess: function _decideAppAccess(rule) { > + let appMatched, appletMatched; define when used. @@ +79,5 @@ > + // GPD SE AC 4.2.3 B and 4.2.3 D (All Device Applications) > + rule.application === Ci.nsIAccessRulesManager.ALLOW_ALL; > + > + if (!appMatched) { > + return false; // bail early. // bail out early. @@ +89,5 @@ > + SEUtils.arraysEqual(rule.applet, this.aid) : > + // GPD SE AC 4.2.3 C and 4.2.3 D (All AID) > + rule.applet === Ci.nsIAccessRulesManager.ALL_APPLET; > + > + return appMatched && appletMatched; return appletMatched; @@ +165,5 @@ > + .then((manifest) => { > + DEBUG && debug("manifest retrieved: " + JSON.stringify(manifest)); > + > + // TODO: Bug 973823 > + // - verify if app is signed by marketplace Is this step still neccesary? I think the check (signed by marketplace) is already done during installation. ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl @@ +27,5 @@ > + * no developer certificate. > + */ > + jsval isAccessAllowed(in unsigned long localId, > + in DOMString seType, > + in nsIVariant aid); I think I asked this several times. Why does SE API use DOMString for aid and ACE used nsIVariant. r- for this is not clear to me.

Comment on attachment 8591173 [details] [diff] [review] (v8) Part 2.1 - GPAccessRulesManager and nsIAccessRulesManager.idl. Add utils to SEUtils.jsm. Review of attachment 8591173 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/SEUtils.jsm @@ +64,5 @@ > + * @param bytes - byte array > + * @param containerTags - byte array of tags > + */ > + parseTLV: function parseTLV(bytes, containerTags) { > + let result = {}; define when used. ::: dom/secureelement/gonk/GPAccessRulesManager.manifest @@ +1,2 @@ > +component {3e046b4b-9e66-439a-97e0-98a69f39f55f} GPAccessRulesManager.js > +contract @mozilla.org/secureelement/access-control/rules-manager/gp;1 {3e046b4b-9e66-439a-97e0-98a69f39f55f} Do we still have other components in @mozilla.org/secureelement/access-control/rules-manager/ ? otherwise I think 'gp' is not neccesary. ::: dom/secureelement/gonk/nsIAccessControlEnforcer.idl @@ +27,5 @@ > * no developer certificate. > */ > jsval isAccessAllowed(in unsigned long localId, > in DOMString seType, > + in DOMString aid); should be done in part 1.

(In reply to Yoshi Huang[:allstars.chh] from comment #103) > Comment on attachment 8591173 [details] [diff] [review] > (v8) Part 2.1 - GPAccessRulesManager and nsIAccessRulesManager.idl. Add > utils to SEUtils.jsm. > > Review of attachment 8591173 [details] [diff] [review]: > > +contract @mozilla.org/secureelement/access-control/rules-manager/gp;1 {3e046b4b-9e66-439a-97e0-98a69f39f55f} > > Do we still have other components in > @mozilla.org/secureelement/access-control/rules-manager/ ? > otherwise I think 'gp' is not neccesary. I can take it out. If another SE type has a different rule manager, we can trivially add it back. The GP spec doesn't seem to require a trusted computing base to be an SE on a UICC, but I'm not aware of what a ASSD, for example, would require for rules management (if not GP, something proprietary perhaps). The ACE code will initialize each "rule manager" separately, although there's actually only one instance, and also one type available currently.

Fixed review comments, and git commit grouping.

Added comment on where parseTLV is used (GPD primarily), removed gp from GPAccessRulesManager contractid.

Updated initial ACE integration. Transmit (Connector exchangeAPDU) now as a ACE check. Note the seType and AID comes from the channel object. Close connector doesn't do a check (It just closes, maliciously (rooted) or not).

Update mock for Components. The ACEService is a singleton service, and each SE GPAAccessRulesManager under it is a separate instance.

Try results with to be renumbered Bug 1132749 (Bug 973823) on top: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b80e5ff766f

If these patches land won't we block access to SE for all apps until developer signature bug is solved? I don't see any checks for 'devtools.debugger.forbid-certified-apps' flag. We agreed with Stephanie that if the phone is rooted ACE does not need to perform any checks and just allow access. I think that isAccessAllowed should in the first place check if the phone is rooted, if yes it should immediately resolve to true. We don't have APDU filtering implemented yet, so we don't need to call isAccessAllowed before transmit calls. I'm pretty sure it's impossible that access rules on the SIM card will be changed between the call to openChannel and transmit. I would propose to call isAccessAllowed only in openChannel and add a suitable comment in transmit.

I was hoping to have it in the second bug once a few extra details get worked out, but it does make sense to pull it into the initial landing for current SE app work (not prevent access by preference). The preference still disabled by default. Paul indicates it should clean wipe the phone upon change to reduce data exposure. From the meeting earlier, it seems like we can remove the transmit ACE isAccessAllowed certificate hash check, if SE has implemented APDU filters. I believe that needs to be clarified if that's not the case. Also, the APDU filter is not yet implemented, nor the Marketplace side signature check at the moment.

As this bug already exists for a while and the comments are already > 100, I am okay to land essential parts in this bug, and fix others in follow-up bugs. Please discuss how you plan to make these landed, and submit the patches for review again.

Flag check mentioned in comment 110 will be done as separate bug 1156710.

Another take on NFC: https://cyberphone.github.io/openkeystore/resources/docs/webnfc--web2device-bridge.pdf

Replace part 4, transmit ace check has been removed. Note (for now) to Bug 1137533, APDU filters.

(In reply to Yoshi Huang[:allstars.chh] from comment #112) > I am okay to land essential parts in this bug, and fix others in follow-up > bugs. > > Please discuss how you plan to make these landed, and submit the patches for > review again. The existing patches, if agreed, should land with the latest update. The dependant bugs, including the debug flag check (Bug 1156710, 'devtools.debugger.forbid-certified-apps'), will handle skipping the ACE check if the device is rooted. A current SE certified app developer, with root access, will need to disable the certificate check by removing the security check.

New try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d5810c2e74

Comment on attachment 8593115 [details] [diff] [review] (v8.1) Part 1 - ACEService and nsIAccessControlEnforcer.idl Review of attachment 8593115 [details] [diff] [review]: ----------------------------------------------------------------- I worried how much time it needs to spend on pulling those rules from start up, and will it increase start up time ? ::: dom/secureelement/gonk/ACEService.js @@ +119,5 @@ > + // Pre-cache SE rules > + this._rulesManagers.forEach((ruleManager, seType) => { > + ruleManager.getAccessRules().then(() => { > + debug("Rules manager instance for '" + seType + "' SE initialised"); > + }); Will this hurt start-up time? @@ +156,5 @@ > + let decision = new GPAccessDecision(rules, > + SEUtils.hexStringToByteArray(certHash), aid); > + > + resolve(decision.isAccessAllowed()); > + return; this 'return' is an no-op, remove it. @@ +173,5 @@ > + // - verify GUID signature > + // - compute the hash of the cert and possibly store it for future use > + // (right now we have the cert hash included in manifest file) > + // - remove this once we have fixed all the todos > + let certHash = manifest.secure_element_sig || ""; simply "" The name 'secure_element_sig' doesn't make sense. 1. secure_element_sig looks like a signature, so certHash sounds like a fingerprint from a certificate. so 'certHash = manifest.secure_element_sig' doesn't look right. 2. it should be a signature of guid signed by the developer.

Fix review comments. Changed manifest certificate hash name to "dev_cert_hash".

Sorry. I updated format of the last few patches (-U8).

(In reply to Yoshi Huang[:allstars.chh] from comment #118) > ----------------------------------------------------------------- > > I worried how much time it needs to spend on pulling those rules from start > up, and will it increase start up time ? > Read this.

(In reply to Yoshi Huang[:allstars.chh] from comment #126) > (In reply to Yoshi Huang[:allstars.chh] from comment #118) > > ----------------------------------------------------------------- > > > > I worried how much time it needs to spend on pulling those rules from start > > up, and will it increase start up time ? > > > Read this. Ah. I commented on the review comment directly. It should not slow down start up time, unless something like System app uses the particular SE directly at startup (as the first SE application to create the SE DOM). I can add a bug to make that a customizable preference (pre-cache or not), or simply remove it, and let the first app take to hit on loading the rules. It's not actually that slow, but the first wallet app SE use, for example, might find the first transaction takes some fractions of a second longer.

Do you want to have the pre-cache code removed for review (it doesn't affect anything except SE apps), or is the patch OK with Bug 1158024 created?

(In reply to Garner Lee from comment #127) > (In reply to Yoshi Huang[:allstars.chh] from comment #126) > > (In reply to Yoshi Huang[:allstars.chh] from comment #118) > > > ----------------------------------------------------------------- > > > > > > I worried how much time it needs to spend on pulling those rules from start > > > up, and will it increase start up time ? > > > > > Read this. > > Ah. I commented on the review comment directly. It should not slow down > start up time, Please prove this, and measure it, and attach your result here. > > I can add a bug to make that a customizable preference (pre-cache or not), At this moment I don't think it makes any sense I didn't see any result here. How do we evaluate the tradeoff? > It's not actually that slow, SIM IO, is slow.

(In reply to Yoshi Huang[:allstars.chh] from comment #129) > (In reply to Garner Lee from comment #127) > > (In reply to Yoshi Huang[:allstars.chh] from comment #126) > > > (In reply to Yoshi Huang[:allstars.chh] from comment #118) To be more specific I mean boot time, as the code runs in main thread of parent process.

This archive has some logs with a modified design, and some carefully placed adhoc timers (not with profile.sh). It also includes WebCrypto support mixed in. This is from a SE app "sim-access-test", with a matching SE application. All times are in milliseconds, retrieved by Date.now(), and times can vary depending on test run. The SIM IO execution times "pre-cache" access rule log below is probably not the actual total execution time during bootup (listening to "final-ui-startup" observer event for pre-caching). The [Echanged APDU [nnnn]] are the interesting parts. The Pre-cache version, with other bootup events in the results: I/Gecko ( 193): ZZZ ACEService pre-cache I/Gecko ( 193): ZZZ GPA [Start of readAccessRules Promise Tasks] ZZZ Delta: 0, time since start: 0 I/Gecko ( 193): ZZZ ACEService TIME Elapsed end: 16 I/Gecko ( 193): ZZZ GPA [Open Channel [5115]] ZZZ Delta: 5115, time since start: 5115 I/Gecko ( 193): ZZZ GPA [Echanged APDU [5159]] ZZZ Delta: 44, time since start: 44 I/Gecko ( 193): ZZZ GPA [Echanged APDU [5188]] ZZZ Delta: 29, time since start: 29 I/Gecko ( 193): ZZZ GPA [Echanged APDU [5249]] ZZZ Delta: 61, time since start: 61 I/Gecko ( 193): ZZZ GPA [Echanged APDU [5852]] ZZZ Delta: 603, time since start: 604 I/Gecko ( 193): ZZZ GPA [Echanged APDU [8457]] ZZZ Delta: 2605, time since start: 2605 I/Gecko ( 193): ZZZ GPA [Echanged APDU [8614]] ZZZ Delta: 157, time since start: 157 I/Gecko ( 193): -*- GPAccessRulesManager _readAccessRules: refresh tag saved: 1,2,3,4,5,6,7,8 I/Gecko ( 193): ZZZ GPA [Echanged APDU [8703]] ZZZ Delta: 89, time since start: 89 I/Gecko ( 193): ZZZ GPA [Echanged APDU [8781]] ZZZ Delta: 78, time since start: 78 I/Gecko ( 193): ZZZ GPA [Echanged APDU [9002]] ZZZ Delta: 221, time since start: 221 I/Gecko ( 193): ZZZ GPA [Echanged APDU [9037]] ZZZ Delta: 35, time since start: 35 I/Gecko ( 193): ZZZ GPA [Rules Parsed.] ZZZ Delta: 0, time since start: 0 I/Gecko ( 193): ZZZ GPA [Closed Channel [9075]] ZZZ Delta: 38, time since start: 38 I/Gecko ( 193): ZZZ GPA [Finished all Promise Tasks] ZZZ Delta: 9102, time since start: 9102 The "no-precache" of the access rules version that fetches the rules on first SE access completes rule reading in under 700ms (cache on app launch only): I/Gecko ( 190): ZZZ ACEService [Manifest] ZZZ Delta: 0, time since start: 0 I/Gecko ( 190): ZZZ ACEService [RulesManager] ZZZ Delta: 0, time since start: 0 I/Gecko ( 190): ZZZ ACEService [In isAccessAllowed Promise Chain] ZZZ Delta: 1, time since start: 1 I/Gecko ( 190): ZZZ ACEService [Public Key] ZZZ Delta: 2, time since start: 3 I/Gecko ( 190): ZZZ ACEService [Dev Cert] ZZZ Delta: 3, time since start: 6 I/Gecko ( 190): ZZZ ACEService [Enter Manifest get.] ZZZ Delta: 1, time since start: 7 I/Gecko ( 190): ZZZ ACEService [Sig Valid] ZZZ Delta: 5, time since start: 12 I/Gecko ( 190): ZZZ ACEService [Got Cert Hash] ZZZ Delta: 2, time since start: 14 I/Gecko ( 190): ZZZ GPA [Start of readAccessRules Promise Tasks] ZZZ Delta: 0, time since start: 0 I/Gecko ( 190): ZZZ GPA [Open Channel [177]] ZZZ Delta: 177, time since start: 177 I/Gecko ( 190): ZZZ GPA [Echanged APDU [212]] ZZZ Delta: 35, time since start: 35 I/Gecko ( 190): ZZZ GPA [Echanged APDU [234]] ZZZ Delta: 22, time since start: 22 I/Gecko ( 190): ZZZ GPA [Echanged APDU [316]] ZZZ Delta: 82, time since start: 82 I/Gecko ( 190): ZZZ GPA [Echanged APDU [348]] ZZZ Delta: 32, time since start: 33 I/Gecko ( 190): ZZZ GPA [Echanged APDU [390]] ZZZ Delta: 42, time since start: 42 I/Gecko ( 190): ZZZ GPA [Echanged APDU [427]] ZZZ Delta: 37, time since start: 37 I/Gecko ( 190): -*- GPAccessRulesManager _readAccessRules: refresh tag saved: 1,2,3,4,5,6,7,8 I/Gecko ( 190): ZZZ GPA [Echanged APDU [501]] ZZZ Delta: 74, time since start: 74 I/Gecko ( 190): ZZZ GPA [Echanged APDU [548]] ZZZ Delta: 47, time since start: 47 I/Gecko ( 190): ZZZ GPA [Echanged APDU [596]] ZZZ Delta: 48, time since start: 49 I/Gecko ( 190): ZZZ GPA [Echanged APDU [627]] ZZZ Delta: 31, time since start: 31 I/Gecko ( 190): ZZZ GPA [Rules Parsed.] ZZZ Delta: 1, time since start: 1 I/Gecko ( 190): ZZZ GPA [Closed Channel [650]] ZZZ Delta: 23, time since start: 23 I/Gecko ( 190): ZZZ GPA [Finished all Promise Tasks] ZZZ Delta: 679, time since start: 679 Later, the cached rules pay about ~300ms to read the refresh tag, then goes about the rest of the read of the access rules. This is the time the first SE app would save on the first SE access. (679 - 336 = 343ms). This is not a huge amount by itself, but there are other associated components an app might use that could also be slow (NFC) that the user may notice. [Skip Rule reading.] ZZZ Delta: 336, time since start: 336 Also in the logs: WebCrypto is apparently not really major bottleneck currently: I/Gecko ( 190): ZZZ ACEService [In isAccessAllowed Promise Chain] ZZZ Delta: 1, time since start: 1 I/Gecko ( 190): ZZZ ACEService [Public Key] ZZZ Delta: 1, time since start: 2 I/Gecko ( 190): ZZZ ACEService [Dev Cert] ZZZ Delta: 1, time since start: 3 I/Gecko ( 190): ZZZ ACEService [Enter Manifest get.] ZZZ Delta: 2, time since start: 5 I/Gecko ( 190): ZZZ ACEService [Sig Valid] ZZZ Delta: 4, time since start: 9 I/Gecko ( 190): ZZZ ACEService [Got Cert Hash] ZZZ Delta: 3, time since start: 12 Some options: Shift pre-cache code to wait for final-ui-startup (boot time addition is < 1 second at the tail end of the bootup based on the 2nd log), or simply remove the pre-cache code. First SE transaction is be slower if removed, but still works.

^^ Wwebapp "sim-access-test"

Can you remove the precache first and discuss this in another bug? Landing ACE without hitting performace should be more priority than tuning the boot up/start up time. I really don't like a bug with more than 100 comments.

I have removed the pre-cache as it isn't essential. A separate bug (like Bug 1158024) can investigate/handle more detailed user centered performance tweaks.

Comment on attachment 8591174 [details] [diff] [review] Part 2.2: New SEUtils.js tests Review of attachment 8591174 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/tests/unit/header_helper.js @@ +74,5 @@ > + TAG_GPD_AID, > + TAG_EXTERNALDO, > + TAG_INDIRECT, > + TAG_EF_ODF > +]; I don't know why you like to type this twice. Cannot you move this to another js and import it?

Add r=allstars.chh

Move GP constants to a separate shared gp_consts.js.

Build support update. Added gp_consts.js along side se_consts.js

r=allstars.chh

Thanks for the review comments. I've updated tests to use a unified gp_consts.js.

Fix refreshTag ACE test regression.

Comment on attachment 8603088 [details] [diff] [review] Part 2.2: New SEUtils.js tests Review of attachment 8603088 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/secureelement/tests/unit/test_SEUtils.js @@ +7,5 @@ > /* exported run_test */ > > Components.utils.import("resource://gre/modules/SEUtils.jsm"); > +let GP = {}; > +Components.utils.import("resource://gre/modules/gp_consts.js", GP); where is it?