467514 - Meaningless type/context in nsIContentPolicy.shouldLoad when on link click in a frame

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Wladimir Palant]

Attachment: Testcase

My understanding of TYPE_DOCUMENT and TYPE_SUBDOCUMENT in nsIContentPolicy is that the former is meant for documents loaded at top level whereas the second is for documents loaded in a frame. Unfortunately, the actual usage of these types is very different when the target attribute is added. For example, a click on a link in a frame will always trigger content policies with TYPE_SUBDOCUMENT and this frame as context - even if it opens in a new window at top level due to target="_blank" (see testcase).

Am I making wrong assumptions here and the context for a document isn't supposed to be the frame they will be loaded into? In Adblock Plus, I need a way to reliably distinguish top-level documents and frames. Also, I need a useful context that actually tells me where the document will be loaded. Both are things that I cannot get with the current implementation. I see at least one way how the implementation can be changed: check load target first and trigger content policies with TYPE_SUBDOCUMENT/load target if one exists or TYPE_DOCUMENT/null if a new window needs to be opened (triggering content policies will have to move inside DocShell). Does that make sense?

Comment 1

[Wladimir Palant]

Note that this issue affects NoScript as well - from what I can see, several code paths expect to receive the correct frame as load context (including Clickjacking protection).

Comment 2

[Giorgio Maone [:ma1]]

Wladimir, thanks for CCing me and for your proposal, which indeed would make things a lot easier. Many thanks also for bug 467520, which is obviously very useful for me.

NoScript doesn't use content policies at all in its ClearClick anti-clickjacking module: it always get the correct frame from the triggering DOM event.

In early versions of NoScript, IFRAME and FRAME blocking were affected by this issue, even if not in a security-sensitive way because the code was quite conservative on that side: the major inconvenience was that links opened by a certain frame and targeted to a different frame or to a top-level window caused the request to be cancelled and the placeholder to cover the origin frame, rather than the target, which was quite confusing indeed.

Recent NoScript versions work around this problem by partially deferring subdocument checks to a later stage, when the request is already targeted, using a progress listener and tracking the origin context info originally acquired in shouldLoad(). NoScript have been done a similar thing against bug 431782 for a long time, too.

Of course these are ugly albeit effective hacks, and fixing both bugs would be desirable.

Comment 3

[Wladimir Palant]

I was actually referring to this code in your main content policy:

> if (aContext instanceof CI.nsIDOMElement) {
>   // this is alternate to what we do in countObject, since we can't get there
>     this.delayExec(this.opaqueIfNeeded, 0, aContext);
> }

But maybe that's just leftover and not really used.

Comment 4

[Giorgio Maone [:ma1]]

Ah, that opaqueIfNeeded() call is there just to push early opacity on iframes
during parent load rather than waiting for the page to finish loading, or to
catch post-load new frames. Otherwise a clickjacking attacker could emit a
transparent iframe, then block the main response (e.g. by sleeping on the server
side or requesting a big resource somewhere from a script element) waiting for
the attack to succeed.

opaqueIfNeeded() is unaffected because the "need" is based on the location of
the top level window, rather than other content policy info, and this is an
invariant for our purposes.

However, as you guessed, the opacity stuff is kind of a leftover, i.e. it's
been almost obsoleted by ClearClick and left there as an auxiliary
countermeasure.

Comment 5

[Wladimir Palant]

Right, that opaqueIfNeeded() call isn't working anyway (I tested) because it is in the TYPE_DOCUMENT branch and not TYPE_SUBDOCUMENT.

Comment 6

[Giorgio Maone [:ma1]]

(In reply to comment #5)
> Right, that opaqueIfNeeded() call isn't working anyway (I tested) because it is
> in the TYPE_DOCUMENT branch and not TYPE_SUBDOCUMENT.

Weird. I must check what's happening, because my TYPE_SUBDOCUMENT "branch" should cascade to TYPE_DOCUMENT (there's no break statement on purpose, because almost all the policies which applies to top-level documents do apply also to subdocuments, quite obviously).

Comment 7

[Wladimir Palant]

This issue appears to be gone in Firefox 28.