1029781 - image loaders should set loadFlags | NS_LOAD_CLASSIFY_URI

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

Since images can hit the malware/phishing/tracking lists, they should go through the channel classifier.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Set NS_LOAD_CLASSIFY_URI in image loader

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8445535 [details] [diff] [review]
Set NS_LOAD_CLASSIFY_URI in image loader

Review of attachment 8445535 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Seth,

This change forces images to go through nsChannelClassifier. This is the correct thing to do for phishing and malware checks, but also we especially need it for tracking protection (which will be prefed off by default), since webbugs are often used for tracking. Generally safebrowsing lookups are very fast (<< .1ms) but it would be great to cross check for performance regressions if such metrics exist.

http://telemetry.mozilla.org/#filter=nightly%2F33%2FURLCLASSIFIER_LOOKUP_TIME&aggregates=multiselect-all!Submissions!Mean!5th%20percentile!25th%20percentile!median!75th%20percentile!95th%20percentile&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph

Monica

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a3fb8fc4d29

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4dcd761df9 for being a likely cause of Android xpcshell test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=42467232&tree=Mozilla-Inbound

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Thanks Wes. Working on it.

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Hey gcp, this seems to be breaking on Classifier::Open:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#745

which is using the profile directory. I'm a little confused, because this breakage is on Android only and I thought that safebrowsing worked on Android. Do I need to add do_get_profile() to these tests, or fix #ifdef MOZ_CLASSIFIER somewhere?

Comment 7

[Gian-Carlo Pascutto [:gcp]]

My best bet is that you're causing Classifier to be initialized earlier or be activated in some tests where it wasn't before. So yeah, problems with the profile seem likely.

I see no reason why this would trigger on Android only.

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=c8cab4f14b91

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Green try with do_get_profile(): https://tbpl.mozilla.org/?tree=Try&rev=439bfe4082df

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3192b2f9195

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f3192b2f9195