Incorrect logic in SafeBrowsing V4 Database MergeUpdate
Reported by
contest....@gmail.com,
Nov 13 2017
|
|||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.75 YaBrowser/17.11.0.1452 Yowser/2.5 Safari/537.36 Steps to reproduce the problem: 1. Create some version of SafeBrowsing V4 Database. 2. Send update with removal field equal to [0] (you should remove the smallest prefix) and add this prefix to addition in the same update. 3. ADDITIONS_HAS_EXISTING_PREFIX_FAILURE will return in MergeUpdate. What is the expected behavior? Following by this https://developers.google.com/safe-browsing/v4/local-databases " The client updates the lists in the local database (applying the removals before the additions) and then performs the validation check." So prefix in position 0 should be removed before it will be added to database. After applying removals fields there will be no prefix-existing collision. What went wrong? There is a check for existing same hashes in database and in additions in update In the 578 line: https://chromium.googlesource.com/chromium/src/+/lkgr/components/safe_browsing_db/v4_store.cc#578. But this check should be applied after removing this prefix in 608 line . If client works on v4 protocol. https://chromium.googlesource.com/chromium/src/+/lkgr/components/safe_browsing_db/v4_store.cc#608 Did this work before? N/A Chrome version: 62.0.3202.75 Channel: n/a OS Version: OS X 10.11.6 Flash Version: Shockwave Flash 27.0 r0
,
Nov 13 2017
,
Nov 13 2017
Thanks for reporting this issue. I am inclined to call this working as intended because if the server sends an update that contains a hash prefix in additions as well as removals, then that's a server-side bug and I think it is prudent to not apply such an update. If this does indeed happen, bailing out with that error code would allow us to track such an error and address it server-side quickly.
,
Nov 13 2017
Hello Varun, But this implementation doesn't suits v4 protocol described here - https://developers.google.com/safe-browsing/v4. I think, that one of the following thing should be done: 1. Patch the description of v4 protocol (mention this situation). 2. Apply this patch (some servers could use this case for sending some additional signals for specific clients).
,
Nov 14 2017
Hi There are cases when such update (remove prefix, add prefix) is desirable and not a bug. It does not corrupt local database, the primary database check will pass successfully. Also protocol specification allows it. So it would be nice if client allows such updates too and reject updates only if their format is invalid or result checksum mismatches.
,
Jan 19 2018
Re #c4: Can you please point me to exact place in the documentation where this is discussed. As far as I can tell, this case isn't specifically called out. Re #c5: To ensure that the on-disk database is always reliable, it is better to failsafe and reject an update that's invalid (incorrect format, checksum mismatch, ...) or has unexpected behavior. The case being discussed here is unexpected and it is better to fail in this case, than accept such an update. Finally, if this were happening too frequently, we would have noticed it in the logs we monitor.
,
Jan 19 2018
Re #c6: "The client updates the lists in the local database (applying the removals before the additions) and then performs the validation check" https://developers.google.com/safe-browsing/v4/local-databases. But in this situation, this is done parallel. |
|||
►
Sign in to add a comment |
|||
Comment 1 by contest....@gmail.com
, Nov 13 2017