1165340 - [Secure Element] Perform ACE.isAccessAllowed check before dispatching HCI EVT TRANSACTION

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Krzysztof Mioduszewski[:tauzen]]

Before dispatching system message containing HCI EVT_TRANSACTION data we need to consult ACE to check if web apps can actually receive it.

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Bug 1165340 - Part 1: Using SEUtils and se_consts

These are mostly 'nit' changes (but needed). I'm considering to combine this with Part 2 patch.

Comment 2

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part 2: Consult ACE before dispatching HCI system message

Comment 3

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part 3: Use 'UICC' as SE type/origin across the stack

Comment 4

[Krzysztof Mioduszewski[:tauzen]]

Updated xpcshell tests are also required. Currently I'm having problems with running the test on latest version of emulator-jb.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8606317 [details] [diff] [review]
Part 3: Use 'UICC' as SE type/origin across the stack

Review of attachment 8606317 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/gonk/NfcService.cpp
@@ +189,5 @@
>        MOZ_ASSERT(static_cast<HCIEventOrigin>(mEvent.mOriginType) < HCIEventOrigin::EndGuard_);
>  
>        event.mOrigin.Construct();
>        event.mOrigin.Value().AssignASCII(HCIEventOriginValues::strings[mEvent.mOriginType].value);
> +      // Open Mobile API specification V2.05, section 6.6.1, index 1 is optional

Quota from specification:
The slot number “1” for a reader is
optional (SIM and SIM1 are both valid for the first SIM-reader, but if there are two
readers then the second reader must be named SIM2)

The specification is for API, not for the implementation.
So the caller of this API should assume the first SE will be "SIM" or "SIM1", it does not say that the implementation should make the SIM1 to SIM.

In our case, our RIL stack (Gecko part) supports multi-SIM (although Flame doesn't support 2 SIM-SEs), so using SIM1 makes our implementation easier.

::: dom/webidl/NfcOptions.webidl
@@ +46,5 @@
>  /**
>   * The source of HCI Transaction Event.
>   */
>  enum HCIEventOrigin {
> +  "UICC",

This seems should file another bug to discuss this as I remember some spec (even OpenMobile) are still using SIM.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8606315 [details] [diff] [review]
Bug 1165340 - Part 1: Using SEUtils and se_consts

Review of attachment 8606315 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/messages/HCIEventTransactionSystemMessageConfigurator.js
@@ +8,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "AppsService",

Why do you change to upper-case?
I check current codebase all uses 'appService'.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8606316 [details] [diff] [review]
Part 2: Consult ACE before dispatching HCI system message

Review of attachment 8606316 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/messages/HCIEventTransactionSystemMessageConfigurator.js
@@ +15,5 @@
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "SEUtils",
>                                    "resource://gre/modules/SEUtils.jsm");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "ACEService",

Ditto
Also move KazyServiceGetter together.

Comment 8

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (1.1 nit fixes) Part 1: Using SEUtils and se_consts

Fix requested in comment 6.

Comment 9

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (1.1 nit fixes) Part 2: Consult ACE before dispatching HCI system message

Fixed according to comment 7.

Comment 10

[Krzysztof Mioduszewski[:tauzen]]

Attachment: Part 3: Update xpcshell tests

xpcshell tests are working for me now on emulator-kk. Yoshi could you review this?

Comment 11

[Krzysztof Mioduszewski[:tauzen]]

Bug 1170220 was raised to discuss the naming of origin/se as it was suggested in comment 5.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8613607 [details] [diff] [review]
Part 3: Update xpcshell tests

Review of attachment 8613607 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/tests/unit/test_HCIEventTransactionSystemMessageConfigurator.js
@@ +43,5 @@
> +let aceAccessAllowed = true;
> +
> +function setMockServices(manifest) {
> +  XPCOMUtils.defineLazyServiceGetter = (obj, service) => {
> +    if(service === "appsService") {

if (...)

@@ +56,5 @@
> +        },
> +      };
> +    }
> +
> +    if(service === "aceService") {

else if ()

@@ +129,5 @@
> +  aceAccessAllowed = true;
> +
> +  HCIEvtTransactionConfigurator
> +  .shouldDispatch(MANIFEST_URL, PAGE_URL, TYPE, TEST_MESSAGES[1])
> +  .then((result) => {

Should merge these two tests and compare result with aceAccessAllowed directly.

These two functions are doing the same thing

Comment 13

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (1.1) Part 3: Update xpcshell tests

Fixed issues raised in comment 12.

Comment 14

[Krzysztof Mioduszewski[:tauzen]]

try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2369a578339

Comment 15

[Krzysztof Mioduszewski[:tauzen]]

Try build all green.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/1d3c6e61c651
https://hg.mozilla.org/integration/b2g-inbound/rev/2b8d4fb773c5
https://hg.mozilla.org/integration/b2g-inbound/rev/141051964661

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d3c6e61c651
https://hg.mozilla.org/mozilla-central/rev/2b8d4fb773c5
https://hg.mozilla.org/mozilla-central/rev/141051964661