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)
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.
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: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
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.
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.
ABP 3.3.2.2175
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10
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.