Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" #3422

Merged
merged 37 commits into from
Nov 11, 2024
Merged

Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" #3422

merged 37 commits into from
Nov 11, 2024

Conversation

diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Oct 20, 2024 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/1206580121312550/1208686409805161/f
Tech Design URL: https://app.asana.com/0/481882893211075/1208643192597095/f

iOS PR: iOS PR: duckduckgo/iOS#3460
BSK PR: duckduckgo/BrowserServicesKit#1039

Description

Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks".

Testing

Test 1: External Users aren't affected

Prerequisites:

  • Make sure you're an external user.

Steps:

  1. The easiest way to test is by disabling Debug > VPN > Tunnel Settings > excludeLocalNetworks, because when enforceRoutes is OFF, you'll be able to access your local network devices, but when it's ON you won't be able to access those devices.
  2. Star the VPN.
  3. Make sure you can access the internet and all local devices.

Test 2: Internal Users get new routing

Prerequisites:

  • Make sure you're an internal user.
  • Make sure the enforceRoutes remote feature flag is ON.

Steps:
2. Star the VPN (the first time you start the VPN as an internal user, "Enforce routes" should be enabled).
3. Make sure you can access the internet.
4. Make sure you cannot access local devices.

Test 3: Internal Users can override enforce routes.

Prerequisites:

  • This should be run after test 2, as it shows that enforce routes won't be force-enabled more than once.
  • Make sure you're an internal user.

Steps:

  1. If the VPN is ON, turn it OFF.
  2. Disable Debug > VPN > Tunnel Settings > enforceRoutes
  3. Start the VPN
  4. Make sure you can access local network devices.

Test 4: TunnelVision fixed when enforceRoutes is ON.

Prerequisites:

  • Make sure you're an internal user.
  • Make sure enforceRoutes is ON.
  • Prepare TunnelVision against ifconfig.me

Steps:

  1. Start the VPN.
  2. Check against an IP address site (not ifconfig.me) that the right tunnel address is shown.
  3. Connect to a WiFi exploiting TunnelVision against site ifconfig.me.
  4. Load site ifconfig.me and ensure it shows the VPN tunnel IP.

Test 5: TunnelCrack blocks all VPN traffic.

Prerequisites:

  • Make sure you're an internal user.
  • Make sure enforceRoutes is ON.
  • Prepare the TunnelCrack local network exploit.

Steps:

  1. Start the VPN.
  2. See that no traffic gets out of the VPN, and the connection is blocked.

Please note: I'm suggesting we show a warning here, but this will be tackled as a follow-up task.

Definition of Done:

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@diegoreymendez diegoreymendez changed the title Fix TunnelVision Fix TunnelVision using enforceRoutes Oct 20, 2024
This was referenced Nov 4, 2024
Merged
Merged
@diegoreymendez diegoreymendez changed the title Fix TunnelVision using enforceRoutes Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" Nov 4, 2024
diegoreymendez
diegoreymendez commented Nov 4, 2024
View reviewed changes
DuckDuckGo/FeatureFlagging/Model/FeatureFlag.swift
@@ -41,6 +41,9 @@ public enum FeatureFlag: String {

/// https://app.asana.com/0/72649045549333/1208231259093710/f
case networkProtectionUserTips

/// /// https://app.asana.com/0/72649045549333/1208617860225199/f
case networkProtectionEnforceRoutes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New remote feature flag for the enforce routes changes.

diegoreymendez
diegoreymendez commented Nov 4, 2024
View reviewed changes
DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenu.swift
.targetting(self)

NSMenuItem(title: "Remove VPN configuration", action: #selector(NetworkProtectionDebugMenu.removeVPNConfiguration(_:)))
NSMenuItem(title: "Reset Site Issue Alert", action: #selector(NetworkProtectionDebugMenu.resetSiteIssuesAlert(_:)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was moved to organize the reset options: first go the general reset options, then single components, then subfeatures.

diegoreymendez
diegoreymendez commented Nov 4, 2024
View reviewed changes
DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenu.swift
Comment on lines +323 to +326
Task {
try await Task.sleep(interval: 0.1)
try await debugUtilities.restartAdapter()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait a bit then restart the adapter so changes are applied. If we try to reset immediately the changes don't make it to the system extension in time.

diegoreymendez
diegoreymendez commented Nov 4, 2024
View reviewed changes
DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionTunnelController.swift
@@ -349,23 +349,19 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
protocolConfiguration.providerBundleIdentifier = Bundle.tunnelExtensionBundleID
protocolConfiguration.providerConfiguration = [
NetworkProtectionOptionKey.defaultPixelHeaders: APIRequest.Headers().httpHeaders,
NetworkProtectionOptionKey.includedRoutes: includedRoutes().map(\.stringRepresentation) as NSArray
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're no longer going to pass routes. The extension will calculate the appropriate routes based on all the settings.

not-a-rootkit
not-a-rootkit approved these changes Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@not-a-rootkit not-a-rootkit left a comment

Choose a reason for hiding this comment

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

Fairly straightforward, and well implemented. Tested the build, seems to work as expected with the feature flagging too.

diegoreymendez added a commit to duckduckgo/BrowserServicesKit that referenced this pull request Nov 11, 2024
@diegoreymendez
Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" (#…
614ea57 
…1039)

Task/Issue URL:
https://app.asana.com/0/1206580121312550/1208686409805161/f
Tech Design URL:
https://app.asana.com/0/481882893211075/1208643192597095/f

iOS PR: duckduckgo/iOS#3460
macOS PR: duckduckgo/macos-browser#3422
What kind of version bump will this require?: Major

## Description

Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks".
@diegoreymendez diegoreymendez merged commit 20c9f0a into main Nov 11, 2024
18 checks passed
@diegoreymendez diegoreymendez deleted the diego/fix-tunnelvision-2 branch November 11, 2024 20:38
diegoreymendez added a commit to duckduckgo/iOS that referenced this pull request Nov 11, 2024
@diegoreymendez
Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" (#…
f8fc6c5 
…3460)

Task/Issue URL:
https://app.asana.com/0/1206580121312550/1208686409805161/f
Tech Design URL:
https://app.asana.com/0/481882893211075/1208643192597095/f

macOS PR: duckduckgo/macos-browser#3422
BSK PR: duckduckgo/BrowserServicesKit#1039

## Description

Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks".
samsymons added a commit that referenced this pull request Nov 12, 2024
@samsymons
Merge branch 'main' into dominik/xcode-16
4a9cb20 
# By Dax the Duck (5) and others
# Via GitHub (3) and Juan Manuel Pereira (1)
* main:
  Remediate TunnelVision, TunnelCrack and fix "Exclude Local Networks" (#3422)
  Sync: Send pixels for account removal + decoding issues (#3530)
  Send success pixel on successful data import (#3437)
  Bump version to 1.114.0 (304)
  Set marketing version to 1.114.0
  Update embedded files
  Introduce report broken site prompt on triple refresh page (#3523)
  add state for import (#3485)
  Bump version to 1.113.0 (303)
  Do not use suggestion view controller visibility to set suggestion (#3529)
  Fix SwiftLint
  Fix SwiftLint
  Update phishing protection datasets to 1686837 (#3494)
  Bump version to 1.113.0 (302)
  Update permission string for location popup (#3526)
  Send auth state with sync unexpectedly disabled pixel (#3521)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@not-a-rootkit not-a-rootkit not-a-rootkit approved these changes

@samsymons samsymons Awaiting requested review from samsymons

Assignees

@diegoreymendez diegoreymendez

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@diegoreymendez @samsymons @not-a-rootkit