1007591 - [SMS] Make SMS react upon the IAC broadcast from Cost Control application to friendly mute Cost Control specific SMS

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Salvador de la Puente González [:salva]]

In order to fulfill bug 987155 we need SMS app to be quiet when Cost Control is sending its specific SMS.

Comment 1

[Marina Rodríguez [:mai]]

Attachment: patch v1.0

Hi,
do you mind reviewing the patch?
Regards

Comment 2

[Julien Wajsberg [:julienw]]

I'd like some more information about what the patch is actually doing, what is the used algorithm and the rationale.

Thanks!

Comment 3

[Marina Rodríguez [:mai]]

Hi Julien, 

This patch tries to improve the current SMS app blacklist feature. That feature avoids the vibration, sound and notification for the sent/received SMS when the sender is part of that list which is generated from "blacklist.json" file, at build time (so it can't be modified/activated/deactivated)

This feature is currently used by Cost Control app for managing balance and Top-up requests in one country. We would like to use it in a global way, all the countries. The problem is that there is no dedicated/exclusive numbers for balance/Top-up services for each country, it means, the same number can be used for another services, not just balance/Top-up in a specific country, moreover, the same number in one country could be used by another one for other services (eg. promotions). For those reasons, we'd need a dynamic list that can be activated/deactivated as needed during a period of time.

This patch has been implemented taking into account the following workflow:

  1. CC app will ask SMS app to silence a list of senders during a period of time via IAC communication
  2. When SMS app confirms via IAC that the senders have been silent , CC app will send the corresponding balance/Top-up message
  3. If an answer is received during the silent period:
       3.1 The SMS app won't send any type of notification or sound.
       3.2 In case CC app detects the answer before finishing the silent period it will immediately ask SMS app to disable the silent mode for that list
  4. If no answer is received during this period: The SMS app automatically deactivates the silence mode for those numbers (To implement this, we set a timeout alarm on the SMS app)


The IAC communication can only be established between certified apps, but if you are concerned about the use of this feature by another app, it's possible to check the origin of the  connection to guarantee which app uses it.

I don't know if I miss something, I hope this clarify your questions.
Regards

Comment 4

[Marina Rodríguez [:mai]]

I've updated the PR with the Julien comments.

Comment 5

[Salvador de la Puente González [:salva]]

Comment on attachment 8426286 [details] [review]
patch v1.0

I like it. Address the comments on GitHub but wait for Borja's review.

Comment 6

[Marina Rodríguez [:mai]]

Updated the PR with the comments of Salva.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8426286 [details] [review]
patch v1.0

I want to review this once more before this goes in.
Thanks for the explanation and sorry for the delay

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8426286 [details] [review]
patch v1.0

Yuren, can you please have a quick look to the changes in the build files? Should be easy but I prefer that you have a look.

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8426286 [details] [review]
patch v1.0

I finally reviewed this, sorry for the delay.

I really think SilentSMS should use Promises for the various asynchronous steps, that will lead to clearer and cleaner code.

Also, maybe we should have a safety net: keeping the blacklist.json file to have a list of numbers that we can actually blacklist (even temporarily). I don't feel comfortable that any number could be made silent...

I admit I don't have the big picture though, so if you think it should stay like this, I'd like to ask the security peers.

Comment 10

[Julien Wajsberg [:julienw]]

Also, please ask review from :steveck if you need a review before the end of this week as I'm in holidays (in your country ;) ) for some days.

Comment 11

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8426286 [details] [review]
patch v1.0

r+++++++

Comment 12

[Marina Rodríguez [:mai]]

updated The pr with the last comments of Salva

Comment 13

[Marina Rodríguez [:mai]]

Updated the PR with the comments of Borja

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 8426286 [details] [review]
patch v1.0

(adding r- just because there is a r+ already)

added more comments.

It just occurred to me that we can have issues with simultaneous requests :( I'm afraid we need to do a task runner...

Comment 15

[Julien Wajsberg [:julienw]]

NI Stéphanie to check the function from the security point of view.

Especially, I'd like to know if we should keep a list of phone numbers that can be silenced, or if the current code and architecture is safe enough already.

Comment 16

[Julien Wajsberg [:julienw]]

> if we should keep a list of phone numbers that can be silenced

Maybe, we can already have a mapping of "type -> numbers" in the sms app, and the IAC would only enable/disable the type.

Comment 17

[Marcelino Veiga Tuimil [:sonmarce]]

Those numbers are already included in cost control configuration files, that is more accurate as they are defined per country (MCC/MNC) for the final purpose (check ballance & top up), and it would be the only app able to call SMS app

Comment 18

[Marina Rodríguez [:mai]]

Hi Stéphanie, 
The patch implement the following security rules:
Only the costcontrol app could make a request to silent sms numbers (a rule form the manifiest field)
The costcontrol app generates the list of sms numbers from its configuration files (filtering by mcc/mnc).
The IAC communication is allowed only for certified apps.
Regards

Comment 19

[Stephanie Ouillon [:arroway] (needinfo me)]

My comment collided with Marina's and Marcelino's at the same time, anyway here it is, it's globally saying the same thing:

From a security perspective, I see two main potential threats: 

1) Somebody can control the list of senders to be silenced in the cost control app. 

The lists of senders to be silenced are hardcoded in costcontrol/js/config/* in the cost control certified app, so from that point of view it looks safe.

2) Another app can communicate via IAC with the sms app and send it a non-legitimate list to be silenced.

The manifest settings related to the IAC API allow to specify the sms app is to talk to the cost control app only. Thus it shouldn't be tricked to received a list of senders from another source. The code have already addressed the issue of checking that it is dealing with the expected request.

So from a high level point of view, I don't see any particular security issue as far as I understand.

Comment 20

[Borja Salguero [:borjasalguero]]

Comment on attachment 8426286 [details] [review]
patch v1.0

Im gonna leave the review to Julien here, due to he started the review before and there are some comments to address.

My main concern is how the tests are created, it's quite tough to read and probably we need a better level of granularity in order to check small things, instead of end-to-end ones.

Comment 21

[Julien Wajsberg [:julienw]]

(In reply to Borja Salguero (this week part-time in Gaia) [:borjasalguero] from comment #20)

> My main concern is how the tests are created, it's quite tough to read and
> probably we need a better level of granularity in order to check small
> things, instead of end-to-end ones.

yes, I agree with this, but I think we'll revisit this later. It's already good to have tests :)

Comment 22

[Julien Wajsberg [:julienw]]

I worked a lot with Marina today, and the patch is going really well. I've seen issues in the costcontrol patch that I reported in Bug 1007588 comment 4.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 8426286 [details] [review]
patch v1.0

added some more comments

I think the next round will be the good one !

Thanks for this good work !

Comment 24

[Julien Wajsberg [:julienw]]

Comment on attachment 8426286 [details] [review]
patch v1.0

r=me

please land with a green travis

Thanks !

Comment 25

[Marina Rodríguez [:mai]]

Master: 857129928b6e56a809cee9d5445effb8fa9f1c2c