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. |
Xinghui Lu (Gerrit)
Xinghui Lu added 1 comment![]( "Open in Gerrit")
!AsyncCheckTracker::IsMainPageResourceLoadPending(resource) ||Sarah KrakowiakI'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`.
- Ari Chivukula
- Carlos Knippschild
- Chris Thompson
- Ken Buchanan
- Michael Crouse
- Sarah Krakowiak
- 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. |
thefrog (Gerrit)
thefrog added 3 comments![]( "Open in Gerrit")
safe_browsing::ThreatSource::UNKNOWN);Could we avoid the default parameter here? That way it's explicit which callers are affected. Then we can double check there's no need to thread anything through further.
GLIC = 10;Thanks for adding this! Reminder to please update the server side as well in a separate CL (see [go/pqrrc](http://go/pqrrc)).
safe_browsing::ThreatSource::UNKNOWN);Same request here -- can we avoid the default parameter?
Related details
- Ari Chivukula
- Carlos Knippschild
- Chris Thompson
- Ken Buchanan
- Michael Crouse
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 3 comments![]( "Open in Gerrit")
Could we avoid the default parameter here? That way it's explicit which callers are affected. Then we can double check there's no need to thread anything through further.
Done
Thanks for adding this! Reminder to please update the server side as well in a separate CL (see [go/pqrrc](http://go/pqrrc)).
Done - cl/919706102
Same request here -- can we avoid the default parameter?
- Ari Chivukula
- Carlos Knippschild
- Chris Thompson
- Ken Buchanan
- Michael Crouse
- 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")
@rohi...@chromium.org: can you PTAL at ios/chrome/browser/safe_browsing/model/safe_browsing_blocking_page.mm
Related details
- Ari Chivukula
- Carlos Knippschild
- Chris Thompson
- Ken Buchanan
- Michael Crouse
- Rohit Rao
- 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. |
thefrog (Gerrit)
thefrog voted and added 3 comments![]( "Open in Gerrit")
Votes added by thefrog
| Code-Review | +1 |
3 comments
safe_browsing LGTM once comments are addressed. Thanks!
safe_browsing::ThreatSource::UNKNOWN);This feedback is for a potential followup CL -- this does not block the current CL. Feel free to Ack.
There might be a bug hiding here for NOTIFICATION_PERMISSION_ACCEPTED CSBRRs. Before and after this change, the underlying IsMainPageLoadPendingWithSyncCheck call will return true, which means it will try to check the pending navigation instead of the last committed navigation, and then since there isn't actually a pending navigation, the allowlist check will [only check](https://source.chromium.org/chromium/chromium/src/+/main:components/safe_browsing/content/browser/base_ui_manager.cc;l=290-292;drc=86fd797e010ae2e2e49cc2e358842d4e90e54feb) the main URL rather than also the redirect chain. If this is all accurate, then this bug would lead to missing CSBRRs because allowlisted URLs in the redirect chain wouldn't be found.
If this bug is real, I recommend (in a followup CL):
1. Creating a new ThreatSource for the notification permission accepted use case and passing it through here instead of unknown, then modifying IsMainPageLoadPendingWithSyncCheck to return false in that case.
2. Renaming this method IsURLAllowlisted to be more specific to notification permission accepted since we're passing through the phishing threat type (and would be passing a notification permission accepted threat source).
TEST_F(AsyncCheckTrackerTest, IsMainPageLoadPending_NoNavigationId) {Please add a new test case similar to this one (maybe called `IsMainPageLoadPending_GlicThreatSource` or similar) with contents something like this:
```
EXPECT_TRUE(AsyncCheckTracker::IsMainPageLoadPending(
rfh_locator,
/*navigation_id=*/std::nullopt,
SBThreatType::SB_THREAT_TYPE_URL_PHISHING,
safe_browsing::ThreatSource::UNKNOWN));
EXPECT_FALSE(AsyncCheckTracker::IsMainPageLoadPending(
rfh_locator,
/*navigation_id=*/std::nullopt,
SBThreatType::SB_THREAT_TYPE_URL_PHISHING,
safe_browsing::ThreatSource::GLIC_COUNTER_ABUSE));
```
Related details
- Ari Chivukula
- Carlos Knippschild
- Chris Thompson
- Ken Buchanan
- Michael Crouse
- Rohit Rao
- Sarah Krakowiak
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. |
Carlos Knippschild (Gerrit)
Carlos Knippschild added 13 comments![]( "Open in Gerrit")
Bug: 515125124nit: use b:123 for internal issues.
```suggestion
Bug: b:515125124
```
Add the `// @generate glic_api` annotation here so that this enum is auto-generated within the TS API and can then be directly referenced from there (instead of using integers).
// Next ID: 4nit: we're not tracking next ID in this file, and they are not as useful for stable, extensible enums.
// LINT.ThenChange(//depot/google3/quality/malware/bineval/safebrowsing_public_taxonomy_extension.proto)Remove internal-only lint entry.
struct SafeBrowsingVerdictResult {nit: add doc comments to this and the next struct
#if BUILDFLAG(SAFE_BROWSING_AVAILABLE)
#include "chrome/browser/safe_browsing/safe_browsing_service.h"
#include "components/safe_browsing/content/browser/ui_manager.h"
#include "components/security_interstitials/core/unsafe_resource.h"
#include "content/public/browser/render_frame_host.h"
#endifMove this build-flag dependent includes block down to where other similar blocks exist.
GURL active_url = contents->GetVisibleURL();Doesn't this mean that the navigation to a bad URL has already happened? Isn't this too late to then show the interstitial?
g_browser_process->safe_browsing_service()->ui_manager().get();nit: The type returned by this call doesn't seem to be clear enough to justify using `auto`. Please replace with the specific type.
This is a log of logic that is very specific to this new SB check, so it would be better to move this to a separate file, especially if future improvements are being planned. This doesn't need to be done in this CL, but something to consider for a follow-up.
ContinueJsTest();Is this final step, that does nothing on the web client side, really needed?
if (!state.enableSkills) {Let's use the kGlicProcessCounterAbuseVerdict feature flag to "un-implement" the new API function as done here for other features.
// Triggered by Glic WebUI client when server reports a dangerous Counternit: I'd say this is triggered by the Glic (or GiC) web client, as the WebUI is just an intermediary layer between it and the browser.
Related details
- Ari Chivukula
- Chris Thompson
- Ken Buchanan
- Michael Crouse
- Rohit Rao
- Sarah Krakowiak
- 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. |
Chris Thompson (Gerrit)
Chris Thompson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Chris Thompson
| Code-Review | +1 |
@cth...@chromium.org: can you PTAL at tools/metrics/histograms/metadata/interstitial/ histograms.xml and components/security_interstitials/core/unsafe_resource.*
- Ari Chivukula
- Ken Buchanan
- Michael Crouse
- Rohit Rao
- Sarah Krakowiak
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. |