Opened 14 months ago

Closed 14 months ago

Last modified 11 months ago

#6849 closed change (fixed)

Remove redundant logic parsing checksums from filter lists

Reported by: sebastian Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords:
Cc: erikvold, kzar, mjethani, sergz, greiner, hfiguiere Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29855555/

Description (last modified by mjethani)

Background

Filter lists may (optionally) contain a special comment with a checksum, e.g:

! Checksum: 2ZMngqXVdZuxdzEge+OuNQ

This checksum is a Base64-encoded MD5 hash of the UTF8-encoded filter list after converting newlines to UNIX-style and removing the line with the checksum on as well as empty lines.

The Adblock Plus legacy Gecko extension verified the checksum, and rejected the filter list if an invalid checksum was provided. However, in all other versions of Adblock Plus (including the Firefox WebExtension), checksum calculation was never implemented, causing any checksum provided to be ignored.

The usefulness of such a checksum appears rather questionable in the first place. Given TCP and TLS, it seems impossible for the data to corrupt (especially in a way that the checksum would guard against). Even if the request gets intercepted and deliberately modified (which again is rather unlikely with TLS in place), the attacker could just remove (or replace) the checksum. Moreover, the de facto lack of checksum validation, with no apparent impact, seems to proof that it is in fact redundant.

With #6850 we will also stop putting the checksum in filter lists in the first place.

What to change

Remove the logic for parsing and handling checksums from lib/synchronizer.js and the related tests.

Integration notes

When the dependency is updated in adblockpluschrome the noop implementation of Utils.generateChecksum() there should be removed as well.

The synchronize_checksum_mismatch string is no longer relevant.

Hints for testers

Adblock Plus now no longer verifies the checksum in a filter list. If a list does contain a checksum, it is ignored. In order to test this, add a wrong checksum and make sure that nothing happens, i.e. the extension continues on as if it's the right checksum. The loading of filter lists in general should work as it did before this change.

Change History (18)

comment:1 Changed 14 months ago by sebastian

  • Summary changed from Remove filter list checksum validation to Remove redundant logic parsing checksums from filter lists

comment:2 Changed 14 months ago by sebastian

  • Description modified (diff)

comment:3 Changed 14 months ago by sebastian

  • Description modified (diff)

comment:4 Changed 14 months ago by sebastian

  • Description modified (diff)

comment:5 Changed 14 months ago by kzar

  • Cc mjethani added
  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set

comment:6 Changed 14 months ago by mjethani

  • Cc sergz added

comment:7 Changed 14 months ago by greiner

  • Cc greiner added

comment:8 Changed 14 months ago by sebastian

Thomas, note that the string synchronize_checksum_mismatch (in adblockplusui) is no longer relevant and can be removed now. Since checksums are effectively ignored, hence this message is never shown, don't bother to wait until this issue has been completed before removing that string.

comment:9 Changed 14 months ago by hfiguiere

I have updated issue 5146 (emscripten implementation of the downloader) to mention we should not implement the Md5 checksum.

Last edited 14 months ago by hfiguiere (previous) (diff)

comment:10 Changed 14 months ago by mjethani

  • Owner set to mjethani

comment:11 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:12 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:13 Changed 14 months ago by mjethani

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:14 Changed 14 months ago by hfiguiere

  • Cc hfiguiere added

comment:15 Changed 14 months ago by abpbot

A commit referencing this issue has landed:
Issue 6849 - Remove filter list checksum verification

comment:16 Changed 14 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:17 Changed 13 months ago by greiner

For reference, I've created ui#189 to remove the unused error message string.

comment:18 Changed 11 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Adding filter lists still works as expected and any checksum values are ignored.

ABP 3.3.2.2175
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10

Note: See TracTickets for help on using tickets.