Opened on 08/09/2018 at 08:39:25 PM

Closed on 08/14/2018 at 05:36:51 PM

Last modified on 10/23/2018 at 11:58:16 AM

#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):

Description (last modified by mjethani)


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.

Attachments (0)

Change History (18)

comment:1 Changed on 08/09/2018 at 08:45:01 PM by sebastian

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

comment:2 Changed on 08/09/2018 at 09:21:17 PM by sebastian

  • Description modified (diff)

comment:3 Changed on 08/09/2018 at 09:35:14 PM by sebastian

  • Description modified (diff)

comment:4 Changed on 08/09/2018 at 09:41:26 PM by sebastian

  • Description modified (diff)

comment:5 Changed on 08/10/2018 at 09:11:11 AM by kzar

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

comment:6 Changed on 08/10/2018 at 09:34:35 AM by mjethani

  • Cc sergz added

comment:7 Changed on 08/10/2018 at 04:08:27 PM by greiner

  • Cc greiner added

comment:8 Changed on 08/10/2018 at 08:10:23 PM 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 on 08/14/2018 at 03:43:43 AM by hfiguiere

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

Last edited on 08/14/2018 at 03:44:11 AM by hfiguiere

comment:10 Changed on 08/14/2018 at 09:21:21 AM by mjethani

  • Owner set to mjethani

comment:11 Changed on 08/14/2018 at 09:22:15 AM by mjethani

  • Description modified (diff)

comment:12 Changed on 08/14/2018 at 09:22:27 AM by mjethani

  • Description modified (diff)

comment:13 Changed on 08/14/2018 at 09:28:08 AM by mjethani

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

comment:14 Changed on 08/14/2018 at 05:26:21 PM by hfiguiere

  • Cc hfiguiere added

comment:15 Changed on 08/14/2018 at 05:31:59 PM by abpbot

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

comment:16 Changed on 08/14/2018 at 05:36:51 PM by mjethani

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

comment:17 Changed on 09/06/2018 at 04:30:40 PM by greiner

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

comment:18 Changed on 10/23/2018 at 11:58:16 AM 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.

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

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from mjethani.
Note: See TracTickets for help on using tickets.