Closed Bug 1416195 Opened 4 years ago Closed 4 years ago

content.js is breaking and potentially leaking error pages in iframes

Categories

(Firefox :: Security, defect)

58 Branch
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1297630
Tracking Status
firefox58 --- affected

People

(Reporter: johannh, Unassigned)

References

Details

(Keywords: sec-audit)

There are various places where content.js uses the global content or docShell objects without considering that messages may have come from iframes. This breaks some parts of SafeBrowsing and CertError pages in iframes and might be exploited somehow to leak data from these pages.

Examples:
https://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/base/content/content.js#285
https://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/base/content/content.js#164

Fortunately these should not leak anything at the moment, because there are if-conditions like this

https://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/base/content/content.js#153

preventing the functions from continuing on non-error pages.

Instead of relying on those conditions we should really just fix content.js to handle frames correctly.

I'm making this a security sensitive bug because I want to prevent people from getting creative and finding other issues like this or ways to circumvent the existing safeguards. This could be limited to error pages in content.js, but it could also affect other content scripts.
Keywords: sec-audit
See Also: → 1421564
AH! Very timely - so I found bug 1421564 as ckershcb and I are conducting a review of all the semi-privileged context in Firefox, to try to sort out some rhyme or reason, and maybe look for improvements (ping/mail me if you have suggestions on that). 

But i was specifically looking at [1] today and wondered what the implication of using chromeGlobal.addEventListener was. Is that what you are talking about when you say "content.js uses the global content " object? I assume so, and I assume that means that any content can fire a CustomEvent with the specific name and it will hit this handler. Its checks the source of the event so its safe, so I assumed that was that.

I'm really curious to understand what 'fix content.js to handle frames correctly' would entail. 

In the meantime, I'll keep an eye out for this pattern while we audit the rest of the about pages.[2]


[1] https://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/base/content/content.js#237

[2] Im keeping notes here:
https://docs.google.com/document/d/1VWXtN9dBp8BvVOSmmQ_slGXI75zVOpYZN1p5hTYUH48/edit#
The issue here is that the content.js code uses `docShell` and `content` globals off the framescript, which only access the toplevel window/docshell. This is incorrect for cert/network errors that appear in frames.
(In reply to Paul Theriault [:pauljt] from comment #1)
> But i was specifically looking at [1] today and wondered what the
> implication of using chromeGlobal.addEventListener was. Is that what you are
> talking about when you say "content.js uses the global content " object? I
> assume so, and I assume that means that any content can fire a CustomEvent
> with the specific name and it will hit this handler. Its checks the source
> of the event so its safe, so I assumed that was that.

Though I was referring to something else, I think your assumption is right about this piece of code. There doesn't seem to be any other way to do it without giving privileges to AboutNetError.xhtml though, and as you're saying that if statement should be enough.

> I'm really curious to understand what 'fix content.js to handle frames
> correctly' would entail. 

The problem is the use of the "docShell" and "content" globals as seen here:

https://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/base/content/content.js#285
https://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/base/content/content.js#164

That means when a certerror page appears inside an iframe, content.js would try to inject the certerror details into its parent (which is not controlled by us, it can be any site that embedded an iframe with a certerror). The correct way to do it would be to get the ownerDocument of whatever triggered the message and use that instead of the global variables.

It currently doesn't because we have if conditions that prevent it, but I doubt that these if conditions were meant for that, since they don't fix the less critical but annoying problem that the framed certerror page doesn't get the certerror details.

I was briefly glancing over the rest of our content scripts looking for this kind of problem, but it doesn't appear more wide-spread to me.
Ok, I found bug 1297630 which turns out to be an exact dupe of this problem (maybe phrased less dramatically). Considering that I haven't found any way to actually exploit this and there don't seem to be any other obvious places where this problem occurs (only in content.js), I think it's safe to dupe this to the public bug 1297630.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1297630

Removing employee no longer with company from CC list of private bugs.

Group: firefox-core-security
You need to log in before you can comment on or make changes to this bug.