User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release) Build ID: 20140212131424

The MozSecureElementManager.webidl atBug 879861 needs to be extended to allow the certified web applications to listen to the HCI event EVT_TRANSACTION. This features is useful for NFC payment scenario,

As requested by Ming, assign to Sidd.

Some design issues: ............................................................................. Issue-1: How to subscribe to certain transaction events(EVT_TRANSACTION) ............................................................................. According to GSMA NFC Handset APIs Requirement Specification v4.1(http://www.gsma.com/digitalcommerce/wp-content/uploads/2013/12/TS26v4-1.pdf), the certified web application will construct following transaction event in order to listen to specific "EVT_TRANSACTION". event.action: com.gsma.services.nfc.action.TRANSACTION_EVENT event.uri: nfc://secure:0/<SEName>/<AID> SEName reflects the originating SE. It should be compliant with SIMAlliance Open Mobile APIs. AID reflects the originating UICC applet identifier. When AID is omitted from the URI, the application is registered to any “EVT_TRANSACTION” events sent from the specified Secure Element. For UICC, SEName must follow the format hereafter: “SIM” [smartcard slot] (e.g. SIM1, SIM2, … SIMn). .............................................................. Issue-2: The data structure of the incoming transaction events .............................................................. On reception of EVT_TRANSACTION, the SE Manager will notfiy the web application about the transaction event. The payload of the event data shall follow the definition in ETSI TS 102.622, which is made up of a list of parameters coded in BER-TLV fields. The tag and length of these fields are defined in following table. Description | Tag | Length -------------------------------- AID | '81' | 5 to 16 -------------------------------- Parameters | '82' | 0 to 255 -------------------------------- .............................................................. Issue-3: Securing transaction events .............................................................. 1)Since “EVT_TRANSACTION” messages are sensitive data, intercepting these events might help a malicious application to lure a user into entering sensitive information into a fake UI. Usage of “EVT_TRANSACTION” events must request certain permissions. All of them must be set to “dangerous” protection level. 2)Transaction events link an web application and an applet installed on a Secure Element. For this reason, securing them shall be done with the same rules that restrict applet access by the Web application through the SE Access WebAPI. If an application is registered to any “EVT_TRANSACTION”, by omitting the AID in the Intent, it will receive the events of any applets to those accessible using the “Access Control Function”.

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.

Move event callback to mozNFC. While SE triggered, like any other user transactions (ex: SMS, email, a phone call), NFC is simply another delivery path for that transaction event notification.

In last NFC workweek, there's a larger discusson on how to register the eSE or sim card AIDs so that the right web application (or application category at least) is notified. Assuming a non-rooted NFC enabled device, this can either be in the manifest (so it can be verified), dynamically at runtime, "route.xml", and/or also by a vetted whitelist on the secure element application itself somewhere. One obvious example is if the user has more than one wallet they want to use (perhaps depending on context: business trip wallet, every day transaction wallet, restricted vacation only wallet, etc). The user story may influence the API/security design.

(In reply to Garner Lee from comment #8) > One obvious example is if the user has more than one wallet they want to use > (perhaps depending on context: business trip wallet, every day transaction > wallet, restricted vacation only wallet, etc). The user story may influence > the API/security design. Good point! Security needs to be considered for this event handling. Since “EVT_TRANSACTION” messages are sensitive transaction data, Intercepting these events might help a malicious application to lure a user into entering sensitive information into a fake UI. The NFC stack shall therefore implement GlobalPlatform Secure Element Access Control standard to check that the recipient app has been signed with an authorised certificate. This check is performed at the time the event is being forwarded from the lower layers to the target application. If no application is authorised as per “Access Control” check, then the event is discarded.

This patch file is a WIP that is still using events to send HCI EVENT TRANSACTION to registered applications. nfc_worker.js is the entry point of the new nfcd message.

WIP update. Rewrite and rebase to head (still events).

Handle event clearing in child process shutdown.

Hi Garner, Can you set the target milestone to either sprint2 or sprint3?

Hi Garner, I am going to show you how to implement the filter: 1) Add your own system message, say 'hci-transaction', to SystemMessagePermissionChecker.jsm [1] The system message could bind a permission if you want. 2) Create an XPCOM component which implements nsISystemMessagesConfigurator [2] 3) Name the contract id of the component you create '@mozilla.org/dom/system-messages/configurator/hci-transaction;1' Note that the contract id MUST match the system message name because the system message internal would look up the contract id based on the system message name. 4) Finally, implement the function "shouldDispatch". Promise.defer is a approach to translate an asynchronous operation into a promise. It typically looks like this: let deferred = Promise.defer(); someAsyncFunc(function callback() { deferred.resolve(true); // returns true to indicate 'should dispatch' }); return deferred.promise; 5) The content of manifest could be fetched by [3]. Please feel free to ask me if you need further help or information. Thanks! [1] http://hg.mozilla.org/mozilla-central/file/83ced53b4298/dom/messages/SystemMessagePermissionsChecker.jsm#l29 [2] http://hg.mozilla.org/mozilla-central/file/83ced53b4298/dom/messages/interfaces/nsISystemMessagesInternal.idl#l60 [3] http://hg.mozilla.org/mozilla-central/file/83ced53b4298/dom/interfaces/apps/nsIAppsService.idl#l29

New WIP using system messages for the benifit of demo apps. It's not yet pulling in the manifest and/or other apps for inspection from the previous comment from Henry (Thanks!) since there's no data/topic filter defined yet.

(In reply to Garner Lee from comment #17) > Created attachment 8476123 [details] [diff] [review] > v3 - 0001-Bug-979767-Supporting-web-application-triggering-bas.patch > > New WIP using system messages for the benifit of demo apps. It's not yet > pulling in the manifest and/or other apps for inspection from the previous > comment from Henry (Thanks!) since there's no data/topic filter defined yet. The DOM part for HCI event has been removed in this version, is this intensional?

(In reply to Yoshi Huang[:allstars.chh] from comment #18) > (In reply to Garner Lee from comment #17) > > Created attachment 8476123 [details] [diff] [review] > > v3 - 0001-Bug-979767-Supporting-web-application-triggering-bas.patch > > > > New WIP using system messages for the benifit of demo apps. It's not yet > > pulling in the manifest and/or other apps for inspection from the previous > > comment from Henry (Thanks!) since there's no data/topic filter defined yet. > > The DOM part for HCI event has been removed in this version, is this > intensional? sorry, intentional

DOM Events were just a temporary solution, until system message filter has landed. This uses system message 'nfc-hci-event-transaction' to deliver hci-evt-transaction to the web app. This is the target solution since it will always show the app/notification to the user. You can use the demo app to test this https://github.com/tauzen/HCI-Event-Demo.

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #20) > DOM Events were just a temporary solution, Please have DOM peers agree on this first. The purpose of system message is to launch the app, just don't abuse it unless you're pretty sure what you're doing.

Fabrice, care to comment on #21? > Please have DOM peers agree on this first. > The purpose of system message is to launch the app, just don't abuse it > unless you're pretty sure what you're doing. Right now, there's no direct DOM interaction anymore, just a system message to launch and deliver data. One iteration of the code had previously set a special NFC only callback to know when to fire to the dynamically set event handler. I'm not sure what the preference is, though I believe the conclusion was "choose the solution that best fits FirefoxOS architecture first, then move it towards whatever standard emerges."

Linking the discussion in webapi here. https://groups.google.com/forum/#!topic/mozilla.dev.webapi/oER2OLg40pg

So, first I'm not a DOM peer ;) System messages have a dual role actually: delivering a message, and eventually waking up the app if it's not running. I remember the "NFC only callback" setup being very very strange, so I'm in favor of going with system messages for now if they fit the use case.

Hi smaug, DOM question from Comment #21, can/should we use system messages only? Or should our hardware/bottom up transaction message forward to an event handler as well? (I'm currently merging both solutions in the mean time...).

I'm not familiar with system messages.

But what is wrong with events?

(In reply to Olli Pettay [:smaug] from comment #27) > But what is wrong with events? Events don't launch applications.

(In reply to Garner Lee from comment #28) > (In reply to Olli Pettay [:smaug] from comment #27) > > But what is wrong with events? > > Events don't launch applications. I guess smaug's question is "After the app is launched by System message, why don't we pass DOM Event callback to the app?" My concern on this (not using DOM callback) is: For the following-up NFC API, CardEmulation API, Host Card Emulation API, they both share the same pattern of this, the Event Target app may not be launched yet. We need to dispatch a system message to wake it up. But if we don't pass DOM event to this app, only using system-message, maybe we will have some design problem in the future, for example, in the HCE case, the app should receive a "on-process-apdu" system message (See Android in [1]), the content of the system message should be looked like SECommand defined in Bug 879861, but the symbol is not available on Parent side, we will need a different dispatch strategy and possibly influence what this bug is doing now. Also the question to DOM peer and WebAPI reviewer is: What's the distinction between a System-message and a DOM callback? Should we use it separately? Or should we use it both at the same time? For the case that when the event happens, the event target(the app) is not launched yet. If we use System-message alone, I am not sure if that benefits the Web Platform, or it helps Web-Developer. For the Web Platform part, this means the event callback will pass by System-Message, and System message is only available on FirefoxOS(I know currently ServiceWorker is under going now). For the Web-developer, from the WebIDL (like in this bug), he may not know the event is passed through System-message, even his webapp is always running, he may spend a lot time digging through MDN or code to find out. Also developer maybe confused by when to use DOM callback, and when to use System-message now. Garner's team think they can get the data from the System-message, why will they listen to DOM callback? I think this is a good question for the DOM, as this question is also existing on our FirefoxOS now. Many Gaia apps will only listen to the System-message, they don't listen to the DOM calback. That means many efforts done in the DOM callback are not greatly appreciated. Although the System message could work for their product, I do think it's our (Mozilla) job to think if this could work on our goal to build a Web Platform. And I'd like to raise my concern that whether only system message is enough here, as this will greatly influence the design of NFC API in the future. And if we change the strategy in the future, at that time it may have impact on our partners (what Garner's team is doing right now). [1]: http://developer.android.com/reference/android/nfc/cardemulation/HostApduService.html#processCommandApdu(byte[],%20android.os.Bundle)

For system messages you need to ask someone who designed that API. Like sicking or fabrice, I think.

Hi Yoshi, to your question on why no events, in the interest of providing both possiblities for consideration earlier before branch, I've rebased HCI Event Transactions with both events and messages firing. Known issues: - Uint8Array wasn't codegening for some reason in a non-customEvent "HCIEventTransactionEvent", so I left the new Event out. - We might want to convert some Uint8Arrays (the data comes unmodified from nfcd), to a hex string in events as well, which might be mildly more programmer friendly depending on how it's used with Secure Elements. - Based on another NFC doc (I need to find it again...) this implementation blind fires events when the message is fired, and when the event hander is dynamically registered at runtime (apps can get one message and one event later). Without more process tracking (which could potentially be possible), it can't deliver just once at the moment. - The to be implemented "Access Control Enforcer" is marked in a few potential locations to filter and restrict access. - nfc (write) AND nfc-hci-events are needed in the manifest. This might be too restrictive. The callback currently requires the NFC navigator DOM to exist. - Works on Flame devices to HID reader/Terminal Simulator. I'm not obsoleting the system message only version yet.

^^ My ideal case is somehow direct an event to a process(es) that might not be runing, like a generic system message, but for navigator object DOM events. Perhaps that the event is merely piggy-backing on the system message for delivery (the message can currently do filtering too). > And I'd like to raise my concern that whether only system message is enough > here, as this will greatly influence the design of NFC API in the future. > And if we change the strategy in the future, at that time it may have impact > on our partners (what Garner's team is doing right now). > > [1]: > http://developer.android.com/reference/android/nfc/cardemulation/ > HostApduService.html#processCommandApdu(byte[],%20android.os.Bundle)

Comment on attachment 8477935 [details] [diff] [review] 0001-Bug-979767-Add-NFC-HCI-Event-Transaction-notification_events_and_messages.patch Review of attachment 8477935 [details] [diff] [review]: ----------------------------------------------------------------- If DOM peer and Fabrice don't think a DOM callback is mandatory, then we can remove DOM callback for now. Next time when you change your WIP dramatically, please also add detail comment on it as others will find out after three versions of WIP, then the fourth one removes event and the reason I found it is "The DOM part of first three versions are for testing" Please split your patches into : - System message, - NFC part. But if you still like to land DOM event part, please also split it to - NFC WebIDL and DOM part. Because WebIDL/DOM needs a DOM peer to review. And if you update the binary protocol with nfcd(NfcMessageHandler), nfcd part needs to be updated as well, but if you don't have too much time, please file another bug and make this bug depends on it, Dimi and I will fix it on Monday. ::: dom/apps/src/PermissionsTable.jsm @@ +340,5 @@ > certified: ALLOW_ACTION > }, > + "nfc-hci-events": { > + app: DENY_ACTION, > + privileged: DENY_ACTION, Not privileged? ::: dom/nfc/NfcContentHelper.js @@ +56,5 @@ > "NFC:CloseResponse", > "NFC:CheckP2PRegistrationResponse", > "NFC:PeerEvent", > "NFC:NotifySendFileStatusResponse", > + "NFC:DOMEvent", Make a more clear name ::: dom/nfc/gonk/Nfc.js @@ +632,5 @@ > return null; > } > > + // Update the current sessionId before sending to the worker > + message.json.sessionId = this._currentSessionId; sessionId is already set few lines above. ::: dom/nfc/gonk/NfcMessageHandler.cpp @@ +33,5 @@ > static const char* kInitializedNotification = "InitializedNotification"; > static const char* kTechDiscoveredNotification = "TechDiscoveredNotification"; > static const char* kTechLostNotification = "TechLostNotification"; > +static const char* kHCIEventTransactionNotification = > + "HCIEventTransactionNotification"; shorter name is okay. @@ +305,5 @@ > bool > +NfcMessageHandler::HCIEventTransactionNotification(const Parcel& aParcel, EventOptions& aOptions) > +{ > + aOptions.mType = NS_ConvertUTF8toUTF16(kHCIEventTransactionNotification); > + aOptions.mSessionId = -1; no need sesson Id. ::: dom/nfc/nsINfcContentHelper.idl @@ +36,5 @@ > + * Uint8Array payload > + * Uint8Array aidOrigin > + * } > + */ > + void notifyHCIEventTransaction(in nsIVariant data); If you plan to do the DOM part, update the UUID and change nsINfcPeerEventListener to nsINfcEventListener. @@ +157,5 @@ > + * > + * @param appId > + * Application ID to be registered > + */ > + void registerTargetForHCIEventTransaction(in nsIDOMWindow window, And make the original (un)registerTargetForPeerReady more generic, like registerTarget(window, appId, event); But it seems AID is missing here? HCI_EVT_Transaction should be fired to target with matching AID, right ? ::: dom/webidl/MozNFC.webidl @@ +64,5 @@ > [CheckPermissions="nfc-write"] > attribute EventHandler onpeerready; > [CheckPermissions="nfc-write"] > attribute EventHandler onpeerlost; > + [CheckPermissions="nfc-write"] nfc-hci-event? Also add nfc-hci-event permission to MozNFC. @@ +65,5 @@ > attribute EventHandler onpeerready; > [CheckPermissions="nfc-write"] > attribute EventHandler onpeerlost; > + [CheckPermissions="nfc-write"] > + attribute EventHandler onhcieventtransaction; The parameters in this event should be documented.

(In reply to Yoshi Huang[:allstars.chh] from comment #33) > And if you update the binary protocol with nfcd(NfcMessageHandler), nfcd > part needs to be updated as well, Dimi updated the nfcd in Bug 1057960, please use the PR there for nfcd. The gecko and nfcd should be landed together, otherwise there will be some warning shown in logcat.

(In reply to Yoshi Huang[:allstars.chh] from comment #34) > (In reply to Yoshi Huang[:allstars.chh] from comment #33) > > And if you update the binary protocol with nfcd(NfcMessageHandler), nfcd > > part needs to be updated as well, Thanks. Updated locally to match the patch version.

> @@ +157,5 @@ > > + * > > + * @param appId > > + * Application ID to be registered > > + */ > > + void registerTargetForHCIEventTransaction(in nsIDOMWindow window, > > And make the original (un)registerTargetForPeerReady more generic, like > registerTarget(window, appId, event); > Ok (if doing events). > But it seems AID is missing here? > HCI_EVT_Transaction should be fired to target with matching AID, right ? > There's more than one way. I was anticipating the application declares in the signed webapp.manifest what it will listen to "secure_element" section, though that's not really implemented either for the SE Access Control Enforcer. Taking a parameter means it might want an array of AIDs on the secure element. If there's no security to be gained, I can go with either, and just have ACE or similar code verify again.

Hi Garner, I have talked to Yoshi offline, and both of us agree that we can remove DOM callback for now based on comment 24 and comment 33. Please split your patches into system message and NFC parts and ask Fabrice and Yoshi for review.

(In reply to Vincent Chang[:vchang] from comment #37) > Hi Garner, I have talked to Yoshi offline, and both of us agree that we can > remove DOM callback for now based on comment 24 and comment 33. Done. I'll update when I submit them to the try server.

Hold on. I may have messed up the patch generation after rebase.

Comment on attachment 8478813 [details] [diff] [review] (v4) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch Review of attachment 8478813 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/gonk/Nfc.js @@ +462,5 @@ > this._currentSessionId = null; > > break; > + case "HCIEventTransactionNotification": > + gMessageManager.notifyHCIEventTransaction(message); gMessageManager is not used to do broadcast. do the broadcast here. And you didn't wrap the message? How does WebApp will understand Origin and OriginIndex? Shouldn't you wrap up to "sim1"? ::: dom/nfc/gonk/NfcGonkMessage.h @@ +30,5 @@ > enum NfcNotification { > Initialized = 2000, > TechDiscovered, > TechLost, > + HCIEventTransaction, Please update NFCD_MINOR_VERSION as well. ::: dom/nfc/gonk/NfcOptions.h @@ +93,5 @@ > : mType(EmptyString()), mStatus(-1), mSessionId(-1), mRequestId(EmptyString()), mMajorVersion(-1), mMinorVersion(-1), > mIsReadOnly(-1), mCanBeMadeReadOnly(-1), mMaxSupportedLength(-1), mPowerLevel(-1) > + { > + mHciEventTransaction.mAidOrigin = 0; > + mHciEventTransaction.mAidOriginIndex = 0; initial default value wisely. ::: dom/nfc/gonk/NfcService.cpp @@ +162,5 @@ > COPY_OPT_FIELD(mCanBeMadeReadOnly, -1) > COPY_OPT_FIELD(mMaxSupportedLength, -1) > > + COPY_OPT_FIELD(mHciEventTransaction.mAidOrigin, 0); > + COPY_OPT_FIELD(mHciEventTransaction.mAidOriginIndex, 1); Now every event from nfcd will have this property. ::: dom/webidl/MozNFC.webidl @@ +82,5 @@ > + * } > + * } > + */ > + [CheckPermissions="nfc-hci-events"] > + attribute EventHandler onhcieventtransaction; Remove this ::: dom/webidl/NfcOptions.webidl @@ +11,5 @@ > }; > > +dictionary HCIEventTransaction > +{ > + unsigned long aidOrigin; origin. The origin means the source of this HCIEventTransaction, not the source of the 'aid'. @@ +12,5 @@ > > +dictionary HCIEventTransaction > +{ > + unsigned long aidOrigin; > + unsigned long aidOriginIndex; ditto

Comment on attachment 8478812 [details] [diff] [review] (v4) Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch Review of attachment 8478812 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/messages/HCIEventTransactionSystemMessageConfigurator.js @@ +37,5 @@ > + if(!evtTransaction) { > + return false; > + } > + let aid = this._byteAIDToHex(evtTransaction.aid); > + let seName = 'SIM0'; // Bug 1043276: Get the SE name from nfcd. NFC Daemon already passes the SE name to gecko.

See the origin passed from nfcd https://github.com/mozilla-b2g/platform_system_nfcd/commit/f309e8373d338e7a83ac04a034c5910f912d95ff#diff-facc6f99589b3adeffaebc49d9b3a2ccR385

This is the latest version of Garners code with minor fixes from me.

Same as above, this the latest version of Garners code with small fixes from me.

Comment on attachment 8478987 [details] [diff] [review] (v5) Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch Review of attachment 8478987 [details] [diff] [review]: ----------------------------------------------------------------- cancelling r? for the dict can be more simpler. Also some handling should be done in NfcService layer as we try not to expose nfcd-gonk specific things to high level. (I am also planing moving nfcd error code in NfcMessageHandler) ::: dom/nfc/gonk/Nfc.js @@ +486,5 @@ > + let evtTxn = message.hciEventTransaction; > + let seNameBase = NFC.NFC_SECURE_ELEMENT_NAMES[evtTxn.originType]; > + > + let msg = { > + seName: seNameBase + evtTxn.originIndex, I'll suggest using 'origin', don't use 'seName' 1. In the beginning we try to differetiate SE API and NFC API, so we try not to put 'SE' in this event/callback to make it more clear this event is from NFC. 2. For impl you can broadcast the message directly without any renaming. ::: dom/nfc/gonk/NfcOptions.h @@ +108,5 @@ > int32_t mIsReadOnly; > int32_t mCanBeMadeReadOnly; > int32_t mMaxSupportedLength; > int32_t mPowerLevel; > + HCIEventTransactionStruct mHciEventTransaction; Don't need another level abstraction. move 'origin', 'aid' and 'payload' into EventOptions ::: dom/nfc/gonk/nfc_consts.js @@ +118,5 @@ > +this.NFC_SECURE_ELEMENT_NAMES = {}; > +this.NFC_SECURE_ELEMENT_NAMES[NFC_SECURE_ELEMENT_SIM] = "SIM"; > +this.NFC_SECURE_ELEMENT_NAMES[NFC_SECURE_ELEMENT_ESE] = "ESE"; > +this.NFC_SECURE_ELEMENT_NAMES[NFC_SECURE_ELEMENT_ASSD] = "ASSD"; > +this.NFC_SECURE_ELEMENT_NAMES[NFC_SECURE_ELEMENT_UNKNOWN] = "UNKNOWN"; These should be defined in NfcGonkMessage.h ::: dom/webidl/NfcOptions.webidl @@ +12,5 @@ > > +dictionary HCIEventTransaction > +{ > + long originType; > + long originIndex; If we can process these two in NfcService, we can replace these two as DOMString, like DOMString origin @@ +51,5 @@ > long maxSupportedLength; > > long powerLevel; > + > + HCIEventTransaction hciEventTransaction; We could try to make this dict as flat as we can. For example, when event is hci-event-transaction, this dict could be looked like { type: "HCIEventTransaction", origin: "SIM1", aid: [..], payload: [..] } without introducing another level 'hciEventTransaction' So you should move members of hciEventTransaction here to make it more simpler

(In reply to Yoshi Huang[:allstars.chh] from comment #47) Also some handling should be done in NfcService layer as we try not to > expose nfcd-gonk specific things to high level. > (I am also planing moving nfcd error code in NfcMessageHandler) Can you clarify. Do you mean to have NfcMessageHandler as a pure(er) unmarshaller, and NfcService will process it more (which just takes/filters EventOptions right now)?

Patch rebased.

Rebaed. hciEvtTransaction is flattened, and no longer sends an empty version of itself on unrelated events. Question: Shoud I move creation of origin string "SIM1" in NfcService layer versus NfcMessageHandler? NfcGonkMessage.h isn't used by NfcService.cpp currently.

Comment on attachment 8479438 [details] [diff] [review] (v6) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch Review of attachment 8479438 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/gonk/Nfc.js @@ +483,5 @@ > + /** > + * FIXME: > + * GSMA 6.0 7.4 UI Application triggering requirements > + * specifices the need to send a URL, but doesn't spec it fully. > + */ Better document the parameters here. (I will file another bug for fixing tech-discovered) ::: dom/nfc/gonk/NfcGonkMessage.h @@ +75,5 @@ > FailEnableLowPowerMode = -30, > FailDisableLowPowerMode = -31, > }; > > +enum SecureElementTypeCode { SecureElementOrigin should be enough ::: dom/nfc/gonk/NfcMessageHandler.cpp @@ +314,5 @@ > + > + int32_t originType = aParcel.readInt32(); > + int32_t originIndex = aParcel.readInt32(); > + > + // Move processing to NfcService.cpp? yeah, moving NfcService is better. And you don't have to include NfcGonkMessage.h. Just list it by order like the NfcTechString in NfcService.cpp @@ +325,5 @@ > + break; > + case SecureElementTypeCode::ASSD: > + aOptions.mOrigin.Assign(SE_ASSD_STR); > + break; > + case SecureElementTypeCode::UNKNOWN: Don't have to do 'case UNKNOWN'. default is enough

Comment on attachment 8479434 [details] [diff] [review] (v6) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch Review of attachment 8479434 [details] [diff] [review]: ----------------------------------------------------------------- Nothing major, but I'd like to see these changes before landing: - Use consistently double quotes for strings in HCIEventTransactionSystemMessageConfigurator.js (it uses both ' and " currently). - Move HCIEventTransactionSystemMessageConfigurator.js and the manifest to dom/nfc. - Add unit tests for HCIEventTransactionSystemMessageConfigurator.js. That would help a lot to understand what the configurator does. ::: dom/apps/src/PermissionsTable.jsm @@ +345,5 @@ > certified: ALLOW_ACTION > }, > + "nfc-hci-events": { > + app: DENY_ACTION, > + privileged: ALLOW_ACTION, We'll need security people to sign off on the privileged level. ::: dom/messages/HCIEventTransactionSystemMessageConfigurator.js @@ +12,5 @@ > +XPCOMUtils.defineLazyServiceGetter(this, "appsService", > + "@mozilla.org/AppsService;1", > + "nsIAppsService"); > + > +let DEBUG = true; nit: switch to false before landing. @@ +32,5 @@ > + debug("mustShowRunningApp returning true"); > + return true; > + }, > + > + shouldDispatch: function shouldDispatch(manifestURL, pageURL, type, message, extra) { nit: here an in other function, use aParamName @@ +92,5 @@ > + return (matchingRule) ? Promise.resolve() : Promise.reject(); > + }, > + > + // FIXME: there is probably something which does this > + _byteAIDToHex: function _byteAIDToHex(arr) { which type is arr? Is is a typed array or a 'normal' array? ::: dom/messages/moz.build @@ +7,5 @@ > DIRS += ['interfaces'] > > EXTRA_COMPONENTS += [ > + 'HCIEventTransactionSystemMessage.manifest', > + 'HCIEventTransactionSystemMessageConfigurator.js', these should rather live in the dom/nfc directory

Thanks for the review. I should get to fixing the issues soon in Part 1. (In reply to Fabrice Desré [:fabrice] from comment #52) > Comment on attachment 8479434 [details] [diff] [review] > (v6) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch > - Add unit tests for HCIEventTransactionSystemMessageConfigurator.js. That > would help a lot to understand what the configurator does. For the moment, it makes more sense in context of in laying the groundwork for Bug 884594, Access Control Element/Enforcer to the secure element (v2.2).

Comment on attachment 8479434 [details] [diff] [review] (v6) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch Review of attachment 8479434 [details] [diff] [review]: ----------------------------------------------------------------- The minor issue I mentioned in yesterday's meeting. ::: dom/messages/HCIEventTransactionSystemMessageConfigurator.js @@ +36,5 @@ > + shouldDispatch: function shouldDispatch(manifestURL, pageURL, type, message, extra) { > + debug('message to dispatch: ' + JSON.stringify(message)); > + debug('manifest url: ' + manifestURL); > + if(!message) { > + return false; shouldDispatch needs to return a promise by design. [1] [1] http://hg.mozilla.org/mozilla-central/file/0753f7b93ab7/dom/messages/interfaces/nsISystemMessagesInternal.idl#l73

Fixed nits, chars are '', strings "", debug is off, Moved files. No unit tests yet. Please note if required (Marionette tests on emulator is pending Bug: 1034998) Sec review --> sec-approval? Seems kind of off. Paul?

Fixed nits, and comments. Secure Element "UNKNOWN" is treated as "don't send it" for now.

Comment on attachment 8479559 [details] [diff] [review] (v7) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch Review of attachment 8479559 [details] [diff] [review]: ----------------------------------------------------------------- some files are missing? where's NfcOptions.webidl?

Reshuffle patch files.

Move NfcOptions.webidl back.

Comment on attachment 8479579 [details] [diff] [review] (v7.1) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch Review of attachment 8479579 [details] [diff] [review]: ----------------------------------------------------------------- Forward r? to Smaug for the WebIDL change ::: dom/nfc/gonk/Nfc.js @@ +477,5 @@ > }, > > + // HCI Event Transaction > + notifyHCIEventTransaction: function notifyHCIEventTransaction(message) { > + debug("notifyHCIEventTransaction: " + JSON.stringify(message)); remove, as onEvent already did this. @@ +484,5 @@ > + * FIXME: > + * GSMA 6.0 7.4 UI Application triggering requirements > + * specifies the need for the following parameters to be derived and sent. > + * One unclear spec is what the URI format "secure:0" refers to, given > + * seName can be something like "SIM0" or "SIM1". like "SIM1", or "SIM2". @@ +491,5 @@ > + * 2) URI, of the format: nfc://secure:0/<SEName>/<AID> > + * - SEName reflects the originating SE. It must be compliant with > + * SIMAlliance Open Mobile APIs > + * - AID reflects the originating UICC applet identifier > + * 3) AID - Application ID on a Secure Element You already list AID in 2) URI. ::: dom/nfc/gonk/NfcGonkMessage.h @@ +79,5 @@ > +enum SecureElementOrigin { > + SIM = 0, > + ESE = 1, > + ASSD = 2, > + UNKNOWN = 99, remove UNKNOWN ::: dom/nfc/gonk/NfcService.cpp @@ +39,5 @@ > +static const nsLiteralString SEOriginString[] = { > + NS_LITERAL_STRING("SIM"), > + NS_LITERAL_STRING("ESE"), > + NS_LITERAL_STRING("ASSD"), > + NS_LITERAL_STRING("UNKNOWN") remove UNKNOWN @@ +177,5 @@ > + } else { > + // UNKNOWN is sent if not a transaction. > + mEvent.mOrigin.Assign(SEOriginString[size - 1]); > + } > + COPY_OPT_FIELD(mOrigin, SEOriginString[size - 1]); if ((mEvent.mOriginType != -1) { mEvent.mOrigin.Assign(SEOriginString[mEvent.mOriginType]); mEvent.mOrigin.AppendInt(mEvent.mOriginIndex, 16 /* radix */); } or maybe we do need to include NfcGonkMessage.h here. But now it should be okay, I'll file another bug to make sure the type won't overflow.

Comment on attachment 8479579 [details] [diff] [review] (v7.1) 0002-Bug-979767-Part-2-NFC-and-Gonk.-Supporting-web-appli.patch (reviewing .webidl only) >+NfcMessageHandler::HCIEventTransactionNotification(const Parcel& aParcel, EventOptions& aOptions) >+{ >+ aOptions.mType = NS_ConvertUTF8toUTF16(kHCIEventTransactionNotification); >+ >+ aOptions.mOriginType = aParcel.readInt32(); >+ aOptions.mOriginIndex = aParcel.readInt32(); >+ >+ int32_t aidLength = aParcel.readInt32(); >+ aOptions.mAid.AppendElements( >+ static_cast<const uint8_t*>(aParcel.readInplace(aidLength)), aidLength); ... but could you use 2 spaces for indentation here and elsewhere. >+ int size = sizeof(SEOriginString)/sizeof(nsLiteralString); space before and after /

(In reply to Garner Lee from comment #53) > Thanks for the review. I should get to fixing the issues soon in Part 1. > > (In reply to Fabrice Desré [:fabrice] from comment #52) > > Comment on attachment 8479434 [details] [diff] [review] > > (v6) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch > > - Add unit tests for HCIEventTransactionSystemMessageConfigurator.js. That > > would help a lot to understand what the configurator does. > > For the moment, it makes more sense in context of in laying the groundwork > for Bug 884594, Access Control Element/Enforcer to the secure element (v2.2). I don't understand what you mean there.

Comment on attachment 8479578 [details] [diff] [review] (v7.1) 0001-Bug-979767-Part-1-Messages-and-Permissions.-Supporti.patch Review of attachment 8479578 [details] [diff] [review]: ----------------------------------------------------------------- Paul, can you check if you're fine with privileged apps getting that permission?

Some background. For privilaged permissions, we have Bug 1042851 for that feature in NFC in general. The demo-hci-event app is actually certified only for the moment. I can back it off to certified only for now too.

Thanks fabrice. Updated.

(In reply to Garner Lee from comment #64) > Some background. For privilaged permissions, we have Bug 1042851 for that > feature in NFC in general. The demo-hci-event app is actually certified only > for the moment. I can back it off to certified only for now too. That's probably a good idea. Otherwise we need to figure out what guidance we need to give to marketplace security reviewers for when examining apps which use this permission.

Updated to comment reviews. And removed UNKNOWN references. (TODO added for it).

Awaiting sec comment. Trying patches: https://tbpl.mozilla.org/?tree=Try&rev=1bb632754752

Updated permissions. Re-restrict to certified only for now.

Last try push was a bad. I'll re-submit once the tree re-opens, for the check-in.

Trying: https://tbpl.mozilla.org/?tree=Try&rev=7a40c49e5d05

Current failures appear unrelated to the change. Mostly green on a limited run subset: https://tbpl.mozilla.org/?tree=Try&rev=959f63bda1e5

https://hg.mozilla.org/integration/b2g-inbound/rev/0f8132707c46 https://hg.mozilla.org/integration/b2g-inbound/rev/98d9bdb538a8

https://hg.mozilla.org/mozilla-central/rev/0f8132707c46 https://hg.mozilla.org/mozilla-central/rev/98d9bdb538a8