Skip to content

Pass block_untunneled_traffic flag throughh to WireGuard service - #39405

Open
simonhong wants to merge 1 commit into
wireguard_block_untunned_traffic_prefsfrom
deliver_block_untunneled_traffic_prefs_to_tunnel_service
Open

Pass block_untunneled_traffic flag throughh to WireGuard service#39405
simonhong wants to merge 1 commit into
wireguard_block_untunned_traffic_prefsfrom
deliver_block_untunneled_traffic_prefs_to_tunnel_service

Conversation

@simonhong

@simonhong simonhong commented Aug 26, 2026

Copy link
Copy Markdown
Member

Pass block_untunneled_traffic flag to wireguard tunnel service.
For now, there is no behavioral change.
In the f/u, tunnel service will create config and install WFP filters based on that flag.

@simonhong simonhong self-assigned this Aug 26, 2026
@simonhong
simonhong force-pushed the deliver_block_untunneled_traffic_prefs_to_tunnel_service branch 2 times, most recently from 925d6b1 to 277e9f7 Compare August 26, 2026 05:52
@simonhong simonhong changed the title Deliver block untunneled traffic prefs to tunnel service Pass block_untunneled_traffic flag throughh to WireGuard service Aug 26, 2026
@simonhong
simonhong force-pushed the deliver_block_untunneled_traffic_prefs_to_tunnel_service branch from 277e9f7 to 68006f1 Compare August 27, 2026 01:35
@simonhong
simonhong marked this pull request as ready for review August 27, 2026 01:39
@simonhong
simonhong requested a review from netzenbot August 27, 2026 01:39
@simonhong
simonhong force-pushed the deliver_block_untunneled_traffic_prefs_to_tunnel_service branch from 68006f1 to e3e6222 Compare August 27, 2026 02:00
@github-actions

Copy link
Copy Markdown
Contributor

[puLL-Merge] - brave/brave-core@39405

Description

Threads a block_untunneled_traffic flag from the browser process down to the Windows WireGuard elevated service. Adds BraveVPNConnectionManager::ShouldBlockUntunneledTraffic() (reads kBraveVPNWireguardBlockUntunneledTraffic && kBraveVPNWireguardEnabled local prefs), extends the IBraveVpnWireguardManager::EnableVpn COM interface with a new [in] BOOL param, and regenerates MIDL output for x86/x64/arm64.

Possible Issues

  • Flag is dead: CreateWireguardConfig() takes block_untunneled_traffic but its body is unchanged in this diff — no AllowedIPs/kill-switch behavior derived from it. Feature currently no-ops end to end. If intentional (plumbing-only PR), state that; otherwise the config generation change is missing.
  • COM interface changed without new IID: IBraveVpnWireguardManager vtable/proc format changed while keeping the same interface ID. During upgrade, a new browser can talk to an already-registered old service (or vice versa) → argument marshalling mismatch, E_INVALIDARG, or stack corruption in an elevated service. Standard practice for these Brave/Chromium elevated-service interfaces is to define a new versioned interface (e.g. IBraveVpnWireguardManager2) and keep the old one. Verify installer sequencing forces service binary replacement before browser use.
  • ShouldBlockUntunneledTraffic() non-const, no return path: relies on NOTREACHED() being [[noreturn]]; will warn/fail on toolchains where it isn't. Also should be const — it only reads prefs — and the callsite in wireguard_connection_api_impl_win.cc compiles only when wireguard is enabled, otherwise it hits NOTREACHED() at runtime. Guard the callsite with the buildflag or drop the NOTREACHED() and return false.
  • const bool by value in declarations: top-level const on value params in headers is noise; Chromium style prefers plain bool.
  • Tray reconnect hardcodes false: StatusTrayRunner::ConnectVPN() passes false. Reconnect path uses last-known-good config so it's likely ignored, but if the empty-param path ever regenerates config the kill switch silently drops. Add a comment or read the pref.
  • Other CreateWireguardConfig callers/tests: only one callsite updated. Confirm unit tests (wireguard_utils_unittest) and any other callers were updated — no test changes are in this diff, so a mandatory-param signature change likely breaks them.
  • local_prefs_ nullability not checked before GetBoolean.

Security Hotspots

  1. components/brave_vpn/common/wireguard/win/brave_wireguard_manager_idl.idl + regenerated _p.c — modifying the interface of an elevated (SYSTEM) COM service in place. Version skew between browser and service produces mismatched proc format strings; NDR stubs on a privileged surface are the highest-risk part of this change. New interface ID recommended.
  2. Kill-switch semantics: since CreateWireguardConfig ignores the flag, a user enabling "block untunneled traffic" gets no enforcement — traffic leaks outside the tunnel while UI implies protection.
