Create API for web client to process Counter Abuse signals from server [chromium/src : main]
Sarah Krakowiak (Gerrit)
Sarah Krakowiak added 1 comment![]( "Open in Gerrit")
@mcr...@chromium.org: can you PTAL at tools/metrics/histograms/metadata/glic/*
@the...@chromium.org: can you PTAL at components/safe_browsing/*
@car...@chromium.org: can you PTAL at the rest?
Related details
- Carlos Knippschild
- Chromium IPC Reviews
- Michael Crouse
- thefrog
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
Shadow: ari...@chromium.org; IPC: ke...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
Shadow IPC reviewer(s): ari...@chromium.org. Please conduct an IPC review and CR+1 when satisfied. Remember to add the main reviewers to the attention set if needed.
Main IPC reviewer(s): ke...@chromium.org. Please wait for the shadowed IPC reviewer to CR+1 before reviewing.
Shadowed: ari...@chromium.org
Reviewer source(s):
ari...@chromium.org, ke...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Ari Chivukula
- Carlos Knippschild
- Ken Buchanan
- Michael Crouse
- thefrog
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
SafeBrowsingVerdictResult sb_verdict_result;Why not just have ProcessCounterAbuseVerdict take a SafeBrowsingVerdictResult? directly
Related details
- Carlos Knippschild
- Ken Buchanan
- Michael Crouse
- Sarah Krakowiak
- thefrog
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
thefrog (Gerrit)
thefrog added 5 comments![]( "Open in Gerrit")
@xing...@chromium.org - I think components/safe_browsing/content/browser/base_ui_manager.cc LGTM, but could you please review the changes there in case I am missing something?
resource.threat_type = safe_browsing::SBThreatType::SB_THREAT_TYPE_SAFE;Let's avoid trying to a show a blocking page with a resource that has a safe threat type. It will hit a DCHECK here: https://source.chromium.org/chromium/chromium/src/+/main:components/safe_browsing/content/browser/base_blocking_page.cc;l=186-187;drc=cf8d6dbdfa21adefd8e8265cc86998ce2cf1e9da
return CSBRR::SAFE_BROWSING_URL_API_TYPE_UNSPECIFIED;I think we should create a new SafeBrowsingUrlApiType and thread it through here so that CSBRRs are attributable to this new case. WDYT?
case ThreatSource::GLIC_COUNTER_ABUSE:Is there something making this case impossible to hit when the user tries to bypass the interstitial, or will they hit the `NOTREACHED`?
return "from_glic_counter_abuse";Please update `SecurityInterstitialSBWithSourceType` in tools/metrics/histograms/metadata/interstitial/histograms.xml as well. And could you add a LINT check around this method that points to that block?
Related details
- Carlos Knippschild
- Ken Buchanan
- Michael Crouse
- Sarah Krakowiak
- Xinghui Lu
Sarah Krakowiak (Gerrit)
Sarah Krakowiak added 1 comment![]( "Open in Gerrit")
SafeBrowsingVerdictResult sb_verdict_result;Why not just have ProcessCounterAbuseVerdict take a SafeBrowsingVerdictResult? directly
We are anticipating future updates, where we provide additional non-SB backed signals related to counter abuse. In this case, we want a `CounterAbuseVerdict` that includes both `SafeBrowsingVerdictResult` and these other signals.
Related details
- Ari Chivukula
- Carlos Knippschild
- Ken Buchanan
- Michael Crouse
- Xinghui Lu
Xinghui Lu (Gerrit)
Xinghui Lu added 2 comments![]( "Open in Gerrit")
@xing...@chromium.org - I think components/safe_browsing/content/browser/base_ui_manager.cc LGTM, but could you please review the changes there in case I am missing something?
Thanks for adding me! I left a comment inline.
!AsyncCheckTracker::IsMainPageResourceLoadPending(resource) ||I'd suggest baking GLIC_COUNTER_ABUSE handling logic into IsMainPageResourceLoadPending since the fact that GLIC is always async might have other effect other than showing post commit warnings. I think there are two options:
1) (preferred) Set resource.navigation_id from chrome/browser/glic/host/glic_page_handler.cc. Then AsyncCheckTracker can tell that the navigation has completed by checking the navigation_id.
2) If navigation_id cannot be easily obtained from glic_page_handler.cc, add threat_source as an additional param in IsMainPageLoadPendingWithSyncCheck[1] and add the GLIC_COUNTER_ABUSE check there.
Related details
- Ari Chivukula
- Carlos Knippschild
- Ken Buchanan
- Michael Crouse
Sarah Krakowiak (Gerrit)
Sarah Krakowiak added 5 comments![]( "Open in Gerrit")
resource.threat_type = safe_browsing::SBThreatType::SB_THREAT_TYPE_SAFE;Let's avoid trying to a show a blocking page with a resource that has a safe threat type. It will hit a DCHECK here: https://source.chromium.org/chromium/chromium/src/+/main:components/safe_browsing/content/browser/base_blocking_page.cc;l=186-187;drc=cf8d6dbdfa21adefd8e8265cc86998ce2cf1e9da
Done
!AsyncCheckTracker::IsMainPageResourceLoadPending(resource) ||I'd suggest baking GLIC_COUNTER_ABUSE handling logic into IsMainPageResourceLoadPending since the fact that GLIC is always async might have other effect other than showing post commit warnings. I think there are two options:
1) (preferred) Set resource.navigation_id from chrome/browser/glic/host/glic_page_handler.cc. Then AsyncCheckTracker can tell that the navigation has completed by checking the navigation_id.
2) If navigation_id cannot be easily obtained from glic_page_handler.cc, add threat_source as an additional param in IsMainPageLoadPendingWithSyncCheck[1] and add the GLIC_COUNTER_ABUSE check there.
I went with option 2, since I did not see a simple way to obtain navigation_id from `g_browser_process` or `WebContents`.
I think we should create a new SafeBrowsingUrlApiType and thread it through here so that CSBRRs are attributable to this new case. WDYT?
Done
Is there something making this case impossible to hit when the user tries to bypass the interstitial, or will they hit the `NOTREACHED`?
There should be a new bypass event specifically for GLIC counter abuse interstitials, I've updated the CL!
Please update `SecurityInterstitialSBWithSourceType` in tools/metrics/histograms/metadata/interstitial/histograms.xml as well. And could you add a LINT check around this method that points to that block?
- Ari Chivukula
- Carlos Knippschild
- Ken Buchanan
- Michael Crouse
- Xinghui Lu
- thefrog
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sarah Krakowiak (Gerrit)
Sarah Krakowiak added 1 comment![]( "Open in Gerrit")
@cth...@chromium.org: can you PTAL at tools/metrics/histograms/metadata/interstitial/ histograms.xml and components/security_interstitials/core/unsafe_resource.*
Related details
- Ari Chivukula
- Carlos Knippschild
- Chris Thompson
- Ken Buchanan
- Michael Crouse
- Xinghui Lu
- thefrog
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |