Facebook import screen screen does not seem to escape contact data

ASSIGNED
Unassigned

Status

Firefox OS
Gaia::Contacts
ASSIGNED
5 years ago
5 years ago

People

(Reporter: st3fan, Unassigned)

Tracking

({sec-moderate})

unspecified
x86
Mac OS X
sec-moderate

Firefox Tracking Flags

(b2g18-)

Details

(Reporter)

Description

5 years ago
I might be wrong here, and I think it would be wise if someone took a better look at it.

I think the problem is that we accept contact info from Facebook and templatize it to fb_import.html without properly escaping the data that we get back from the Facebook API.

I have browsed through the code and I don't see the usual escapeHTML() calls being made.

The data we get back from Facebook is *probably* ok but we cannot say for sure. From a security pov it is better to always escape data to prevent injection attacks. Specially data coming from foreign sources.

Comment 1

5 years ago
I don't believe that is necessary. The data is filtered out by Facebook. Doing that at our side will impact both performance and memory consumption.
(Reporter)

Comment 2

5 years ago
(In reply to Jose M. Cantera from comment #1)
> I don't believe that is necessary. The data is filtered out by Facebook.
> Doing that at our side will impact both performance and memory consumption.

Do you have a pointer to the API documentation? I'm just curious what they promise.
(In reply to Jose M. Cantera from comment #1)
> I don't believe that is necessary. The data is filtered out by Facebook.
> Doing that at our side will impact both performance and memory consumption.

It doesn't feel appropriate to be delegating the security of user's phones for performance reasons. There are many reasons this data could be bad; malicious actors at facebook, compromise of 3rd party systems. These might be unlikely but the risks are real.
(Reporter)

Comment 4

5 years ago
Yeah I agree with :mgoodwin ... properly escaping should be a rule and not an exception. Specially if the data comes from third party sources.
Agreed that we can't rely solely on the security practices of our partners.  What's the sec rating on this one?
tracking-b2g18: --- → ?
Needing info from Sid to get sec rating, and if this is a relatively high security risk should we be nominating this for tef? or leo?
Flags: needinfo?(sstamm)
Flags: needinfo?(sstamm) → needinfo?(dveditz)
(In reply to Stefan Arentz [:st3fan] from comment #0)
> I think the problem is that we accept contact info from Facebook and
> templatize it to fb_import.html without properly escaping the data that we
> get back from the Facebook API.

I agree we should not trust data from Facebook, but how would you escape it? Without knowing what contexts it's going to be used in you could pick the wrong kind of escaping.

I don't know the app at all. On the theory that our CSP protects us from the worst kinds of injections (sec-high or sec-critical) then I'm going with sec-moderate.
Flags: needinfo?(dveditz)
Keywords: sec-moderate
(Reporter)

Comment 8

5 years ago
I think it is fine to store the data as-is. As long as apps properly escape it when displaying it on screen.

The contacts code actually does this correctly in other places where it displays for example lists of contacts or details of contacts. It is just here in the Facebook integration code that a decision has been made to not escape the Facebook data.
Not tracking this since the initial comment doesn't confirm that Facebook does *not* escape contact data and we haven't got any info in here as to what the risks could be here.  Someone should get in touch with Facebook and get confirmation of the API terms/promises, as well as find out what the current escaping is and bring that info back to the discussion.  Re-nom if there is cause to show that adding filtering on B2G side will decrease risk.
tracking-b2g18: ? → -
(Reporter)

Comment 10

5 years ago
"Re-nom if there is cause to show that adding filtering on B2G side will decrease risk."

Sorry but this is advocating bad coding practices. Of course adding filtering on the B2G side decreases risk. That is why it is a mandatory coding practice.

We should encourage our developers to consistently practice secure coding.

Updated

5 years ago
Assignee: nobody → gtorodelvalle

Updated

5 years ago
Status: NEW → ASSIGNED
Setting the assignee to default until the bug since I am not currently working on this bug ;-)
Assignee: gtorodelvalle → nobody
You need to log in before you can comment on or make changes to this bug.