304848 - Extra QIing to get the value of isSubDocumentRelevant

Status: RESOLVED DUPLICATE of bug 832834

(Core :: Security: PSM, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

It looks like the isSubDocumentRelevant boolean is only checked in one spot in
nsSecureBrowserUIImpl::OnStateChange, but is always set.  Setting it requires a
QI cascade, which is expensive.  Is there a reason not to move this code to the
one case when the boolean's value is needed?

That said, why do we even have this boolean?  Why shouldn't a page become
insecure if it has a subframe loaded via some random insecure protocol?  At the
moment it looks like we only do this for http/file/wyciwyg/ftp...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Could someone maybe answer my questions from comment 0 paragraph 2?  That looks
like a security bug to me, basically...

Comment 2

[Kai Engert [:KaiE:]]

The code was introduced by me, when I tried to make the (completely broken)
original lock icon tracking better.

And I think the best thing to do were to work on bug 62178, and follow my
suggestions there, to incorporate the lock icon tracking directly into document
loading, and not have it rely on some after-the-fact notifications.

But I should address your question.

There is only one explanation why I could have added the whitelist of protocols:

For SSL pages, there were progress events delivered to
nsSecureBrowserUIImpl::OnStateChange, where the delivered request was
- either not QIable to nsIChannel
- or the associated nsIChannel did not carry the state from the SSL session,
causing UpdateSubrequestMembers to update the lock icon to insecure

I do not remember the type of the disturbing request objects.

Maybe a whitelist of disturbing objects, that should be filtered out for
security tracking, is more reasonable than the current whitelist of protocols.

If you think it is reasonable to improve that logic (because working on the
suggested less indirect security tracking is too far away), I suggest to do
testing and logging, in order to come up with a list of disturbing objects.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> - either not QIable to nsIChannel

These should just be ignored, imo.

> - or the associated nsIChannel did not carry the state from the SSL session,

Sounds like reflow and parser requests.  We've already removed the former and
are working on removing the latter; should I revisit this bug at that point?

In any case, I just filed this because it was showing up in profiles; if fixing
bug 62178 will fix this as well, we should just do that.

Comment 4

[Kai Engert [:KaiE:]]

The proposed patch for bug 62178 removes the code to QI for isSubDocumentRelevant decision.

Only requests having name "about:layout-dummy-request" are continue to be filtered. (Not sure we still get those, that part of the patch was coded 2 years ago).

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Only requests having name "about:layout-dummy-request" are continue to be
> filtered.

The only such request left is the parser request and that's slated for the chopping block.

What about "about:document-onload-blocker"?  Should security code be ignoring that?  It's used in much the way that about:layout-dummy-request was....

Comment 6

[Kai Engert [:KaiE:]]

> What about "about:document-onload-blocker"?

I have added ignoring these state events to the patch in bug 62178.


> It's used in much the way that about:layout-dummy-request was....

Was? I still get them on the trunk, with seamonkey.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It looks like the parser request still claims to be about:layout-dummy-request.  We have a bug to remove that; I should just do it.

Comment 8

[Boris Zbarsky [:bzbarsky]]

We no longer have any about:layout-dummy-request requests.

Comment 9

[Kai Engert [:KaiE:]]

reassign bug owner.
mass-update-kaie-20120918

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

This seems like something we should (and now can) do.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

This was removed by bug 832834.