Closed Bug 1070251 Opened 5 years ago Closed 5 years ago

Anonymization does not anonymize inProcessTabChildGlobal URLs

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox33 --- fixed
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: khuey, Assigned: njn)

References

Details

(Keywords: privacy)

Attachments

(2 files)

And if you construct an in-process <iframe mozbrowser> you will get some interesting URLs in there.
And it would be nice if the anonymization tests told you what was failing ....
Thank you for the report. I'll take a look on Monday.
Assignee: nobody → n.nethercote
BTW, how did you find this? Comment 1 make it sound like it was via the mochitest...
I got exceedingly lucky (or unlucky, depending on your point of view) and added a leak until shutdown that entrains an inProcessTabChildGlobal in a mochitest-chrome test that runs before the about:memory tests.  Figuring out what was going on was fun.

https://bugzilla.mozilla.org/show_bug.cgi?id=1051995#c29
The problem is in GetCompartmentName. If it's a non-system compartment, we anonymize the name. But if it's a system compartment, all we do is strip out the paths in any file: URLs. So basically, the problem is that content URLs are ending up in system compartment names.

khuey, how do I reproduce this? I tried this but it didn't cause the URL to show up in about:memory:

> <!DOCTYPE html>
> <html><body>
> <iframe src="http://mozilla.org" mozbrowser>
> </body></html>
Flags: needinfo?(khuey)
Try setting dom.mozBrowserFramesEnabled to true?
Flags: needinfo?(khuey)
So I can reproduce by applying the patch from bug 1051995 and running dom/tests/mochitest/localstorage/test_clear_browser_data.html. The relevant entry is this one:

> ├────1 (00.43%) ── [System Principal], inProcessTabChildGlobal?ownedBy=http://www.example.com/chrome/dom/tests/mochitest/localstorage/frame_clear_browser_data.html

Probably the safest thing to do would be the following.

- In GetCompartmentName, look for "inProcessTabChildGlobal?ownedBy=".

- If it's present, look at what follows.

- If what follows is a "chrome:" URI, leave it; otherwise, anonymize it.

That will cause us to unnecessarily anonymize entries like this one:

> ├────1 (00.42%) ── [System Principal], inProcessTabChildGlobal?ownedBy=data:application/vnd.mozilla.xul+xml;charset=utf-8,<window%20id='win'/>

but that doesn't seem like a big loss and it's better to be conservative here.

I don't know if this will be testable.
Even better steps to reproduce:

> mach mochitest --keep-open dom/tests/mochitest/localstorage/test_clear_browser_data.html toolkit/components/aboutmemory/tests/test_memoryReporters.xul
Kind of gross. But the test now tells you which memory report path triggered a failure, which is nice.
Attachment #8493511 - Flags: review?(khuey)
Comment on attachment 8493511 [details] [diff] [review]
Anonymize non-chrome inProcessTabChildGlobal URLs in memory reports when necessary

Review of attachment 8493511 [details] [diff] [review]:
-----------------------------------------------------------------

You probably should add a test for this issue ...

::: toolkit/components/aboutmemory/tests/test_memoryReporters.xul
@@ +112,5 @@
>      }
>  
>      // Shouldn't get any anonymized paths.
>      if (aPath.contains('<anonymized')) {
> +        present.anonymizedWhenUnnecessary = aPath;

I don't know why you don't just ok(false, ...) here but ok.
Attachment #8493511 - Flags: review?(khuey) → review+
> You probably should add a test for this issue ...

I'd love to. How?
Comment on attachment 8495081 [details] [diff] [review]
For backporting to Aurora and Beta

Approval Request Comment
[Feature/regressing bug #]: Bug 1010064 added memory report dumps to OOM crashes. To avoid the exposure of sensitive data (such as the URLs viewed at crash time) bug 1010064 added the ability to anonymize memory report dumps. But I missed an obscure case, which this patch fixed.

[User impact if declined]: it's unlikely but possible that privacy-sensitive data (one or more of the current URLs open) would be submitted in an OOM crash report.

[Describe test coverage new/current, TBPL]: The memory reporter tests were updated.

[Risks and why]: Negligible. The change is very simple.

[String/UUID change made/needed]: None.
Attachment #8495081 - Flags: approval-mozilla-beta?
Attachment #8495081 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/9d59c9841895
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Attachment #8495081 - Flags: approval-mozilla-beta?
Attachment #8495081 - Flags: approval-mozilla-beta+
Attachment #8495081 - Flags: approval-mozilla-aurora?
Attachment #8495081 - Flags: approval-mozilla-aurora+
Flags: in-testsuite+
Thank you for merging, RyanVM.
You need to log in before you can comment on or make changes to this bug.