Changes

Changes

  • components/.../brave_wireguard_manager_idl.idl: adds [in] BOOL block_untunneled_traffic to EnableVpn.
  • win_build_output/midl/.../{x86,x64,arm64}: regenerated headers, proxy/stub format strings, .tlb.
  • browser/brave_vpn/win/brave_vpn_wireguard_service/service/brave_wireguard_manager.{h,cc}: implements new param, forwards to CreateWireguardConfig.
  • browser/brave_vpn/win/wireguard_utils_win.{h,cc}: adds param to EnableBraveVpnWireguardService / ...Impl, forwards over COM.
  • browser/brave_vpn/win/wireguard_connection_api_impl_win.cc: passes manager_->ShouldBlockUntunneledTraffic().
  • components/brave_vpn/browser/connection/brave_vpn_connection_manager.{h,cc}: new pref-backed accessor, NOTREACHED() on non-wireguard builds.
  • components/brave_vpn/common/wireguard/wireguard_utils.{h,cc}: CreateWireguardConfig signature extended (unused in body).
  • .../status_tray/status_tray_runner.cc: passes false on tray reconnect.
sequenceDiagram
    participant UI as Browser (VPN UI)
    participant Mgr as BraveVPNConnectionManager
    participant Impl as WireguardConnectionAPIImplWin
    participant Utils as wireguard_utils_win
    participant COM as BraveWireguardManager (SYSTEM service)
    participant Cfg as CreateWireguardConfig

    UI->>Impl: PlatformConnectImpl(credentials)
    Impl->>Mgr: ShouldBlockUntunneledTraffic()
    Mgr->>Mgr: local_prefs_ BlockUntunneled && WireguardEnabled
    Mgr-->>Impl: bool
    Impl->>Utils: EnableBraveVpnWireguardService(keys, host, block, proxy, cb)
    Utils->>COM: EnableVpn(pub, priv, addr, endpoint, block, &last_error)
    COM->>Cfg: CreateWireguardConfig(..., block)
    Note over Cfg: flag currently unused in body
    Cfg-->>COM: config
    COM-->>Utils: HRESULT
    Utils-->>Impl: OnWireguardServiceLaunched(success)
Loading

@simonhong
simonhong force-pushed the deliver_block_untunneled_traffic_prefs_to_tunnel_service branch from e3e6222 to 14a1b36 Compare August 27, 2026 02:48
@simonhong
simonhong force-pushed the deliver_block_untunneled_traffic_prefs_to_tunnel_service branch from 14a1b36 to 3407561 Compare August 27, 2026 04:29
const std::string& vpn_server_hostname,
const std::string& mapped_ipv4_address) {
const std::string& mapped_ipv4_address,
const bool block_untunneled_traffic) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

block_untunneled_traffic is added to the signature but never used in the body — the config template still hardcodes AllowedIPs = 0.0.0.0/0, ::/0, so everything plumbed through EnableVpn is dropped here and the flag has no effect. Either consume it (e.g. adjust AllowedIPs) in this PR or drop the parameter until the config change lands.

local_prefs_->GetBoolean(prefs::kBraveVPNWireguardEnabled);
#else
NOTREACHED();
#endif

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: this new guard spans more than 3 lines and has an #else branch, so per BS-045 the #endif should say what it closes: #endif // BUILDFLAG(ENABLE_BRAVE_VPN_WIREGUARD). (The rest of this file uses bare #endifs, so feel free to skip if you'd rather keep the file uniform.) (Refined Rule: #endif Comments Based on Block Length)

prefs::kBraveVPNWireguardBlockUntunneledTraffic) &&
local_prefs_->GetBoolean(prefs::kBraveVPNWireguardEnabled);
#else
NOTREACHED();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: NOTREACHED() is defined in base/notreached.h, which this file doesn't include (only base/check.h / base/logging.h are). Please add the include rather than relying on a transitive one. (✅ Always Include What You Use (IWYU))

void CheckConnection();

void SetSelectedRegion(const std::string& name);
bool ShouldBlockUntunneledTraffic();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: worth a one-line comment on this new public method — the implementation NOTREACHED()s when ENABLE_BRAVE_VPN_WIREGUARD is off and it also requires kBraveVPNWireguardEnabled, neither of which is obvious from the name. (✅ Method Documentation Should Describe the Contract)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants