Closed Bug 1501680 Opened 1 year ago Closed 1 year ago

Cookies sent while SSL report submission on incoming.telemetry.mozilla.org

Categories

(Firefox :: Security, defect, P1)

63 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 65
Tracking Status
firefox-esr60 64+ verified
firefox63 --- wontfix
firefox64 + verified
firefox65 + verified

People

(Reporter: modi.konark, Assigned: johannh)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Attached image cookie-sslreports.png
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36

Steps to reproduce:

1.Opted-in to report SSL errors via Report errors like this to help Mozilla identify and block malicious sites
2. Now all the reports sent to `https://incoming.telemetry.mozilla.org/submit/sslreports/` have cookie attached with it.

EG: https://expired.badssl.com/, this will generate a SSL error report.

(You need to have a .mozilla.org cookie - Try visiting few pages on mozilla.org)


Actual results:

SSl reports have cookie attached to it. Irrespective of normal / private window.
Cookies can have long expiry in this case the expiry of cookie is Oct'2020.

It would be worth checking, if a user has `.mozilla.org` cookie set, which other telemetry signals sends it back. 


Expected results:

No tracking identifier should be sent with any telemetry data.
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:65.0) Gecko/20100101 Firefox/65.0 (20181031100345)

I was able to reproduce the mentioned behavior on OS X 10.13.6 using the latest Nightly and Fx release build. I can see in the Browser Toolbox that the SSL report has a cookie attached to it following the steps. However, I`m not sure what should be the behavior here, so I`m assigning component in order to get an input from the development team. Please change if this is not the right component!
Component: Untriaged → Security
Product: Firefox → Core
Gah, we had that kind of thing before and we need to audit this.
Assignee: nobody → jhofmann
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Product: Core → Firefox
Blocks: 1503960
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/104801e66fb2
Don't send credentials in ssl error reports. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/104801e66fb2
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Comment on attachment 9021912 [details]
Bug 1501680 - Don't send credentials in ssl error reports. r=Gijs

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1241821

User impact if declined: Cookies are sent with SSL error reports, giving us too much identifiable information about the individual user (although these are never stored in the back-end).

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: - Visit mozilla.org
- Visit https://expired.badssl.com/ and check "Report errors like this to help Mozilla identify and block malicious sites"
- Open the Browser Toolbox (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox) and select the network panel.
- Reload https://expired.badssl.com/ and watch the network log for a request to https://incoming.telemetry.mozilla.org/submit/sslreports/
- This request should not have cookies attached

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): A one-line change adding a parameter that is very unlikely to have any other effect (there is no reason why we would need cookies in these reports), plus tests.

String changes made/needed: None
Attachment #9021912 - Flags: approval-mozilla-beta?
I'm kinda wondering if this is worth backporting to ESR60 too since it's a privacy leak and it won't be EOL for another year or so.
Blocks: 1241821
Flags: qe-verify+
Flags: needinfo?(jhofmann)
Flags: in-testsuite+
Comment on attachment 9021912 [details]
Bug 1501680 - Don't send credentials in ssl error reports. r=Gijs

[Triage Comment]
Fixes a privacy issues when sending Telemetry. Approved for 64.0b7.
Attachment #9021912 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0 

This has been verified as fixed on Windows 10 x64 and OS X 10.13.6 using the latest Nightly build from (201811012200580).

Will keep the qe-verify+ flag until the Beta 64.0b7 is released.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> I'm kinda wondering if this is worth backporting to ESR60 too since it's a
> privacy leak and it won't be EOL for another year or so.

I agree and I think it should apply cleanly on ESR, can you try it?
Flags: needinfo?(jhofmann)
Grafts cleanly, go ahead with the uplift request.
Comment on attachment 9021912 [details]
Bug 1501680 - Don't send credentials in ssl error reports. r=Gijs

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Hypothetical user privacy leak (we are not actually using this data in the backend but that's a weak promise of course).

User impact if declined: Users may get their cookies for .mozilla.org leaked to our telemetry servers.

Fix Landed on Version: 65,64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): A one-line change adding a parameter that is very unlikely to have any other effect (there is no reason why we would need cookies in these reports), plus tests.

String or UUID changes made by this patch: None
Attachment #9021912 - Flags: approval-mozilla-esr60?
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0 (20181105164654)

The issue has been verified as fixed on Windows 10 x64 and OS X 10.13.6 using the latest Beta 64.0b7. 

I can confirm no tracking identifier is sent with any telemetry data.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment on attachment 9021912 [details]
Bug 1501680 - Don't send credentials in ssl error reports. r=Gijs

Taking this patch for ESR60 to protect user privacy.
Attachment #9021912 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Backed out from ESR60 for asserts in test_toolkit_securityreporter.js.
https://hg.mozilla.org/releases/mozilla-esr60/rev/4997cf9a50db

I didn't have a chance to look into it, but I'm hoping it just needs a bit of test fix-up.
https://treeherder.mozilla.org/logviewer.html#?job_id=210690727&repo=mozilla-esr60&lineNumber=2200
Flags: needinfo?(jhofmann)
No longer blocks: 1503960
Blocks: 1456924
Attached patch Patch for ESR 60Splinter Review
I think this version of the patch should make the test work in beta.
Flags: needinfo?(jhofmann) → needinfo?(ryanvm)
Beta or ESR60?
Flags: needinfo?(ryanvm) → needinfo?(jhofmann)
Comment on attachment 9025508 [details] [diff] [review]
Patch for ESR 60

Yeah, ESR, sorry, seems like someone has been requesting too many beta uplifts recently. It's like hard-wired into my brain to write this stuff when I attach a rebased patch.
Attachment #9025508 - Attachment description: Patch for beta → Patch for ESR 60
Flags: needinfo?(jhofmann)
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Firefox/60.0 (20181203164059)

The issue has been verified as fixed on Windows 10 x64 and OS X 10.14 using the latest ESR 60.4.0. 

I can confirm no tracking identifier is sent with any telemetry data.
You need to log in before you can comment on or make changes to this bug.