349234 - we need to reduce the number of remote lookups per user

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

We're getting more remote lookups per user than we can handle for a user base the size of firefox's.  We currently check all document loads.  Instead of checking each document load, only checking top level loads (that is, ignore frame loads) is expected to reduce the number of check by an order of magnitude.

Comment 1

[Daniel Veditz [:dveditz]]

Is this really a "security" bug? If it's more of a "business/marketing confidential" bug I can move it into the mozilla.org confidential group.

Actually you should be getting very few lookups from Firefox 2 users, since the remote lookup is turned off by default. Are these lookups coming from the similar feature in the Google Toolbar instead? If so this bug may be in the entirely wrong place.

(I'm not going to disagree that lookups should only be done on the top-level documents, though. That's the right thing whether or not Firefox is the source of the load problem you're getting.)

Comment 2

[Tony Chang (Google)]

You could try to hide a phishing page in a frame.  E.g., suppose you hit a phishing page while browsing in a bitty browser:
http://www.bitty.com/

Comment 3

[Daniel Veditz [:dveditz]]

If the framing site maliciously put phishing stuff in a frame to hide it from you I'd say that makes the framing site bad; it should go on the phishing list, too.

On the accidental side, people who regularly log on to their bank in a bitty-browser (why?) and then one day happen to stumble across a phish for their bank and decide "why not, as long as it's here I'll do my banking now" has got to be a very tiny population. If we potentially leave that crowd at risk to ensure the service stays up for everyone else I'm fine with that.

Comment 4

[Tony Chang (Google)]

Attachment: v1: use onLocationChange instead

This seems to do what we want (ignore (i)frames, only use the final url if there are redirects, fires on background tabs, fires on forward/backward nav and reload, does not fire on tab select).

Comment 5

[Jesse Ruderman]

> only use the final url if there are redirects

That makes me worry.  It gives a phisher a lot of mobility after spamming, for one thing.

Comment 6

[Tony Chang (Google)]

(In reply to comment #5)
> > only use the final url if there are redirects
> 
> That makes me worry.  It gives a phisher a lot of mobility after spamming, for
> one thing.

This is true, but the short life of existing phishing sites suggests that phishers don't do this.  Probably because email filters catch them first.

We could add back in the onStateChange for STATE_IS_NETWORK, which checks the first url, but I don't know a way to prevent it from firing a second time on onLocationChange if it isn't a redirect.

Comment 7

[Tony Chang (Google)]

on trunk

Comment 8

[Daniel Veditz [:dveditz]]

Why is this bug hidden with the security flag? Just an overabundance of paranoia?

I plan on removing the flag next time I stumble across this bug, please add an explanatory comment if I shouldn't.

Comment 9

[Tony Chang (Google)]

(In reply to comment #8)
> Why is this bug hidden with the security flag? Just an overabundance of
> paranoia?
> 
> I plan on removing the flag next time I stumble across this bug, please add an
> explanatory comment if I shouldn't.

Using frames is a way to bypass phishing protection.  Your call as to whether or not that's a security bug.

Comment 10

[Mike Schroepfer]

Comment on attachment 234868 [details] [diff] [review]
v1: use onLocationChange instead

a=schrep/connor for drivers .

Comment 11

[Tony Chang (Google)]

on branch