220711 - Need to rework checking of URI so that it is not called multiple times

Status: RESOLVED WORKSFORME

(Core :: Security, defect)

Description

[Ian Neal]

Currently, for certain clicks on links, urlSecurityCheck is called more than
once, this should not be necessary.

Comment 1

[Ian Neal]

Attachment: Initial Patch v0.4d

This initial patch removes the call to urlSecurityCheck from the beginning of
handleLinkClick function and adds new calls futher within the function where
urlSecurityCheck would have no longer been called because of the removal.

I'm not sure that we do actually need a call to urlSecurityCheck for the right
click (event.button=2) but I have it in there for the moment.

Comment 2

[Ian Neal]

Attachment: Patch v0.4e - removes duplicate calls to urlSecurityCheck

This patch removes the first call to urlSecurityCheck in handleLinkClick and
adds new calls where a call to an |openNew| function returns false, an
|openNew| function is not called and wrt saveModifier where handleLinkClick is
called with linkNode not being null.

Comment 3

[Ian Neal]

Accepting

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133087 [details] [diff] [review]
Patch v0.4e - removes duplicate calls to urlSecurityCheck

This really needs review from caillon....

Why the "if (linkNode)" check, by the way?

Comment 5

[Ian Neal]

Well I'm pretty sure that I don't need the check when the linkNode is null but
I'm presuming that extensions could call this with linkNode not being null
otherwise why do we still take the linkNode argument in handleLinkClick? Mozilla
itself only calls it with a null for that argument.

Comment 6

[Boris Zbarsky [:bzbarsky]]

My question is, why does it matter whether linkNode is null?  The URL you are
testing is not associated with linkNode at this point....

And yes, linkNode used to be used and then someone tweaked something, I guess.

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Eek, why was I not cc'd?

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 133087 [details] [diff] [review]
Patch v0.4e - removes duplicate calls to urlSecurityCheck

There's just something about this patch that I don't like.  Maybe it's the fact
that now there are so many seemingly random security checks.  Some callers have
them, others don't, etc.

Would it make more sense to require the callers of anything that loads a URL to
do the check before calling the handleLinkClick/doOpenNewFoo methods?

Also note that the security check is relatively cheap, so it doesn't really
hurt anything here.  The performance impact of doing perhaps an extra security
check is negligible, and this bug may not be worth fixing.  It's better to do
too many checks than too few....

Comment 9

[Ian Neal]

Attachment: Patch v0.4f - reworks contentAreaClick function removing duplicate calls to urlSecurityCheck

Patch reworks contentAreaClick function by inling handleLinkClick function and
moving call to urlSecurityCheck into openNewTabOrWindow function where it
hasn't previously been called from openNew* functions.

Need to fix this so that bug 84128 can be fixed as multiple calls to
urlSecurityCheck would cause multiple user alerts.

Comment 10

[Ian Neal]

Comment on attachment 133087 [details] [diff] [review]
Patch v0.4e - removes duplicate calls to urlSecurityCheck

Cancelling previous request

Comment 11

[Ian Neal]

Attachment: Patch v0.5 - makes sure urlSecurityCheck is called

This patch:
* Fixes the duplicate calls to urlSecurityCheck in handleLinkClick
  - instead of checking at start of function only checks if openNewWindowWith
    or openNewTabWith have not been called.
* Fixes the fact that urlSecurityCheck is not called before trying to load the
url in middleMousePaste function if ctrl key is not being pressed

Comment 12

[Ian Neal]

I guess firefox might need a similar middleMousePaste fix.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 163169 [details] [diff] [review]
Patch v0.5 - makes sure urlSecurityCheck is called

This is no longer anything like the current code.

Comment 14

[Daniel Veditz [:dveditz]]

This seems to have effectively been fixed when this function was rewritten at some point.