1592321 - Collect a sanitized path for files on Windows in the same manner as untrusted modules

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

We have WinUtils::PreparePathForTelemetry which sanitizes a filepath using an allowlist. It returns either a bare filename or a subpath of %ProgramFiles%, %SystemRoot%, or %TEMP%. We collect this data for untrusted modules being loaded into Firefox, which is very similar in concept to the notion of weird things added to the browser doing eval().

Comment 1

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1592321 - Collect a sanitized path for files doing eval() on Windows in the same manner as untrusted modules r?aklotz

Comment 2

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: data-review.txt

Comment 3

[Chris H-C :chutten]

Comment on attachment 9104961 [details]
data-review.txt

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is Telemetry so is documented in its definitions file [Events.yaml](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Events.yaml) and the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, :tjr and :ckerschb is responsible.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

    Is the data collection request for default-on or default-off?

Default on for all channels.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---
Result: datareview+

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

While working on tests for this, I determined that the Windows function I was using to get the filename was actually still getting it if it behaved as a URL. I wasn't entirely sure how to handle that, so I ended up grabbing the scheme - if it was recognized as a URL - and including that in the report because it could actually better identify a piece of software - in a not-unqiue manner - than a single filename.

However if the scheme is http/https/ftp I only include the scheme and not anything else. Examples (from the tests now in the commit):

gallop://thing/fire -> gallop://.../fire
gallop://fire -> gallop://.../fire
firegestures/content -> content
firegestures\\content -> content
/home/tom/files/thing -> thing
file://c/uers/tom/file.txt -> file://.../file.txt
c:/uers/tom/file.txt -> file.txt
http://example.com/ -> http
http://example.com/thing.html -> http

Now, in addition to the filename on a user's computer, we might receive a scheme that indicates a particular software is installed (and that something is injecting a file identified by that scheme into Firefox.)

Chutten, does this change data-review at all? Should I rewrite the text file and re-quest re-approval?

Comment 5

[Chris H-C :chutten]

The data collection review was for bare filenames or filenames + paths under system root, program files, or temp. Adding these schemes and final url path components is a definite increase in the data being collected. So we're going to need to either update the existing data collection review or add a new one.

I'm not sure that this is still cat1 collection, though, as the final component of a non-file-scheme URI could be anything. Would you still be able to answer your questions if we stick to just the scheme for non-file: schemes? (and maybe a sentinel "<no scheme>" for those without schemes?)

Comment 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: data-review.txt

(In reply to Chris H-C :chutten from comment #5)

The data collection review was for bare filenames or filenames + paths under system root, program files, or temp. Adding these schemes and final url path components is a definite increase in the data being collected. So we're going to need to either update the existing data collection review or add a new one.

Here's a new file. Section (5) is the only one I changed.

I'm not sure that this is still cat1 collection, though, as the final component of a non-file-scheme URI could be anything. Would you still be able to answer your questions if we stick to just the scheme for non-file: schemes? (and maybe a sentinel "<no scheme>" for those without schemes?)

Well, I don't know if any of this collection will answer our questions, I'm just trying to make an incremental collection that I hope will; and if it doesn't - I'll be back.

But you're right - the scheme itself is likely more important than whatever is at the end of it, so I changed it to only collect the scheme if it's a non-file scheme.

You're also right that this is a less-than-ideal fit for Category 1. It's technical data about Firefox, but technical data about potentially-unique modifications to Firefox. It is; however, extremely similar to the third party module effort - because this is stuff going into our process that we don't intend to have in there. Like third party modules, some we want to block and some we don't. But if users want any of them to continue to function, we do need to know more about them to support them.

Comment 7

[Chris H-C :chutten]

Comment on attachment 9107597 [details]
data-review.txt

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is Telemetry so is documented in its definitions file [Events.yaml](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Events.yaml) and the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Tom Ritter and Christoph Kerschbaumer are responsible.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

    Is the data collection request for default-on or default-off?

Default on for all channels.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---
Result: datareview+

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1063d960295
Collect a sanitized path for files doing eval() on Windows in the same manner as untrusted modules r=aklotz

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c1063d960295

Comment 10

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9104948 [details]
Bug 1592321 - Collect a sanitized path for files doing eval() on Windows in the same manner as untrusted modules r?aklotz

Beta/Release Uplift Approval Request

• User impact if declined: This patch collects data about things hat prevent us from deploying a security feature. Delaying it will cause us to wait at least another release before deploying it.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): While it has been deployed in Nightly, we have not yet received any data from this telemetry. (So the code hasn't run.) We may never though see it in Nightly though - the Nightly population is small, and the reports we received really only moved the needle slightly Beta and then really picked up in Release.

The parsing code is covered by gtests.

• String changes made/needed:

Comment 11

[Pascal Chevrel:pascalc]

Tom, the explanation on why this patch is of medium risk looks more like an explanation of why you need this fix, not what unwanted side-effects it could have on our userbase, could you explain why this is a medium risk uplift? Thanks

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I'm removing the uplift request. In Nightly we just got a few submissions indicating that calls are coming from a scheme moz-extension://, in the parent process, not using the system context. That's all we know, but it's enough to either begin investigation or iterate on how to collect more information safely.

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9104948 [details]
Bug 1592321 - Collect a sanitized path for files doing eval() on Windows in the same manner as untrusted modules r?aklotz

Re-requesting uplift because we identified the issue we think is the problem (in Bug 1596421) but if we still trigger results this would help us figure out what the source of results would be.

(In reply to Pascal Chevrel:pascalc from comment #11)

Tom, the explanation on why this patch is of medium risk looks more like an explanation of why you need this fix, not what unwanted side-effects it could have on our userbase, could you explain why this is a medium risk uplift? Thanks

At the time I filled this out I put it as medium risk because even though we have automated tests, we had not executed it in Nightly. And it processes strings that are by definition unknown and unexpected. At this point we have gotten some data from Nightly so that's some confidence, but it's still conceivable that if you hit this (very-very-edge-case) code path, an unforeseen format could cause something bad to happen (like a crash.) (To be clear, we should be fine processing any type of string, there's not an expected format we require things to be in - but I'm being extra cautious in my risk assessment.)

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9104948 [details]
Bug 1592321 - Collect a sanitized path for files doing eval() on Windows in the same manner as untrusted modules r?aklotz

Uplift approved for 71 beta 11, thanks.

Comment 15

[Oana Pop-Rus]

https://hg.mozilla.org/releases/mozilla-beta/rev/89525246903f