1001619 - Security Review: Directory Tiles Server-side

Status: RESOLVED FIXED

(mozilla.org :: Security Assurance: Review Request, task)

Description

[Olivier Yiptong [:oyiptong]]

Who is/are the point of contact(s) for this review?
[e-mail]

Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):
This is a webservice that supports the Directory Tiles features of Firefox Desktop and Fennec (eventually).
It serves Directory Links when clients call on an API endpoint.
It also captures impression and click data pings.

Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description

Server application server can be found at: https://github.com/oyiptong/onyx

We plan on riding the Firefox release trains and would like to have this launched for nightly within the next few weeks.

To help prioritize this work request, does this project support a goal specifically listed on this quarter's goal list? If so, which goal?

It fits in "Invest in Sustainability"/Content Services and "Get Firefox on a Growth Trajectory"/Firefox

Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?
Yes

Are there any portions of the project that interact with 3rd party services?
Yes. We will be using Amazon Web Services for hosting the application, assets and will be using AWS services.

Will your application/service collect user data? If so, please describe
Yes. Impression data, clicks, unique user identifiers

Comment 1

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

Couple of quick questions:

* Is the current code stable/finished enough to start doing this review?
* Is there a development or staging instance running somewhere?
* Is there API documentation or some high level documentation about what this service implements and how it interacts with Firefox?

Comment 2

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

Because this project is relatively small, I am going to add comments inline here as comments.

Comment 3

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

This is not really a security issue per se, but I think we should not encrypt the session data.

There are only two things in the cookie, a session id and a timestamp. I think for the sake of transparency of this project, which should be as high as possible, those two values should be in the cookie in plain text.

If you are worried about people tampering with those values, I would suggest to only add a simple hash to the cookie. This is actually what Flask does by default. It's session values are stored in a cookie as:

> base64(json(session_dict)) + "." + hash(session_dict, SECRET_KEY)

If you don't want to lock in to the Flask method then I would suggest to do something very similar manually and use a simple HMAC-SHA1 to sign the data.

Comment 4

[Olivier Yiptong [:oyiptong]]

I think Flask already does HMAC-SHA1 signing by default via the itsdangerous package.

SecureSessions use itsdangerous by default.

Comment 5

[Olivier Yiptong [:oyiptong]]

(In reply to Stefan Arentz [:st3fan] from comment #1)
> Couple of quick questions:
> 
> * Is the current code stable/finished enough to start doing this review?
Yes
> * Is there a development or staging instance running somewhere?
No
> * Is there API documentation or some high level documentation about what
> this service implements and how it interacts with Firefox?
Yes, there are a couple:

https://docs.google.com/a/mozilla.com/document/d/1_9pTBxmabAMLxNESfchTOsY1U5E1BzFQiJgdVsQJQ20

and

https://docs.google.com/a/mozilla.com/document/d/1ItO5SuUQpPr0Sls0GK-TXxSP_3ZhEEB86z4UjyGeC0w

Comment 6

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

This is more about privacy than security: I see the IP address is sent to Heka. Is that actually used in the backend?

Comment 7

[Olivier Yiptong [:oyiptong]]

(In reply to Stefan Arentz [:st3fan] from comment #6)
> This is more about privacy than security: I see the IP address is sent to
> Heka. Is that actually used in the backend?

Yes, we are building reporting infrastructure. One of the uses of the IP address is to obtain geo data.
We plan on sending just the approx geolocation to sponsors, and also for our own bean counting.

Comment 8

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

Ok I do not know what our policies are WRT storing IP addresses. You should probably check that with someone on the privacy team.

Comment 9

[Olivier Yiptong [:oyiptong]]

Bryan, what is our position about storing IP addresses?

The server is currently aggregating IP addresses to compute geo location. We will be doing that outside of the response/request cycle, meaning that the data will be stored elsewhere as implemented.

Does that comply with our privacy policy? How about the end-user agreement?

Comment 10

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

My only other concern is that the SESSION_COOKIE_SECURE setting in Flask defaults to False. I wonder if it would make sense to programmatically set that based on the extra headers that the front-end web server or proxy sets.

Comment 11

[Olivier Yiptong [:oyiptong]]

I was planning to have a config that overrides the default settings.
The production config would have SESSION_COOKIE_SECURE set to True

Comment 12

[Olivier Yiptong [:oyiptong]]

Attachment: production config sample

Comment 13

[Olivier Yiptong [:oyiptong]]

Attachment: production config sample

Comment 14

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

Great. I'm going to close this to RESOLVED/FIXED. Please reopen if things change and need a new look.

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Olivier Yiptong [:oyiptong] from comment #9)
> Bryan, what is our position about storing IP addresses?
> 
> The server is currently aggregating IP addresses to compute geo location. We
> will be doing that outside of the response/request cycle, meaning that the
> data will be stored elsewhere as implemented.
> 
> Does that comply with our privacy policy? How about the end-user agreement?

I'll try to gather the FHR policy on this because we're lining up with their usage but my understanding from that team is that we don't store IP addresses at all.  We can use the IP address in our geoIP service to gather a region but once we've detected the region we drop the IP address.  The region is required for Directory Tiles so we'll need to use the IP address to determine region but beyond that need the IP address has no value to us and likely leaks too much additional information.

Comment 16

[Olivier Yiptong [:oyiptong]]

(In reply to Bryan Clark (Firefox Search PM) [:clarkbw] from comment #15)
> I'll try to gather the FHR policy on this because we're lining up with their
> usage but my understanding from that team is that we don't store IP
> addresses at all.  We can use the IP address in our geoIP service to gather
> a region but once we've detected the region we drop the IP address.  The
> region is required for Directory Tiles so we'll need to use the IP address
> to determine region but beyond that need the IP address has no value to us
> and likely leaks too much additional information.

Is it OK to obtain the location from the geoIP service outside of the front-end server?

The IP addresses will then go from the front-end server, to a message queue, then will ship to a worker that will translate this data and strip out the IP address. The data would not be stored permanently, but will be stored temporarily, in a work queue.

Comment 17

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Olivier Yiptong [:oyiptong] from comment #16)
> (In reply to Bryan Clark (Firefox Search PM) [:clarkbw] from comment #15)
> > I'll try to gather the FHR policy on this because we're lining up with their
> > usage but my understanding from that team is that we don't store IP
> > addresses at all.  We can use the IP address in our geoIP service to gather
> > a region but once we've detected the region we drop the IP address.  The
> > region is required for Directory Tiles so we'll need to use the IP address
> > to determine region but beyond that need the IP address has no value to us
> > and likely leaks too much additional information.
> 
> Is it OK to obtain the location from the geoIP service outside of the
> front-end server?
> 
> The IP addresses will then go from the front-end server, to a message queue,
> then will ship to a worker that will translate this data and strip out the
> IP address. The data would not be stored permanently, but will be stored
> temporarily, in a work queue.

That's a good question I'm not sure I'm qualified to answer well enough.