1423510 - Hybrid Telemetry Security Review

Status: RESOLVED FIXED

(Firefox Graveyard :: Security: Review Requests, enhancement)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Project name: Hybrid Content Telemetry

Work description: Security Review for this project. The project itself is about adding an API so that (hosted) semi-privileged web pages can record Telemetry through Firefox. We designed the API (bug 1417471) and started with the implementation (bug 1417473).

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I've completed this review. Overall looks pretty similar to uitours (for better or worse). I didn't see any security issues, though Im a little nervous about there not being any validation/sanitization on the data coming from the page. 

Findings:

Mostly looks ok, seems to be basically the same as uitours. Could maybe improve the security of the API a little:
- Don’t inject the API into every page (not a security issue but don’t pollute the DOM)?
- Don’t listen for the CustomEvent on every page?
- Using a permission only raises an issue for web extensions
   - Web Extensions often have permission to inject script into ALL urls
   - This means web extensions can inject spurious data into our telemetry

We still need to know what we are doing with uitours and web extensions though, so we just will have to do the same thing.


Working notes: https://docs.google.com/document/d/14CsuQyBlH7ABDbiGBmQ3W5pfQwz4UWkQuNLdjZ7Df44/edit

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

PS One thing I do very much like that we didnt just jam this onto the uitours permission :)

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Paul Theriault [:pauljt] from comment #1)
> I've completed this review. Overall looks pretty similar to uitours (for
> better or worse). I didn't see any security issues, though Im a little
> nervous about there not being any validation/sanitization on the data coming
> from the page. 

Thank you very much for the in-depth and timely review Paul, this is much appreciated!
Both Gijs (front-end part) and :chutten (Telemetry bits) will take care of reviewing the code for the modules they own.

Regarding the sanitization/validation, I intentionally delegated that to the c++ Telemetry core which already performs these when the API is called from JS-land: https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/toolkit/components/telemetry/TelemetryEvent.cpp#761-840

The idea was to avoid replicating the same checks, as we already have them in one place (along with test-coverage). I'm happy to change this if it poses a security threat though (less happy about duplicating the test coverage :D).

> Findings:
> 
> Mostly looks ok, seems to be basically the same as uitours. Could maybe
> improve the security of the API a little:
> - Don’t inject the API into every page (not a security issue but don’t
> pollute the DOM)?
> - Don’t listen for the CustomEvent on every page?

These make sense. I'll reach out to Gijs to see how it can be done.

> - Using a permission only raises an issue for web extensions
>    - Web Extensions often have permission to inject script into ALL urls
>    - This means web extensions can inject spurious data into our telemetry

I wonder if there's a way to detect this behaviour. We should have server-side metrics that should alert us if anything too weird happens.

Comment 4

[Alessio Placitelli [:Dexter]]

I've updated the patch in the main implementation bug to account to implement the |canUpload| method, as described in the security notes. Do you or :freddyb need to take another look at the code (https://reviewboard.mozilla.org/r/204188/diff/1-2/)?

I've filed bug 1423600 to port the API to the new security infrastructure/API once it's available.

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> I wonder if there's a way to detect this behaviour. We should have
> server-side metrics that should alert us if anything too weird happens.

See also bug 1415644. We will likely prevent access here. 

(In reply to Alessio Placitelli [:Dexter] from comment #4)
> I've updated the patch in the main implementation bug to account to
> implement the |canUpload| method, as described in the security notes. Do you
> or :freddyb need to take another look at the code
> (https://reviewboard.mozilla.org/r/204188/diff/1-2/)?
> 
> I've filed bug 1423600 to port the API to the new security
> infrastructure/API once it's available.

Nothing more from my side.

Comment 6

[Paul Theriault [:pauljt] (no longer reading bugmail)]

For posterity, not that the permission has been added in bug 1444114

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Er, For posterity, NOTE that the permission has been added in bug 1444114