1575974 - Neuter pingsender.exe arbitrary file/destination capabilities

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

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

pingsender.exe can be used to send an arbitrary file to an arbitrary destination. This makes it an attractive application to be used by an attacker inside a network to perform exfiltration because the binary is signed by Mozilla and used in normal operation.

(This technique is often called 'living off the land', more details/examples here: https://github.com/LOLBAS-Project/LOLBAS )

It feels like the simplest way to neuter this capability is to hard-code acceptable endpoints into the binary. It can still receive the target on the command-line - just validate it against a hardcoded list (which can contain test uri's, etc) As long as the list is only composed of publicly registered domain names (no intranet names) we've registered that point to public IP addresses (no internal IP addresses) - I believe that'd neuter it effectively.

We have an ask to generate such a list in the short term for threat detection purposes; once we have it - if we agree that the list is the appropriate course of action - it wouldn't be hard to hard-code it.

gfritzsche is not accepting needinfos... chutten, could you help us figure out how to find the endpoints, and who could make a decision on hardcoding the list?

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Off the top of my head all pings in a production builds should go to incoming.telemetry.mozilla.org. For testing we use localhost and maybe something else too. As for the files telemetry pings should come from the 'saved-telemetry-pings' folder under the user's profile directory. Crash pings sent from the crashreporter client come from the 'Pending Pings' folder under the user-level Firefox directory (~/.firefox/Pending Pings on Linux, %USERPROFILE%/AppData/Roaming/Mozilla/Firefox/Pending Pings on Windows and ~/Library/Application Support/Firefox/Pending Pings on macOS).

Comment 2

[Chris H-C :chutten]

(gfritzsche is on leave until January-ish)

ni?mreid for confirmation that the ping endpoint for Telemetry is and will continue to be https://incoming.telemetry.mozilla.org

To my knowledge, though we support setting a toolkit.telemetry.server to a value other than the default of incoming.tmo, no one really does. The configuration I've seen is usually limited to "let Telemetry go to Moz" or "No Telemetry Ever".

Also, since pingsender is a "best effort" utility, even if we do want to support configurable endpoints we can still restrict pingsender to incoming.tmo and localhost and have everything else fail. Firefox will send any pings that pingsender refused to send the next time it's opened.

As for who can make a decision, I feel comfortable being that person once I hear from the pipeline side (mreid, and whoever he chooses to bring into the conversation). What's proposed won't forbid custom Telemetry endpoints, just make them have slower data.

:gsvelto, about using "maybe something else too" as a target for testing... could you expand on that a little?

Comment 3

[Jonathan Claudius [:claudijd] (use NEEDINFO)]

+ryan (the person who originally raised this concern, to ride along with us on this)

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

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

:gsvelto, about using "maybe something else too" as a target for testing... could you expand on that a little?

I just checked, we only use localhost for testing and AFAIK that's what we also use to test manually (e.g. when using gzipServer to intercept the pings). The only platform that I know of where this might be different is Android but we don't use pingsender there so we don't care.

Comment 5

[Ryan Merritt]

(In reply to Jonathan Claudius [:claudijd] (use NEEDINFO) from comment #3)

+ryan (the person who originally raised this concern, to ride along with us on this)

Thanks Jon.

Restricting the destination to localhost and hardcoding incoming.telemetry.mozilla.org as the allowed default destinations should address the concerns for potential abuse. With the hardcoded destination in place, to redirect network traffic an attacker would need to setup a local proxy, modify the hosts file, or otherwise conduct MiTM attacks that would hopefully raise flags on their own accord. Thanks!

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

Sifting through our code it seems that the telemetry URL is scattered across multiple places. We might consider defining it in only one place via a build configuration variable so that all its hardcoded values can still be changed before a build.

Comment 7

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

Attachment: Bug 1575974 - Restrict pingsender to an allowlist of destination hosts r?gsvelto

Comment 8

[Chris H-C :chutten]

(triage stuffs)

Comment 9

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

I wrote a test for this; but it fails for me locally because I don't have the necessary libcurl functions, and I wrote it to fail open in that case. If we wanted to have it fail closed; we could write a test for it; but that could cause pingsender to stop working on a lot of systems...

Comment 10

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

Attachment: Bug 1575974 - Add tests for pingsender url parsing r?gsvelto

This is ugly. We need the exit code from pingsender to know if we were
allowed to send the ping to the given destination. But the test http
server only sends responses when you call promiseNextRequest or similar
causing pingsender to hang if called in blocking mode.

So we call it in non-blocking mode, but return a promise that yields execution
(using settimeout) so the http server can respond. Then we grab the exit code
after a second for comparison purposes.

Depends on D44046

Comment 11

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

Attachment: Bug 1575974 - Report any non-zero exit code as a process failure in nsIProcess r?gsvelto

EXIT_FAILURE is 'implementation defined' but can be defined to be 1.
In our case, pingsender exits with EXIT_FAILURE but nsIProcess wasn't
reporting it as failure because it thought failures were always negative.

Depends on D44706

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d36c7091cc29b24ec560f7f85d6d281bcd3ba6d show failures in test_crash_service.js - I tracked that down. :gsvelto could you re-r+ the latest change to the non-zero exit code patch?

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed:
https://hg.mozilla.org/integration/autoland/rev/1dc0b5a41dc8a112955dc5c1640eb6511f2f9b86
https://hg.mozilla.org/integration/autoland/rev/225d858fe3351bbd37841fe29d22c5f3a8d97b2f
https://hg.mozilla.org/integration/autoland/rev/cc4a002262292311a9f92a89bbd22775ac084c25

Backed out for ESlint and MinGW debug compilation failures: https://hg.mozilla.org/integration/autoland/rev/e3fac96f56d3e67d5a7e63260d820209dbbc1348

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=265937673&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Cretry%2Csuperseded&revision=cc4a002262292311a9f92a89bbd22775ac084c25

Failure log eslint: https://treeherder.mozilla.org/logviewer.html#?job_id=265937673&repo=autoland
Failure log MinGW debug compilation: https://treeherder.mozilla.org/logviewer.html#?job_id=265937634&repo=autoland
builds/worker/workspace/build/src/toolkit/components/telemetry/pingsender/pingsender_win.cpp:65:66: error: member reference base type 'char [256]' is not a structure or union

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed:
https://hg.mozilla.org/integration/autoland/rev/521655c73cd1f418b8732c47b9821661ccfb303b
https://hg.mozilla.org/integration/autoland/rev/a9512bb19ea7f620e31a293a1a11ed97d44c526d
https://hg.mozilla.org/integration/autoland/rev/e32925d2b886b71bd8ae31617dc5005b473cc35d
https://hg.mozilla.org/integration/autoland/rev/44164f4b9d6d3e1e2a19130b9cf5387604252ae6

Backed out for xpcshell perma fails on test_nsIProcess.js:
https://hg.mozilla.org/integration/autoland/rev/c0b71b2b56a6e7a950ae925e675dc1c56ce3714d
https://hg.mozilla.org/integration/autoland/rev/b7df42acd649c0b2226d734606b1dce2b0c4edc1

Follow-up push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=265990719&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Cretry%2Csuperseded&revision=44164f4b9d6d3e1e2a19130b9cf5387604252ae6
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=265988462&repo=autoland
[task 2019-09-10T21:12:13.182Z] 21:12:13 INFO - TEST-PASS | xpcom/tests/unit/test_nsIProcess.js | observe - [observe : 150] "process-finished" == "process-finished"
[task 2019-09-10T21:12:13.182Z] 21:12:13 INFO - TEST-PASS | xpcom/tests/unit/test_nsIProcess.js | observe - [observe : 151] 0 == 0
[task 2019-09-10T21:12:13.182Z] 21:12:13 WARNING - TEST-UNEXPECTED-FAIL | xpcom/tests/unit/test_nsIProcess.js | observe - [observe : 167] "process-failed" == "process-finished"

Comment 15

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

Alright this should be fixed up in https://treeherder.mozilla.org/#/jobs?repo=try&revision=b69db0ba02d64e4316874d732b67706e3f01bad6

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/09b3abc31caf16819587342c7353d3d78b2346c5
https://hg.mozilla.org/integration/autoland/rev/7220b21e357b790f3acebc2c555450c69e81ef12
https://hg.mozilla.org/integration/autoland/rev/a6d48d36b831296f3caaf301caa0164c28dca6f6
https://hg.mozilla.org/mozilla-central/rev/09b3abc31caf
https://hg.mozilla.org/mozilla-central/rev/7220b21e357b
https://hg.mozilla.org/mozilla-central/rev/a6d48d36b831

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider backporting, especially given where we are in the current ESR cycle?

Comment 18

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

There's a case for putting it in ESR for enterprise... but I guess I'd like to let it ride the trains just in case there was something unexpected that affects telemetry negatively...

Comment 19

[Ryan VanderMeulen [:RyanVM]]

OK, I'm going to mark it for ESR tracking for the 71 cycle and we can revisit in a couple months.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Hey Tom, we're about halfway through the Fx71/68.3esr cycle now - any thoughts on uplifting this to ESR68?

Comment 21

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)

Hey Tom, we're about halfway through the Fx71/68.3esr cycle now - any thoughts on uplifting this to ESR68?

We should do that, this is an important fix in the context of enterprise deployments and those are (mostly?) going to be ESR.

Comment 22

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

Comment on attachment 9089203 [details]
Bug 1575974 - Restrict pingsender to an allowlist of destination hosts r?gsvelto

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: An important fix in the context of enterprise deployments and those are (mostly?) going to be ESR.
• User impact if declined: Enterprises will be sad. Or at least the one who reached out to us.
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I'm told that even if pingsender doesn't work, we still submit telemetry in a different manner. So low.
• String or UUID changes made by this patch:

Comment 23

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

Everything applies cleanly, let's do it.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9089203 [details]
Bug 1575974 - Restrict pingsender to an allowlist of destination hosts r?gsvelto

Approved for 68.3esr.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/5dfbcf9e2c93
https://hg.mozilla.org/releases/mozilla-esr68/rev/adf225ac6141
https://hg.mozilla.org/releases/mozilla-esr68/rev/a3a7c1ecb806