1059168 - B2G NFC: Make sure the techList and origin string won't overflow in NfcService

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

It's possible that there's something wrong while unmarshalling parcels from nfcd, so it might overflow when convert the ints to String.

Comment 1

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

[Blocking Requested - why for this release]:

regression from Bug 933588

Comment 2

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: per comment1, this is a potential regression

Comment 3

[Henry Chang [:hchang]]

Yoshi, 

Do you mean [1] and [2]? It seems converting |mEvent.mOriginType| 
to string is already safe. Only [1] may cause out-of-bounds 
array access. I can pick this as my first nfc bug even though
it's not quite relevant to nfc :p Thanks!

[1] http://hg.mozilla.org/mozilla-central/file/c360f3d1c00d/dom/nfc/gonk/NfcService.cpp#l132
[2] http://hg.mozilla.org/mozilla-central/file/c360f3d1c00d/dom/nfc/gonk/NfcService.cpp#l176

Comment 4

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

yeah, but I already had patch for this. :P

Comment 5

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

Attachment: Patch

Comment 6

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

Comment on attachment 8482658 [details] [diff] [review]
Patch

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

Hi smaug
Bug 1053732 is still under review in Gaia part, so this patch is base on m-c.
Could you review this for me?

Thanks

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8482658 [details] [diff] [review]
Patch

