Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #483 - Add Safe-Browsing via Proxy and Update DB List #1339

Closed
wants to merge 48 commits into from

Conversation

Brandon-T
Copy link
Collaborator

@Brandon-T Brandon-T commented Aug 7, 2019

  • Security Review: https://github.com/brave/internal/issues/632

  • Added Safe-Browsing API Client.

  • Fixes Update error/warning page communication and iconography & use Google Safe Browsing via proxy #483

  • Updated to have CoreData and to use the same Database Schema as Go-Lang server.

  • Also uses same logic as the Go-Lang sblookup server so should be good for searching the DB for hashes (short and long).

  • Host Prefix/Suffix calculation should be done via a TRIE structure but the current algorithm works and matches the Go-Lang implementation's results perfectly.

  • Now handles full-update and partial-update.

  • DB is stored persistently.

  • Handles DB corruption via Checksum

  • Rewrote/Ported a lot from Go-Lang's implementation (took almost 32 hours straight on top of our existing previous PR-Code) and is feature complete.

  • Now handles Database Expiration

  • Now handles Back-Off-Mode

  • Now handles Find and Update wait duration

  • Now schedules updates periodically

  • Now handles Cache and Expiration

  • Now Handles IP-Address Canonicalization

Spec followed is: https://developers.google.com/safe-browsing/v4/update-api

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • My patch or PR title has a standard commit message that looks like Fix #123: This fixes the shattered coffee cup! (or No Bug: <message> if no relevant ticket)
  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New files have MPL-2.0 license header.

Test Plan:

Screenshots:

Reviewer Checklist:

  • PR is linked to an issue via Zenhub.
  • Issues are assigned to at least one epic.
  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable)

@jumde
Copy link
Contributor

jumde commented Aug 8, 2019

  • Similar to desktop, this feature should have a toggle in settings to turn it off.

@jumde
Copy link
Contributor

jumde commented Aug 8, 2019

@Brandon-T - I'm seeing a crash on fullUpdate

(lldb) bt 5
* thread #3, queue = 'NSOperationQueue 0x280a5dea0 (QOS: UNSPECIFIED)', stop reason = EXC_BREAKPOINT (code=1, subcode=0x21d88a504)
    frame #0: 0x000000021d88a504 CoreData`+[NSManagedObjectContext __Multithreading_Violation_AllThatIsLeftToUsIsHonor__]
    frame #1: 0x000000021d75ae6c CoreData`-[NSManagedObjectContext executeFetchRequest:error:] + 952
    frame #2: 0x000000024869d5c8 libswiftCoreData.dylib`(extension in CoreData):__C.NSManagedObjectContext.fetch<A where A: __C.NSFetchRequestResult>(__C.NSFetchRequest<A>) throws -> Swift.Array<A> + 72
  * frame #3: 0x0000000104bda6f8 Client`closure #1 in SafeBrowsingDatabase.update(response=Client.ListUpdateResponse @ 0x000000016ba3a1d0, self=0x0000000281fdb140, completion=0x000000010498e404 Client`closure #1 (Swift.Optional<Swift.Error>) -> () in closure #1 (Swift.Optional<Client.FetchResponse>, Swift.Optional<Swift.Error>) -> () in Client.SafeBrowsingClient.fetch((Swift.Optional<Swift.Error>) -> ()) -> () at GSafeBrowsing.swift:113) at GSafeBrowsingDatabase.swift:154:35
    frame #4: 0x0000000104bdda5c Client`thunk for @callee_guaranteed (@guaranteed ListUpdateResponse) -> (@error @owned Error) at <compiler-generated>:0
(lldb) 

@Brandon-T
Copy link
Collaborator Author

This should be fixed already :D

@Brandon-T Brandon-T force-pushed the feature/SafeBrowsing branch 7 times, most recently from 9b53a6c to 2cffb10 Compare August 15, 2019 15:02
@jumde jumde added the blocked: needs design Needs design before work can commence label Aug 15, 2019
@jumde
Copy link
Contributor

jumde commented Aug 15, 2019

cc: @jamesmudgett for designs of the warning pages

@jumde
Copy link
Contributor

jumde commented Aug 15, 2019

@Brandon-T - Globally disabling Block Phishing and Tracking should disable Safe Browsing. But if Block Phishing and Tracking is globally disabled but Block Phishing is enabled in site shields the blocking still works.

@jumde
Copy link
Contributor

jumde commented Aug 15, 2019

I think we should clearly highlight that we are using Google Safe Browsing here. May be add a line after Block Phishing & Malware cc: @tomlowenthal

@jumde
Copy link
Contributor

jumde commented Aug 15, 2019

Two URLs on the test page are not getting flagged as malicious, see IOS/OSX Warnings on test safebrowsing.appspot.com:

@Brandon-T
Copy link
Collaborator Author

Brandon-T commented Aug 15, 2019

Two URLs on the test page are not getting flagged as malicious, see IOS/OSX Warnings on test safebrowsing.appspot.com:

I checked these two URLs against Google’s Go-Lang implementation, it doesn’t get flagged either. So I decided to test it out on the cloud with my personal API key using the Google Developer Console and it also doesn’t flag these two URLsso I’m not sure what’s going on. There’s also a stackoverflow issue for this.

I’ll see if I can figure out why. It seems to flag it in safari but safari seems to be flagging the entire domain and not just that page. For GSafe v2 it flags but v4 doesn’t. I’ll see if I can figure out why.

@jumde
Copy link
Contributor

jumde commented Aug 15, 2019

@Brandon-T - Safari blocks this URL correctly on iOS. Social Engineering one does not work though. May be worth following up with the Safe Browsing team. cc: @tomlowenthal can help.

This is not a blocker, I think we can file a separate issue for this if needed.

// Technically this should be done "TRIE" data structure

//TODO: Fix for IP Address..
if let hostName = host?.replacingOccurrences(of: "\(scheme ?? "")://", with: "") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit test for this will help

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, let's make sure we stop on ETLD+1 so that we never lookup co.uk for example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use a whitelist of schemes here (probably: http://, https://, ftp://, ws:// , and wss://) since Safe Browsing doesn't make sense for file:// or data: URIs.

@iccub
Copy link
Contributor

iccub commented Oct 17, 2021

closing, error pages design will be updated here #4338

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update error/warning page communication and iconography & use Google Safe Browsing via proxy
7 participants