Closed Bug 913336 Opened 7 years ago Closed 7 years ago

Validate/sanitize arguments to NFC API calls

Categories

(Firefox OS Graveyard :: NFC, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox23 unaffected, firefox24 unaffected, firefox25 unaffected, firefox-esr17 unaffected, firefox-esr24 unaffected, b2g18 unaffected)

RESOLVED FIXED
Tracking Status
firefox23 --- unaffected
firefox24 --- unaffected
firefox25 --- unaffected
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected

People

(Reporter: pauljt, Unassigned)

References

Details

(Keywords: sec-high)

I am concerned about the ndefWrite and ndefPush methods in the webnfc API. I think there might be wrapper bypass issues, but I need assistance in trying to understand if this is an issue, and also what the solution is. (hence the CC's)

The IDL is: https://github.com/svic/mozilla-central/blob/master/dom/nfc/nsIDOMNfc.idl

There is already a note in the main bug recommending a move to WebIDL, which may well prevent this issue, but I am not sure.

The issue I am concerned about revolves about this function below (noting that _records_ comes from content): https://github.com/svic/mozilla-central/blob/master/dom/system/gonk/NfcContentHelper.js#L77

encodeNdefRecords: function encodeNdefRecords(records) {
    var encodedRecords = new Array();
    for(var i=0; i < records.length; i++) {
      var record = records[i];
      encodedRecords.push({
        tnf: record.tnf,
        type: btoa(record.type),
        id: btoa(record.id),
        payload: btoa(record.payload),
      });
    }
    return encodedRecords;
  }

This is called with a value supplied from content as part of ndefWrite [1] and ndefPush [2]. I worry that something like payload could be a reference to something cross-origin etc, and that chrome code could be induced into bypassing a security boundary on behalf on content. After showing this to David Chan from the security team, he came up with an example like this:

var record = new ndefRecord(1, "foo", "bar", frame.location) // [3][4] arg 4 is jsval
var r = mozNfc.ndefWrite([record]) // [5][6] requires an array of NdefRecord
r.onsuccess = function() { console.log(atob(this.records[0].payload)); }

Garner - can you try out the above on your build? Note that "frame" in this case is an iframe with a page loaded from a different origin (ie frame.location shouldn't be accessible in the parent page)

[1] https://github.com/svic/mozilla-central/blob/master/dom/system/gonk/NfcContentHelper.js#L125
[2] https://github.com/svic/mozilla-central/blob/master/dom/system/gonk/NfcContentHelper.js#L162
[3] - https://github.com/svic/mozilla-central/blob/master/dom/nfc/nsIDOMNdefRecord.idl#L40
[4] - https://github.com/svic/mozilla-central/blob/master/dom/nfc/NdefRecord.cpp#L45
[5] - https://github.com/svic/mozilla-central/blob/master/dom/nfc/nsNfc.cpp#L186
[6] - https://github.com/svic/mozilla-central/blob/master/dom/nfc/nsNfc.cpp#L124

This probably doesn't need to be kept closed, but I am just erring on the side of caution.
Yes, this is almost certainly a security problem, a la bug 856042. Is this API available to the web, or only b2g?

This stuff _really_ need to move to WebIDL.
It looks like it hasn't landed yet, and it'll be exposed only to certified apps when available: https://wiki.mozilla.org/WebAPI/WebNFC#Application_Permissions

There doesn't seem to be anything getting in the way of using WebIDL from looking briefly at the IDLs, so this should preferably be converted before landing on central.
For the uninformed here, how exactly does converting to WebIDL fix this?
(Simply by disallowing things such as location objects from being passed here?)
The way I understand this is (take with a grain of salt :-): Paris bindings unwrap what is passed in right away, in the content compartment, so the |new ndefRecord()| call would fail (Permission denied to access property), instead of passing an Xray through to the ndefWrite/encodeNdefRecords methods. bholley, is this right?
I believe that, with WebIDL, the fact that we have a type for record.payload will cause us to stringify it in the bindings layer, presumably in the content compartment (where stringifying window.location will throw). But I'm not positive. Boris, can you confirm?
Flags: needinfo?(bzbarsky)
Right.  WebIDL bindings help to the extent that you don't use 'any' or 'object' types or inteface types, because everything else is converted to a C++ object while still in the content compartment, and then the C++ object is converted to a new chrome-side JS value.

So if this API took a Location object, content could obviously still pass in a Location, but then presumably the API implementation would be really careful about what it did with the .href.  But if the API takes a DOMString, and you pass a Location, the toString() will happen with content permissions, not chrome ones.
Flags: needinfo?(bzbarsky)
Oh, and the point is you will still be able to pass in Location objects, but only ones for which you could call toString() in your script anyway.
(In reply to Boris Zbarsky [:bz] from comment #7)
> Right.  WebIDL bindings help to the extent that you don't use 'any' or
> 'object' types or inteface types.

Ok.  That's what I expected.
Marking high because that's what bug 856042, but NFC is unlanded so marking things unaffected, except leaving 26 unmarked in case it lands there.
Hi all, as per: https://bugzilla.mozilla.org/show_bug.cgi?id=674741#c229

The NdefRecord (in XPIDL) payload field is now checked if it's a string or not. Potentially, it could be a byte array later, but it won't take non-primitive types. There's an additional "ValidateNdef" call that type checks that it is NdefRecord as well in nsNfc.cpp.

As for when to land NFC, since it's a FxOS 1.3 feature, I believe there's now more time to move things to WebIDL and try to land again soon.

From the perspective of planning (and code maintenance to use new internal APIs doing adding Nfc user story features using those APIs), I would consider getting the code in earlier than not may be useful, as it is also currently configured to be disabled by runtime preference value. Also, it might help interested parties investigate how WebActivies can work securely with the "NFC user stories" that are planned (bluetooth pairing, wifi setup, etc.).
The NFC DOM impl is now converted to WebIDL:

Please view:
https://bug674741.bugzilla.mozilla.org/attachment.cgi?id=810257

The MozNdefRecord field "payload" is now a DOMString for the time being.

attribute sequence<octet> payload was rejected by the code generator, and apparently, the WebIDL Wiki indicates Arrays aren't fully supported yet (I was pointed to ImageData where it does use one though).
Flags: needinfo?(reuben.bmo)
Typed arrays (not to be confused with the never-will-be-implemented "IDL Array Objects") work fine, as long as you want the "payload" to be writable randomly (as in, having someone mutate the middle of it).
(In reply to Boris Zbarsky [:bz] from comment #13)
> Typed arrays (not to be confused with the never-will-be-implemented "IDL
> Array Objects") work fine, as long as you want the "payload" to be writable
> randomly (as in, having someone mutate the middle of it).

Typed arrays read only doesn't extend to elements? The fields are intended to be readonly (which seems to align with the current W3C draft).
Readonly just affects whether the page can set the property on the object.  It says nothing about whether it can modify the state of whatever you return from the property getter.
Thanks for the explanation.

I'll work on trying out the Uint8ClampedArray. I think it should be compatible. Uint8Array resulted in a C++ compile error when I tried yesterday. I don't know if switching the field could prevent the landing (API change), but I'll work on both in parallel.
Uint8ClampedArray and Uint8Array look identical-ish on the C++ side.  What error did you get, exactly?  Or was this for a JS-implemented WebIDL API?
(In reply to Boris Zbarsky [:bz] from comment #17)
> Uint8ClampedArray and Uint8Array look identical-ish on the C++ side.  What
> error did you get, exactly?  Or was this for a JS-implemented WebIDL API?

Just to do a minor update: Nevermind. What happens is a Uint8Array required in the constructor binding is returned as an JSObject*. No problems.
Sorry, I keep missing requests because hidden bugs don't show up in my dashboard. It looks like you're already on the right track here using a typed array for payload. Can you perhaps use generated events for MozNfcEvent? Or do we still have problems with any/jsval attributes?
Flags: needinfo?(reuben.bmo)
(In reply to Reuben Morais [:reuben] from comment #19)
> Sorry, I keep missing requests because hidden bugs don't show up in my
> dashboard. It looks like you're already on the right track here using a
> typed array for payload. Can you perhaps use generated events for
> MozNfcEvent? Or do we still have problems with any/jsval attributes?

Concerning MozNfcEvent, it's no longer in the API, given that it had no new use due to rewrites to take some aspects of NFC W3C spec draft 1 (we're moving APIs around to do it, though the spec doesn't really cover application launches due to routing NDEF messages). I *think* that's the last immediate change request.

http://w3c.github.io/nfc/proposals/common/nfc.html

As for Uint8Array for MozNdefRecord (WebIDL now) fields (id, type, payload), it's currently 3 DOMStrings, not jsvals or any. That extra change will be in a future patch after we get things for the initial landing (shifting feature schedules), very likely in the patch that supports Nexus 4 NFC hardware (binary protocol).

The next patch upload has never everything written in JS implemented WebIDL DOM interfaces...which should get all the sanitation done in the generated C++ binding code.
never --> near.
Concerning the 10/25 DOM landing deadline, given all content facing APIs presented are flowing through WebIDL generated bindings, are there any remaining concerns from the security perspective? If so, can we identify them, and get them resolved?
Flags: needinfo?
Is there a link to the current API?

If the API does not use the "object" or "any" types and doesn't use callback functions and callback interfaces, then it should be safe.

If it uses any of those, the uses need to be very carefully audited.
Flags: needinfo?
Hi (bz): https://bugzilla.mozilla.org/show_bug.cgi?id=674741

The NFC DOM is in Patch 1, in the (current) attachment https://bugzilla.mozilla.org/attachment.cgi?id=820050.

There are no arbitrary arguments or callbacks with "any" or "object". There will be event handlers like onpeerfound/lost and onforeground dispatch, but in future patches only. Those are expected to have specific types declared.
Hi, this is somewhat tangentally related to security of the same WebIDL objects (although, if different, I will open a new bug). The test case referenced says the new DOM objects are exposed to web pages/apps. I believe that's the intent, unless there is a mechanism to cleanly distinguish where the boundary between local app and a internet connected webpage might be. 

Do we need to update the test cases?

https://bugzilla.mozilla.org/show_bug.cgi?id=674741#c399
Flags: needinfo?
I commented in the other bug.
Flags: needinfo?
Hi Paul, can we close this bug, as fixed? We're using WebIDL now.
Flags: needinfo?(ptheriault)
Sure can, Thanks!
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(ptheriault)
Resolution: --- → INVALID
Group: b2g-core-security
Resolution: INVALID → FIXED
You need to log in before you can comment on or make changes to this bug.