Closed Bug 1522210 Opened 11 months ago Closed 11 months ago

FingerPrinting and Cryptomining classifiers need a separate content log id.

Categories

(Core :: Privacy: Anti-Tracking, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(7 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

At the moment, fingerprinting and cryptomining classifiers use tracking protection content log ID and that makes impossible, for the front end code, to distinguish them.

Blocks: 1522565
Blocks: 1522566

Andrea, we haven't really discussed how fingerprinting/cryptocurrency mining blocking would work in Gecko anywhere, so I'm going to do that here. This may be expanding the scope of this bug a bit, sorry about that. If you feel like we need more bugs to be filed, please feel free to do so.

Looking at UrlClassifierFeatureCryptomining.cpp and UrlClassifierFeatureFingerprinting.cpp I can see several problems with how they currently work.

  • They currently hijack NS_ERROR_TRACKING_URI and also use nsIHttpChannelInternal::CancelForTrackingProtection(). They should stop doing that. We probably want to have a new error code for each of the two features, and cancel the channel for each one with the appropriate error code. You'd need to audit all usages of NS_ERROR_TRACKING_URI and see which ones require similar handling for these two categories, I would expect a minority of them would. LMK if you need help on that.

  • Similar to STATE_LOADED_TRACKING_CONTENT and STATE_BLOCKED_TRACKING_CONTENT you'd need two nsIWebProgressListener content blocking event codes per url-classifier feature. Note that now only these flags need to use distinct bits https://searchfox.org/mozilla-central/rev/c035ee7d3a5cd6913e7143e1bce549ffb4a566ff/uriloader/base/nsIWebProgressListener.idl#289 so there are many new bits freed up for usage. Here some refactoring in url-classifier is also needed, for example UrlClassifierCommon::ShouldEnableClassifier() shouldn't be calling NotifyTrackingProtectionDisabled() directly, maybe it should go through the feature or something?

I would really appreciate if you did this in small incremental patches, since this will be a significant amount of code change which will be hard to review (since it will be easy to miss something somewhere) so keeping each patch focused on a single small functional change would make the review much simpler. :-) Thanks!

Priority: -- → P2

Ehsan, thanks for this comment. This is I was thinking to do. Actually I already tried to introduce 2 separate error code (and 2 sepeate blocking state in nsWebProgressListener) when I was implementing these classifiers. I know that there is a lot of code moving.

Attachment #9039093 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 2 - Canceling nsIChannel with error code, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 3 - Canceling nsIChannel with error code, r?ehsan
Attachment #9039094 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 3 - abstract blocking state codes, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 4 - abstract blocking state codes, r?ehsan
Attachment #9039096 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 4 - fingerprinting, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - fingerprinting, r?ehsan
Attachment #9039097 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 5 - cryptomining, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 8 - cryptomining, r?ehsan
Attachment #9039562 - Attachment is obsolete: true
Attachment #9039096 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - fingerprinting, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - fingerprinting, r?ehsan, r?johannh
Attachment #9039097 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 8 - cryptomining, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - cryptomining, r?ehsan, r?johannh
Attachment #9039092 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 1 - UrlClassifierFeatureFactory::IsClassifierBlockingErrorCode, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 1 - UrlClassifierFeatureFactory::IsClassifierBlockingErrorCode, r=ehsan
Attachment #9039161 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 2 - rename variables in nsHttpChannel, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 2 - rename variables in nsHttpChannel, r=ehsan
Attachment #9039093 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 3 - Canceling nsIChannel with error code, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 3 - Canceling nsIChannel with error code, r=ehsan
Attachment #9039094 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 4 - abstract blocking state codes, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 4 - abstract blocking state codes, r=ehsan
Attachment #9039560 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 5 - generalize content blocking notification, r?ehsan → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 5 - generalize content blocking notification, r=ehsan
Attachment #9039096 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - fingerprinting, r?ehsan, r?johannh → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - fingerprinting, r=ehsan, r?johannh
Attachment #9039097 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - cryptomining, r?ehsan, r?johannh → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - cryptomining, r=ehsan, r?johannh
Attachment #9039096 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - fingerprinting, r=ehsan, r?johannh → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - fingerprinting, r=ehsan, r=johannh
Attachment #9039097 - Attachment description: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - cryptomining, r=ehsan, r?johannh → Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - cryptomining, r=ehsan, r=johannh
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/00494efd99e7
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 1 - UrlClassifierFeatureFactory::IsClassifierBlockingErrorCode, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/1e7503dad26a
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 2 - rename variables in nsHttpChannel, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/c5f891b63342
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 3 - Canceling nsIChannel with error code, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/41bc4923fc47
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 4 - abstract blocking state codes, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/95acf4cd115f
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 5 - generalize content blocking notification, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/8534212acce3
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - fingerprinting, r=Ehsan,johannh
https://hg.mozilla.org/integration/autoland/rev/22fd084b4286
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - cryptomining, r=Ehsan,johannh
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d60130501cfe
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 1 - UrlClassifierFeatureFactory::IsClassifierBlockingErrorCode, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/72bf2b10aec9
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 2 - rename variables in nsHttpChannel, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a74ac91a0c08
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 3 - Canceling nsIChannel with error code, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8899b62062f
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 4 - abstract blocking state codes, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed3cfd2e655
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 5 - generalize content blocking notification, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa151834287d
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - fingerprinting, r=ehsan,johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/1705824aa93e
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - cryptomining, r=ehsan,johannh
Flags: needinfo?(amarchesini)
Component: Safe Browsing → Privacy: Anti-Tracking
Product: Toolkit → Core
Depends on: 1525458
You need to log in before you can comment on or make changes to this bug.