>   notifyHCIEventTransaction: function notifyHCIEventTransaction(message) {
>+    message.origin = message.originType + message.originIndex;
This really needs some explanation. What does the origin mean here?
Why can't all the users of the API use originType + originIndex?
Or if they can't, why do we need originType + originIndex and not just origin

>-static const nsLiteralString SEOriginString[] = {
>-  NS_LITERAL_STRING("SIM"),
>-  NS_LITERAL_STRING("ESE"),
So here you have "ESE", but the new webidl enum has "eSE". Why that difference?


> 
>     // HCI Event Transaction parameters.
>-    int size = sizeof(SEOriginString) / sizeof(nsLiteralString);
>-    // TODO: We need a map or something to more rigorously validate against
>-    // Gonk Message header values from inside NfcService.
>-    if ((mEvent.mOriginType != -1) && (mEvent.mOriginType < size)) {
>-      mEvent.mOrigin.Assign(SEOriginString[mEvent.mOriginType]);
>-      mEvent.mOrigin.AppendInt(mEvent.mOriginIndex, 16 /* radix */);
>-      event.mOrigin.Construct();
>-      event.mOrigin.Value() = mEvent.mOrigin;
I have no idea what this old code does, but getting rid of it looks good.

> enum NFCTechType {
>+  "NDEF",
>+  "NDEF_WRITABLE",
>+  "NDEF_FORMATABLE",
>+  "P2P",
>   "NFC_A",
>   "NFC_B",
>-  "NFC_ISO_DEP",
>   "NFC_F",
>   "NFC_V",
>-  "NDEF",
>-  "NDEF_FORMATABLE",
>+  "NFC_ISO_DEP",
>   "MIFARE_CLASSIC",
>   "MIFARE_ULTRALIGHT",
>-  "NFC_BARCODE",
>-  "P2P",
>-  "UNKNOWN_TECH"
> };
Why the reordering and why dropping NFC_BARCODE?


r- just because I want some explanations (I'm not too familiar with some of this code).

Comment 8

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

(In reply to Olli Pettay [:smaug] from comment #7)
Hi Smaug
Sorry for not being clear.


> >   notifyHCIEventTransaction: function notifyHCIEventTransaction(message) {
> >+    message.origin = message.originType + message.originIndex;
> This really needs some explanation. What does the origin mean here?

It's from GSMA spec, [1]
Origin means the source of event, it could be from SIM, embedded SE, or ASSD (SD card with Secure element).
And in case that more than one of Secure Elements are of the same kind , from the specification it uses a digit to indentify them, like SIM1, SIM2.

> Why can't all the users of the API use originType + originIndex?
Also from the same specification, it uses a String to represent the origin, so users of this API will expect they get a String like 'SIM1', so I concat them.

> Or if they can't, why do we need originType + originIndex and not just origin
> 
That also works(just origin), that's pretty what the original code is doing.

> >-static const nsLiteralString SEOriginString[] = {
> >-  NS_LITERAL_STRING("SIM"),
> >-  NS_LITERAL_STRING("ESE"),
> So here you have "ESE", but the new webidl enum has "eSE". Why that
> difference?
>
From the specification it uses the term 'eSE' so that's why I do the renaming here.
But I'll file another bug to fix the typo since it's not quite related to this bug.


> > enum NFCTechType {
> >+  "NDEF",
> >+  "NDEF_WRITABLE",
> >+  "NDEF_FORMATABLE",
> >+  "P2P",
> >   "NFC_A",
> >   "NFC_B",
> >-  "NFC_ISO_DEP",
> >   "NFC_F",
> >   "NFC_V",
> >-  "NDEF",
> >-  "NDEF_FORMATABLE",
> >+  "NFC_ISO_DEP",
> >   "MIFARE_CLASSIC",
> >   "MIFARE_ULTRALIGHT",
> >-  "NFC_BARCODE",
> >-  "P2P",
> >-  "UNKNOWN_TECH"
> > };
> Why the reordering and why dropping NFC_BARCODE?

Basically the same idea from your comment https://bugzilla.mozilla.org/show_bug.cgi?id=1053732#c9

The original code from NfcOptions.webidl uses sequence<DOMString> techList, I thought I might reuse the 'enum NFCTechType', and the reordering is to make sure it could also work in lower level deserialization in NfcService.cpp, to make it have the order of the 'nsLiteralString NfcTechString[]' defined NfcService.cpp, so I could remove NfcTechString since WebIDL binding code has already defined that.

And dropping NFC_BARCODE is the same reason, NfcTechString[] doesn't define this, but to prevent confusion I'll create another part of patch to remove these non-used enums defined in MozNFCTag.webidl.

Thanks for the review and sorry for the confusion here.
I'll try to use a DOMString origin in NfcOptions.webidl to see if it's easier.


[1] : http://www.gsma.com/newsroom/wp-content/uploads/2014/03/TS26v5-0.pdf

Comment 9

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

(In reply to Yoshi Huang[:allstars.chh] from comment #8)
> And dropping NFC_BARCODE is the same reason, NfcTechString[] doesn't define
> this, but to prevent confusion I'll create another part of patch to remove
> these non-used enums defined in MozNFCTag.webidl.
>
Actualy NFC_BARCODE is defined in NfcService but called 'BARCODE', I'll add it back.

Comment 10

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

Attachment: Patch v2.

Use DOMString origin

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8483380 [details] [diff] [review]
Patch v2.

> enum SecureElementOrigin {
>   SIM = 0,
>   ESE = 1,
>   ASSD = 2,
>+  OriginEndGuard
maybe set to = 3 explicitly

Comment 12

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

Attachment: Patch v3.

assign OriginEndGuard = 3

Comment 13

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

try :https://tbpl.mozilla.org/?tree=Try&rev=d3cf55db08ad

https://hg.mozilla.org/integration/b2g-inbound/rev/07442dfa17e3

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/07442dfa17e3

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Note that due to recent policy changes, all patches must have approval for uplift regardless of blocking status. Please request aurora approval on this patch when you get a chance. Sorry for the inconvenience :(

Comment 16

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

Comment on attachment 8483907 [details] [diff] [review]
Patch v3.

Approval Request Comment
[Feature/regressing bug #]:
933588, 979767

[User impact if declined]:
Prevent crash by buffer overflow

[Describe test coverage new/current, TBPL]:
Demo Wallet app

[Risks and why]: 
No

[String/UUID change made/needed]:
No

Comment 17

[bhavana bajaj [:bajaj]]

(In reply to Yoshi Huang[:allstars.chh] from comment #16)
> Comment on attachment 8483907 [details] [diff] [review]
> Patch v3.
> 
> Approval Request Comment
> [Feature/regressing bug #]:
> 933588, 979767
> 
> [User impact if declined]:
> Prevent crash by buffer overflow
> 
> [Describe test coverage new/current, TBPL]:
> Demo Wallet app
> 
> [Risks and why]: 
> No
> 
> [String/UUID change made/needed]:
> No

Do we have any automation tests for this ?

Comment 18

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

(In reply to bhavana bajaj [:bajaj] from comment #17)
> > Approval Request Comment
> > [Feature/regressing bug #]:
> > 933588, 979767
> > [Describe test coverage new/current, TBPL]:
> > Demo Wallet app
Sorry, should be Marionette-webapi + Demo Wallet app

> > 
> Do we have any automation tests for this ?

For the 'techList' part check (from Bug 933588), marionette-webapi also covers it.
But from the 'origin' (from Bug 979767), there's no automation test yet, so using a demo app to verify this.

Comment 19

[bhavana bajaj [:bajaj]]

(In reply to Yoshi Huang[:allstars.chh] from comment #18)
> (In reply to bhavana bajaj [:bajaj] from comment #17)
> > > Approval Request Comment
> > > [Feature/regressing bug #]:
> > > 933588, 979767
> > > [Describe test coverage new/current, TBPL]:
> > > Demo Wallet app
> Sorry, should be Marionette-webapi + Demo Wallet app
> 
> > > 
> > Do we have any automation tests for this ?
> 
> For the 'techList' part check (from Bug 933588), marionette-webapi also
> covers it.
> But from the 'origin' (from Bug 979767), there's no automation test yet, so
> using a demo app to verify this.

Can you please file a separate bug to add missing automation tests here and check that in ?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f6dd0320bca