diff --git a/app/src/main/java/eu/faircode/netguard/ActivityMain.java b/app/src/main/java/eu/faircode/netguard/ActivityMain.java index d67fb589..f3ea778d 100644 --- a/app/src/main/java/eu/faircode/netguard/ActivityMain.java +++ b/app/src/main/java/eu/faircode/netguard/ActivityMain.java @@ -467,6 +467,21 @@ public void onClick(View v) { } }); + // Private DNS pinned to a hostname while we block DoT: name resolution + // fails outright, so the user may never see the notification that says + // so — nothing they tap can load anything either. + TextView tvPrivateDns = findViewById(R.id.tvPrivateDns); + tvPrivateDns.setVisibility(View.GONE); + tvPrivateDns.setOnClickListener(new View.OnClickListener() { + @Override + public void onClick(View v) { + Intent settings = new Intent(Settings.ACTION_WIRELESS_SETTINGS); + if (settings.resolveActivity(getPackageManager()) == null) + settings = new Intent(Settings.ACTION_WIFI_SETTINGS); + startActivity(settings); + } + }); + // Application list RecyclerView rvApplication = findViewById(R.id.rvApplication); rvApplication.setHasFixedSize(false); @@ -567,6 +582,16 @@ protected void onResume() { tvLocalNetwork.setVisibility( LocalNetworkAccess.isMissing(this) ? View.VISIBLE : View.GONE); + // Only while we are actually filtering: with the VPN off, port 853 is + // not blocked and a pinned resolver works fine. + TextView tvPrivateDns = findViewById(R.id.tvPrivateDns); + if (tvPrivateDns != null) { + boolean vpnEnabled = PreferenceManager.getDefaultSharedPreferences(this) + .getBoolean("enabled", false); + tvPrivateDns.setVisibility( + vpnEnabled && Util.isPrivateDnsBlocked(this) ? View.VISIBLE : View.GONE); + } + super.onResume(); } diff --git a/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java b/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java index 3df0bda4..ded42060 100644 --- a/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java +++ b/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java @@ -9,6 +9,7 @@ final class NetworkReloadPolicy { static final String REASON_NETWORK_CHANGED = "Network changed"; static final String REASON_CONNECTED_CHANGED = "Connected state changed"; static final String REASON_LINK_PROPERTIES_CHANGED = "link properties changed"; + static final String REASON_PRIVATE_DNS_CHANGED = "private DNS changed"; static final String REASON_METERED_CHANGED = "Metered state changed"; static final String REASON_CONNECTIVITY_CHANGED = "connectivity changed"; @@ -30,10 +31,17 @@ static String onConnectivityChanged() { } static String onLinkPropertiesChanged(List lastDns, List currentDns, - boolean compareDns, boolean reloadOnConnectivity) { + boolean compareDns, boolean reloadOnConnectivity, + String lastPrivateDns, String currentPrivateDns) { if (compareDns ? !same(lastDns, currentDns) : reloadOnConnectivity) return REASON_LINK_PROPERTIES_CHANGED; + // Pinning Private DNS to a hostname leaves the resolver list alone, so + // the comparison above never sees it — yet it decides whether blocking + // DoT stops name resolution outright, which the user has to be told. + if (!Objects.equals(lastPrivateDns, currentPrivateDns)) + return REASON_PRIVATE_DNS_CHANGED; + return null; } @@ -62,6 +70,15 @@ static boolean shouldRestartWireGuard(String reason) { REASON_CONNECTIVITY_CHANGED.equals(reason); } + /** + * The same decision across a coalesced burst of callbacks, which keeps only + * the last reason. The need for a rebind is sticky: once any reason in the + * burst required one, a later reason that does not must not cancel it. + */ + static boolean shouldRestartWireGuard(boolean pendingRestart, String reason) { + return pendingRestart || shouldRestartWireGuard(reason); + } + static boolean same(List last, List current) { if (last == null || current == null || last.size() != current.size()) return false; diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 1f060e80..7f4532f1 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -119,6 +119,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.zip.GZIPInputStream; @@ -1693,7 +1694,7 @@ private Builder getBuilder(List listAllowed, List listRule) { // In "hostname" mode it does not fall back — the user picked that // resolver explicitly, so DNS simply fails and nothing resolves. That // looks like TC broke the connection, with no hint of why, so say it. - if (prefs.getBoolean("block_dot", true) && Util.isPrivateDnsHostnameMode(this)) { + if (Util.isPrivateDnsBlocked(this)) { Log.w(TAG, "Private DNS set to a hostname: DoT is blocked and Android will not" + " fall back to plaintext DNS, so name resolution fails"); showPrivateDnsNotification(Util.getPrivateDnsSpecifier(this)); @@ -3146,6 +3147,7 @@ private void listenNetworkChanges() { private Boolean last_connected = null; private Boolean last_metered = null; private List last_dns = null; + private String last_private_dns = null; @Override public void onAvailable(Network network) { @@ -3168,17 +3170,26 @@ public void onLinkPropertiesChanged(Network network, LinkProperties linkProperti // Make sure the right DNS servers are being used List dns = linkProperties.getDnsServers(); + // Non-null only when Private DNS is pinned to a hostname, which + // leaves the resolver list untouched — so this is the only part + // of the properties that reveals the change. + String private_dns = (Build.VERSION.SDK_INT < Build.VERSION_CODES.P + ? null : linkProperties.getPrivateDnsServerName()); SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(ServiceSinkhole.this); String reason = NetworkReloadPolicy.onLinkPropertiesChanged( last_dns, dns, Build.VERSION.SDK_INT >= Build.VERSION_CODES.O, - prefs.getBoolean("reload_onconnectivity", false)); + prefs.getBoolean("reload_onconnectivity", false), + last_private_dns, + private_dns); if (reason != null) { Log.i(TAG, "Changed link properties=" + linkProperties + "DNS cur=" + TextUtils.join(",", dns) + - "DNS prv=" + (last_dns == null ? null : TextUtils.join(",", last_dns))); + "DNS prv=" + (last_dns == null ? null : TextUtils.join(",", last_dns)) + + " private DNS cur=" + private_dns + " prv=" + last_private_dns); last_dns = dns; + last_private_dns = private_dns; reloadAfterNetworkChange(reason); } } @@ -3228,19 +3239,24 @@ public void onLost(Network network) { // ConnectivityManager callbacks within milliseconds of each other. Each // reload is a foreground-service update + wakelock + native VPN restart + // WireGuard rebind, so bursts are coalesced into a single reload using the - // last reason once the burst settles. Every reason string currently in use - // maps to the same shouldRestartWireGuard()==true branch, so collapsing to - // the latest reason changes no behaviour beyond the log line. + // last reason once the burst settles. Not every reason needs the rebind, so + // the need for one is accumulated across the burst rather than read off the + // surviving reason: a reason that does not need it must not cancel one that + // did, or the tunnel keeps a socket bound to a network that is gone. private static final long NETWORK_RELOAD_DEBOUNCE_MS = 1500L; private static final Object NETWORK_RELOAD_TOKEN = new Object(); private final Handler networkReloadDebounceHandler = new Handler(Looper.getMainLooper()); + private final AtomicBoolean pendingWireGuardRestart = new AtomicBoolean(false); private void reloadAfterNetworkChange(final String reason) { + // Callbacks arrive off the main thread; the reload runs on it. + if (NetworkReloadPolicy.shouldRestartWireGuard(reason)) + pendingWireGuardRestart.set(true); networkReloadDebounceHandler.removeCallbacksAndMessages(NETWORK_RELOAD_TOKEN); networkReloadDebounceHandler.postAtTime(new Runnable() { @Override public void run() { - if (NetworkReloadPolicy.shouldRestartWireGuard(reason)) + if (pendingWireGuardRestart.getAndSet(false)) net.kollnig.missioncontrol.wg.WgEgress.INSTANCE.onUnderlyingNetworkChanged(); reload(reason, ServiceSinkhole.this, false); } diff --git a/app/src/main/java/eu/faircode/netguard/Util.java b/app/src/main/java/eu/faircode/netguard/Util.java index 5ed4b240..4a7fb0fb 100644 --- a/app/src/main/java/eu/faircode/netguard/Util.java +++ b/app/src/main/java/eu/faircode/netguard/Util.java @@ -57,6 +57,7 @@ import androidx.core.app.ActivityCompat; import androidx.core.app.NotificationManagerCompat; import androidx.core.net.ConnectivityManagerCompat; +import androidx.preference.PreferenceManager; import net.kollnig.missioncontrol.BuildConfig; import net.kollnig.missioncontrol.R; @@ -572,6 +573,18 @@ public static String getPrivateDnsSpecifier(Context context) { return Settings.Global.getString(context.getContentResolver(), "private_dns_specifier"); } + /** + * Whether the device is left with no working resolver: Private DNS pinned + * to a hostname while we block DoT. Deliberately keyed on the mode rather + * than on the resolver name, which is only decoration for the warning — + * a name that reads back empty must not turn the warning off. + */ + public static boolean isPrivateDnsBlocked(Context context) { + return PreferenceManager.getDefaultSharedPreferences(context) + .getBoolean("block_dot", true) + && isPrivateDnsHostnameMode(context); + } + public interface DoubtListener { void onSure(); } diff --git a/app/src/main/res/layout/main.xml b/app/src/main/res/layout/main.xml index ac818322..326f2deb 100644 --- a/app/src/main/res/layout/main.xml +++ b/app/src/main/res/layout/main.xml @@ -64,6 +64,26 @@ android:textColor="?attr/colorOff" android:visibility="gone" /> + + + . + */ + +package eu.faircode.netguard; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import android.content.Context; +import android.content.SharedPreferences; +import android.provider.Settings; + +import androidx.preference.PreferenceManager; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.RuntimeEnvironment; + +/** + * Which Private DNS configurations leave the device with no working resolver. + * + *

