851213 - Facebook import screen screen does not seem to escape contact data

Status: RESOLVED INCOMPLETE

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

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

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

[Jose Manuel Cantera]

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.

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(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.

Comment 3

[Mark Goodwin [:mgoodwin]]

(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.

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Yeah I agree with :mgoodwin ... properly escaping should be a rule and not an exception. Specially if the data comes from third party sources.

Comment 5

[Lucas Adamski [:ladamski]]

Agreed that we can't rely solely on the security practices of our partners.  What's the sec rating on this one?

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

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?

Comment 7

[Daniel Veditz [: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.

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

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.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

"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.

Comment 11

[Germán Toro del Valle (:gtorodelvalle)]

Setting the assignee to default until the bug since I am not currently working on this bug ;-)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

FirefoxOS is no longer under active development.