psiddh Reporter
	Description • 11 years ago

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

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 1 • 11 years ago

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

	Ming Yin
	Comment 2 • 11 years ago

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.

	Ming Yin
	Comment 3 • 11 years ago

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.

	Anders Rundgren
	Comment 4 • 11 years ago

Needs complete overhaul.  See:

https://bugzilla.mozilla.org/show_bug.cgi?id=879861

	Sandip Kamat
	Comment 5 • 11 years ago

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.

	Kamil Leszczuk [:kamituel]
	Comment 6 • 10 years ago

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?

	Ken Chang[:kenkai]
	Comment 7 • 10 years ago

(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

	Ming Yin
	Comment 8 • 10 years ago

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.

	Kamil Leszczuk [:kamituel]
	Comment 9 • 10 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 10 • 10 years ago

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

	Anders Rundgren
	Comment 11 • 10 years ago

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

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 12 • 10 years ago

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

	Kamil Leszczuk [:kamituel]
	Comment 13 • 10 years ago

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.

	Kamil Leszczuk [:kamituel]
	Comment 14 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 15 • 10 years ago

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?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 16 • 10 years ago

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.

	Kamil Leszczuk [:kamituel]
	Comment 17 • 10 years ago

> 
> 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!

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 18 • 10 years ago

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

	Kai-Chih Hu [:khu]
	Comment 19 • 10 years ago

Can we update the target milestone and ETA? Thanks!

	Kamil Leszczuk [:kamituel]
	Comment 20 • 10 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 21 • 10 years ago

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.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 22 • 10 years ago

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

	Krzysztof Mioduszewski[:tauzen]
	Comment 23 • 10 years ago

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.

	Krzysztof Mioduszewski[:tauzen]
	Comment 24 • 10 years ago

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.

	Krzysztof Mioduszewski[:tauzen]
	Comment 26 • 10 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 27 • 10 years ago

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?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 28 • 10 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 29 • 10 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 30 • 10 years ago

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?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 31 • 10 years ago

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.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 32 • 10 years ago

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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 33 • 10 years ago

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?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 34 • 10 years ago

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.

	Krzysztof Mioduszewski[:tauzen]
	Comment 35 • 10 years ago

(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

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 36 • 10 years ago

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.

	Vincent Chang[:vchang][changyihsin]
	Comment 37 • 10 years ago

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

	Kamil Leszczuk [:kamituel]
	Comment 38 • 10 years ago

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

	Kamil Leszczuk [:kamituel]
	Comment 39 • 10 years ago

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

	Kamil Leszczuk [:kamituel]
	Comment 40 • 10 years ago

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.

	Dimi Lee [:dimi]
	Comment 41 • 10 years ago

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|

	Dimi Lee [:dimi]
	Comment 42 • 10 years ago

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

	Dimi Lee [:dimi]
	Comment 43 • 10 years ago

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

	Dimi Lee [:dimi]
	Comment 44 • 10 years ago

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

	Dimi Lee [:dimi]
	Comment 45 • 10 years ago

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.

	Ming Yin
	Comment 47 • 10 years ago

(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

	Dimi Lee [:dimi]
	Comment 48 • 10 years ago

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

	Kamil Leszczuk [:kamituel]
	Comment 49 • 10 years ago

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.

	Kamil Leszczuk [:kamituel]
	Comment 50 • 10 years ago

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!

	Kamil Leszczuk [:kamituel]
	Comment 51 • 10 years ago

The GPAccessRulesManager. See comment above.

	Dimi Lee [:dimi]
	Comment 52 • 10 years ago

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.

	Vincent Chang[:vchang][changyihsin]
	Comment 53 • 10 years ago

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?

	Vincent Chang[:vchang][changyihsin]
	Comment 54 • 10 years ago

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.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 55 • 10 years ago

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

	Garner Lee Assignee
	Comment 56 • 10 years ago

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.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 57 • 10 years ago

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.

	Vincent Chang[:vchang][changyihsin]
	Comment 58 • 10 years ago

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?

	Vincent Chang[:vchang][changyihsin]
	Comment 59 • 10 years ago

Garner is working on this.

	Krzysztof Mioduszewski[:tauzen]
	Comment 60 • 10 years ago

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

	Garner Lee Assignee
	Comment 65 • 10 years ago

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

	Garner Lee Assignee
	Comment 66 • 10 years ago

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

	Garner Lee Assignee
	Comment 67 • 10 years ago

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

	Dimi Lee [:dimi]
	Comment 68 • 10 years ago

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 ?

	Dimi Lee [:dimi]
	Comment 69 • 10 years ago

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

	Dimi Lee [:dimi]
	Comment 70 • 10 years ago

Comment on attachment 8570215 [details] [diff] [review]
Part 2 - GPAccessRulesManager and nsIAccessRulesManager.idl

f- because i would like to see comment addressed or explained

	Dimi Lee [:dimi]
	Comment 71 • 10 years ago

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

	Garner Lee Assignee
	Comment 72 • 10 years ago

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

Fix bad description.

	Sandip Kamat
	Comment 73 • 10 years ago

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

	Garner Lee Assignee
	Comment 74 • 10 years ago

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

	Dimi Lee [:dimi]
	Comment 80 • 10 years ago

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

	Dimi Lee [:dimi]
	Comment 81 • 10 years ago

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

	Dimi Lee [:dimi]
	Comment 82 • 10 years ago

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 ?

	Garner Lee Assignee
	Comment 83 • 10 years ago

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

	Garner Lee Assignee
	Comment 84 • 10 years ago

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

	Garner Lee Assignee
	Comment 85 • 10 years ago

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

	Garner Lee Assignee
	Comment 87 • 10 years ago

Alignment nit fixed.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 88 • 10 years ago

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?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 89 • 10 years ago

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?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 90 • 10 years ago

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

	Garner Lee Assignee
	Comment 91 • 10 years ago

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

	Garner Lee Assignee
	Comment 92 • 10 years ago

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

	psiddh Reporter
	Comment 93 • 10 years ago

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)

	Garner Lee Assignee
	Comment 94 • 10 years ago

Fixed review comments.

	Garner Lee Assignee
	Comment 95 • 10 years ago

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.

	Garner Lee Assignee
	Comment 96 • 10 years ago

Move connector code outside Promise.