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.

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

fix: remove wrong check of OverrideAndroidVPN when creating android routing rules #3

Conversation

Benyamin-Tehrani
Copy link

Bug description:

The iproute2 rule being created here is:
9000: from all fwmark 0xY/0x20000 goto 9010.
Its purpose is to control whether traffic that has been marked by VPNService.protect will bypass Mihomo.

According to the current code,

  • When OverrideAndroidVPN == true, this rule is created as 9000: from all fwmark 0x20000/0x20000 goto 9010, so
    • Traffic bypasses Mihomo when its VpnProtect bit is 1
    • Traffic goes into Mihomo for processing when its VpnProtect bit is 0
  • When OverrideAndroidVPN == false, this rule is created as 9000: from all fwmark 0x0/0x20000 goto 9010, meaning that
    • Traffic bypasses Mihomo when its VpnProtect bit is 0
    • Traffic goes into Mihomo for processing when its VpnProtect bit is 1

However, the logic here is wrong

In fact, this rule should always be created as 9000: from all fwmark 0x20000/0x20000 goto 9010, regardless of whether OverrideAndroidVPN is true or not

The reason is: the design goal of OverrideAndroidVPN is to guide Mihomo whether or not to use Android VPN as an upstream NIC.

  • If OverrideAndroidVPN == true, withauto-detect-interface, Mihomo automatically recognizes the Android VPN (tun0) as the upstream NIC, and all requests outbound from Mihomo are sent to tun0.
  • If OverrideAndroidVPN == false, all requests outbound from Mihomo should bypass the Android VPN, and go directly to the primary NIC (e.g. wlan0).

So, anyway, by the design of this feature, there should not be a situation where traffic sent outbound by Android VPN is still received by Mihomo. In other words, any traffic marked with Android VpnProtect mark must bypass Mihomo, and other normal traffic must goes into Mihomo, no matter what the parameters are set to. This ensures that Mihomo can always receives and processes traffic properly.

Current logic leads to the bug that, when OverrideAndroidVPN is set to false, all network traffic will first bypasses Mihomo and goes to the Android VPN for processing, then directly goes into the primary NIC (e.g., wlan0). Mihomo is unable to receive any of the traffic. This is obviously problematic, I think.

@Benyamin-Tehrani
Copy link
Author

This problem is quite easy to reproduce:

  1. Run a socks5 server on your PC
  2. Import the following configuration in Clash.Meta for Android, and turn off features like DNS hijacking, for testing:
mixed-port: 7890
socks-port: 7891

mode: rule
log-level: info

proxies:
- name: "socks5-test"
  type: socks5
  server: <SOCKS5-SERVER-ADDRESS>
  port: <SOCKS5-SERVER-PORT>
  username: <SOCKS5-SERVER-USERNAME>
  password: <SOCKS5-SERVER-PASSWORD>
  udp: true

proxy-groups:
  - name: test
    type: select
    proxies:
      - socks5-test
      - DIRECT

rules:
  - MATCH,test
  1. Start the VPN service of Clash.Meta for Android.
  2. According to the commit MetaCubeX/mihomo@69454b0,
  • Set the environment variable DISABLE_ OVERRIDE_ANDROID_VPN=true
  • Start the Mihomo Binary
  • You can see that these two Mihomo core can coexist normally. The traffic is processed by Mihomo Binary firstly, then forwarded to Clash.Meta for Android.
  1. Change to DISABLE_OVERRIDE_ANDROID_VPN=false and restart Mihomo Binary.
  • Then, you can notice that Mihomo Binary can't receive traffic any more.
  • According to the log of Clash.Meta for Android, it receives and processes all traffic.
  • Because Clash.Meta for Android only has a local socks5 proxy currently, you can't use your browser to access www.google.com and other websites that require proxies.

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

Successfully merging this pull request may close these issues.

1 participant