mirror/fdroidclient
1
0
Fork 0
mirror of https://github.com/f-droid/fdroidclient.git synced 2026-07-30 09:06:35 -04:00
Code Issues Packages Projects Releases Wiki Activity

updates based on pr suggestions to avoid unintentional dns lookups

Browse Source
This commit is contained in:
mnbogner
2026-07-07 20:16:47 -07:00
parent 0f95122faf
commit 3695c4b80c
2 changed files with 212 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

88
app/src/full/java/org/fdroid/fdroid/nearby/SwapWorkflowActivity.java
View File

@@ -17,6 +17,7 @@ import android.content.ServiceConnection;
import android.content.pm.PackageManager;
import android.graphics.Bitmap;
import android.graphics.LightingColorFilter;
import android.net.InetAddresses;
import android.net.Uri;
import android.net.wifi.WifiInfo;
import android.net.wifi.WifiManager;
@@ -45,6 +46,7 @@ import androidx.annotation.LayoutRes;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.StringRes;
import androidx.annotation.VisibleForTesting;
import androidx.appcompat.app.AppCompatActivity;
import androidx.appcompat.widget.SearchView;
import androidx.core.content.ContextCompat;
@@ -83,6 +85,7 @@ import java.util.Set;
import java.util.Stack;
import java.util.Timer;
import java.util.TimerTask;
import java.util.regex.Pattern;
import javax.inject.Inject;
@@ -483,20 +486,26 @@ public class SwapWorkflowActivity extends AppCompatActivity {
confirmSwapConfig = repoConfig;
}
private static boolean isSwapUrl(Uri uri) {
return isSwapUrl(uri.getHost(), uri.getPort());
static boolean isSwapUrl(Uri uri) {
// check for null argument to prevent unexpected null pointer exceptions
return uri != null && isSwapUrl(uri.getHost(), uri.getPort());
}
private static boolean isSwapUrl(String host, int port) {
@VisibleForTesting
static boolean isSwapUrl(String host, int port) {
Utils.debugLog(TAG, String.format(Locale.ENGLISH, "Checking swap URL %s:%d", host, port));
if (port < 1024) {
// only root can use < 1024, so this can't be a swap repo
Utils.debugLog(TAG, String.format(Locale.ENGLISH, "Swap URL %s:%d is invalid because the port is out of range ( < 1024)", host, port));
return false;
}
// verify that host is a numeric ip address to prevent a possible dns loookup
if (!isNumericAddress(host)) {
Utils.debugLog(TAG, String.format(Locale.ENGLISH, "Swap URL %s:%d is invalid because it does not use a numeric ip address", host, port));
return false;
}
try {
InetAddress hostIp = InetAddress.getByName(host);
if (hostIp instanceof Inet4Address && FDroidApp.subnetInfo != null) {
if (isIpv4(host) && FDroidApp.subnetInfo != null) {
if (FDroidApp.subnetInfo.isInRange(host)) {
Utils.debugLog(TAG, String.format(Locale.ENGLISH, "Swap URL %s:%d is on the subnet (ipv4)", host, port));
return true;
@@ -504,7 +513,7 @@ public class SwapWorkflowActivity extends AppCompatActivity {
Utils.debugLog(TAG, String.format(Locale.ENGLISH, "Swap URL %s:%d is invalid because it is not on the subnet (ipv4)", host, port));
return false;
}
} else if (hostIp instanceof Inet6Address && FDroidApp.subnet6Info != null) {
} else if (isIpv6(host) && FDroidApp.subnet6Info != null) {
if (FDroidApp.subnet6Info.isInRange(host)) {
Utils.debugLog(TAG, String.format(Locale.ENGLISH, "Swap URL %s:%d is on the subnet (ipv6)", host, port));
return true;
@@ -517,14 +526,73 @@ public class SwapWorkflowActivity extends AppCompatActivity {
Utils.debugLog(TAG, String.format(Locale.ENGLISH, "Swap URL %s:%d is invalid because it was in an unexpected format", host, port));
return false;
}
} catch (UnknownHostException e) {
// if the host can't be parsed, it isn't a valid ip address
Utils.debugLog(TAG, String.format(Locale.ENGLISH, "Swap URL %s:%d is invalid because it could not be parsed", host, port));
return false;
} catch (NullPointerException e) {
// FDroidApp.subnetInfo/subnet6Info may not have been setup
Utils.debugLog(TAG, String.format(Locale.ENGLISH, "Swap URL %s:%d could not be validated because the subnet info was null", host, port));
return false;
} catch (IllegalArgumentException e) {
// in testing, isNumericAddress may return true for non-numeric adresses and
// subnetInfo/subnet6Info.isInRange fails to parse it and throws an exception
Utils.debugLog(TAG, String.format(Locale.ENGLISH, "Swap URL %s:%d could not be validated because it could not be parsed", host, port));
return false;
}
}
@VisibleForTesting
private static boolean isNumericAddress(String host) {
if (host == null) {
return false;
}
// use new isNumericAddress method if possible, otherwise attempt to use regex
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
return InetAddresses.isNumericAddress(host);
} else {
// regex found online, verified with unit testing
String ipv4String = "^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$";
String ipv6String = "^(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}$|" +
"^(?:[0-9a-fA-F]{1,4}:){1,7}:$|" +
"^(?:[0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}$|" +
"^(?:[0-9a-fA-F]{1,4}:){1,5}(?::[0-9a-fA-F]{1,4}){1,2}$|" +
"^(?:[0-9a-fA-F]{1,4}:){1,4}(?::[0-9a-fA-F]{1,4}){1,3}$|" +
"^(?:[0-9a-fA-F]{1,4}:){1,3}(?::[0-9a-fA-F]{1,4}){1,4}$|" +
"^(?:[0-9a-fA-F]{1,4}:){1,2}(?::[0-9a-fA-F]{1,4}){1,5}$|" +
"^[0-9a-fA-F]{1,4}:(?::[0-9a-fA-F]{1,4}){1,6}$|" +
"^:(?::[0-9a-fA-F]{1,4}){1,7}$|" +
"^::$|" +
"^(?:[0-9a-fA-F]{1,4}:){1,4}:(?:(?:25[0-5]|(?:2[0-4]|1?[0-9])?[0-9])\\.){3}(?:25[0-5]|(?:2[0-4]|1?[0-9])?[0-9])$";
Pattern ipv4Pattern = Pattern.compile(ipv4String);
Pattern ipv6Pattern = Pattern.compile(ipv6String);
return ipv4Pattern.matcher(host).matches() || ipv6Pattern.matcher(host).matches();
}
}
private static boolean isIpv4(String host) {
// use new parseNumericAddress method if possible, otherwise attempt to check for format
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
try {
InetAddress i = InetAddresses.parseNumericAddress(host);
return (i instanceof Inet4Address);
} catch (IllegalArgumentException e) {
return false;
}
} else {
// no IPv4 literal should contain ":"
return !host.contains(":");
}
}
private static boolean isIpv6(String host) {
// use new parseNumericAddress method if possible, otherwise attempt to check for format
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
try {
InetAddress i = InetAddresses.parseNumericAddress(host);
return (i instanceof Inet6Address);
} catch (IllegalArgumentException e) {
return false;
}
} else {
// every IPv6 literal should contain ":"
return host.contains(":");
}
}

134
app/src/testFull/java/org/fdroid/fdroid/nearby/SwapWorkflowActivityTest.java Normal file
View File

@@ -0,0 +1,134 @@
package org.fdroid.fdroid.nearby;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import android.net.Uri;
import android.os.Build;
import org.apache.commons.net.util.SubnetUtils;
import org.apache.commons.net.util.SubnetUtils6;
import org.fdroid.fdroid.FDroidApp;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.annotation.Config;
/**
* Tests for {@link SwapWorkflowActivity#isSwapUrl}, which is the security gate
* that decides whether an incoming swap URL points at a repo on the local
* subnet. The host must be a numeric IP literal on our subnet; it must never
* be resolved via DNS (see merge request !1682 discussion). The two SDK
* branches (API 29+ {@code InetAddresses.isNumericAddress} vs. the legacy
* pattern fallback) are exercised separately.
*/
@RunWith(RobolectricTestRunner.class)
@Config(sdk = Build.VERSION_CODES.P) // default: legacy pattern-match branch
public class SwapWorkflowActivityTest {
private SubnetUtils.SubnetInfo savedSubnetInfo;
@Before
public void setUp() {
savedSubnetInfo = FDroidApp.subnetInfo;
// pretend the device sits on 192.168.0.0/24(ipv4) 2001:db8:abcd::/64(ipv6)
FDroidApp.subnetInfo = new SubnetUtils("192.168.0.1/24").getInfo();
FDroidApp.subnet6Info = new SubnetUtils6("2001:db8:abcd::/64").getInfo();
}
@After
public void tearDown() {
FDroidApp.subnetInfo = savedSubnetInfo;
}
// ------------------------------------------------------------------
// legacy branch (API < 29, host.matches("[0-9.]+"))
// ------------------------------------------------------------------
@Test
public void testValidSwapUrlLegacy() {
assertTrue(SwapWorkflowActivity.isSwapUrl("192.168.0.50", 8888));
assertTrue(SwapWorkflowActivity.isSwapUrl("2001:db8:abcd::1234", 8888));
}
@Test
public void testOutOfSubnetLegacy() {
assertFalse(SwapWorkflowActivity.isSwapUrl("10.0.0.50", 8888));
assertFalse(SwapWorkflowActivity.isSwapUrl("2010:db8:abcd::1234", 8888));
}
@Test
public void testHostnameIsNeverResolvedLegacy() {
// a hostname must be rejected outright, never resolved via DNS
assertFalse(SwapWorkflowActivity.isSwapUrl("example.com", 8888));
assertFalse(SwapWorkflowActivity.isSwapUrl("localhost", 8888));
}
// removed testIpv6IsRejectedLegacy because ipv6 addresses are now supported
// ------------------------------------------------------------------
// modern branch (API 29+, android.net.InetAddresses.isNumericAddress)
// ------------------------------------------------------------------
@Test
@Config(sdk = Build.VERSION_CODES.R)
public void testValidSwapUrlModern() {
assertTrue(SwapWorkflowActivity.isSwapUrl("192.168.0.50", 8888));
assertTrue(SwapWorkflowActivity.isSwapUrl("2001:db8:abcd::1234", 8888));
}
@Test
@Config(sdk = Build.VERSION_CODES.R)
public void testOutOfSubnetModern() {
assertFalse(SwapWorkflowActivity.isSwapUrl("10.0.0.50", 8888));
assertFalse(SwapWorkflowActivity.isSwapUrl("2010:db8:abcd::1234", 8888));
}
@Test
@Config(sdk = Build.VERSION_CODES.R)
public void testHostnameIsNeverResolvedModern() {
// a hostname must be rejected outright, never resolved via DNS
// several of these are considered numeric due to a Robolectric issue, but fail
// anyway due to an IllegalArgumentException that happens further in the code
assertFalse(SwapWorkflowActivity.isSwapUrl("foo", 8888));
assertFalse(SwapWorkflowActivity.isSwapUrl("1", 8888));
assertFalse(SwapWorkflowActivity.isSwapUrl("example.com", 8888));
assertFalse(SwapWorkflowActivity.isSwapUrl("localhost", 8888));
}
// removed testIpv6IsRejectedModern because ipv6 addresses are now supported
// ------------------------------------------------------------------
// port and null-safety (independent of the numeric-address branch)
// ------------------------------------------------------------------
@Test
public void testPrivilegedPortIsRejected() {
assertFalse(SwapWorkflowActivity.isSwapUrl("192.168.0.50", 80));
assertFalse(SwapWorkflowActivity.isSwapUrl("192.168.0.50", 1023));
}
@Test
public void testNullHostIsRejected() {
assertFalse(SwapWorkflowActivity.isSwapUrl((String) null, 8888));
}
@Test
public void testNullUriIsRejected() {
// regression: checkIfNewRepoOnSameWifi() may pass a null repo Uri
assertFalse(SwapWorkflowActivity.isSwapUrl((Uri) null));
}
@Test
public void testUriWithoutHostIsRejected() {
assertFalse(SwapWorkflowActivity.isSwapUrl(Uri.parse("mailto:swap@example.com")));
}
@Test
public void testValidSwapUri() {
assertTrue(SwapWorkflowActivity.isSwapUrl(
Uri.parse("http://192.168.0.50:8888/fdroid/repo")));
}
}