Only "hostname" mode does: Android falls back to plaintext DNS when a + * resolver it picked itself cannot be reached over DoT, but not when the user + * named one explicitly, so blocking port 853 stops resolution outright. + */ +@RunWith(RobolectricTestRunner.class) +public class PrivateDnsBlockedTest { + private Context context; + private SharedPreferences prefs; + + @Before + public void setUp() { + context = RuntimeEnvironment.getApplication(); + prefs = PreferenceManager.getDefaultSharedPreferences(context); + prefs.edit().clear().commit(); + setPrivateDns("off", null); + } + + private void setPrivateDns(String mode, String specifier) { + Settings.Global.putString(context.getContentResolver(), "private_dns_mode", mode); + Settings.Global.putString(context.getContentResolver(), "private_dns_specifier", specifier); + } + + @Test + public void pinnedResolverWithDotBlockingBreaksResolution() { + setPrivateDns("hostname", "dns.google"); + assertTrue(Util.isPrivateDnsBlocked(context)); + } + + @Test + public void automaticModeDoesNotBreakResolution() { + // Android falls back to plaintext on its own, which is the whole reason + // the onboarding slide could go. + setPrivateDns("opportunistic", null); + assertFalse(Util.isPrivateDnsBlocked(context)); + } + + @Test + public void privateDnsOffDoesNotBreakResolution() { + setPrivateDns("off", null); + assertFalse(Util.isPrivateDnsBlocked(context)); + } + + @Test + public void researchModeLeavesPinnedResolversAlone() { + // Research mode turns DoT blocking off, so a pinned resolver still works. + setPrivateDns("hostname", "dns.google"); + prefs.edit().putBoolean("block_dot", false).commit(); + assertFalse(Util.isPrivateDnsBlocked(context)); + } + + /** + * The decision must not hinge on reading the specifier: if that read comes + * back empty while the mode says "hostname", resolution is still broken and + * staying silent would leave the user with no explanation at all. + */ + @Test + public void pinnedResolverWithoutReadableNameStillBreaksResolution() { + setPrivateDns("hostname", null); + assertTrue(Util.isPrivateDnsBlocked(context)); + assertNull(Util.getPrivateDnsSpecifier(context)); + } + + @Test + public void specifierIsOnlyReportedForPinnedResolvers() { + setPrivateDns("hostname", "dns.google"); + assertEquals("dns.google", Util.getPrivateDnsSpecifier(context)); + + setPrivateDns("opportunistic", "dns.google"); + assertNull(Util.getPrivateDnsSpecifier(context)); + } +}