Allow $private HTTP rule to block any private IP

This is a little magic compared with our previous approach of "list
every private IP range", but given then the sheer number we were
missing[1][2] this feels more reasonable.

Also refactor out some of the logic into separate classes, hopefully to
make things a little cleaner.

Fixes #594.

[1]: https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml
[2]: https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml

build.gradle

@@ -116,8 +116,9 @@ dependencies {
 
     shade 'org.squiddev:Cobalt:0.5.1-SNAPSHOT'
 
-    testImplementation 'org.junit.jupiter:junit-jupiter-api:5.4.2'
+    testImplementation 'org.junit.jupiter:junit-jupiter-api:5.7.0'
-    testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.4.2'
+    testImplementation 'org.junit.jupiter:junit-jupiter-params:5.7.0'
+    testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.0'
     testImplementation 'org.hamcrest:hamcrest:2.2'
 
     deployerJars "org.apache.maven.wagon:wagon-ssh:3.0.0"

src/main/java/dan200/computercraft/ComputerCraft.java

@@ -22,30 +22,17 @@ import net.minecraftforge.fml.common.Mod;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
-import java.util.Objects;
 import java.util.concurrent.TimeUnit;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 @Mod( ComputerCraft.MOD_ID )
 public final class ComputerCraft
 {
     public static final String MOD_ID = "computercraft";
 
-    // Configuration options
-    public static final String[] DEFAULT_HTTP_ALLOW = new String[] { "*" };
-    public static final String[] DEFAULT_HTTP_DENY = new String[] {
-        "127.0.0.0/8",
-        "0.0.0.0/8",
-        "10.0.0.0/8",
-        "172.16.0.0/12",
-        "192.168.0.0/16",
-        "fd00::/8",
-    };
-
     public static int computerSpaceLimit = 1000 * 1000;
     public static int floppySpaceLimit = 125 * 1000;
     public static int maximumFilesOpen = 128;
@@ -61,14 +48,10 @@ public final class ComputerCraft
 
     public static boolean httpEnabled = true;
     public static boolean httpWebsocketEnabled = true;
-    public static List<AddressRule> httpRules = Collections.unmodifiableList( Stream.concat(
+    public static List<AddressRule> httpRules = Collections.unmodifiableList( Arrays.asList(
-        Stream.of( DEFAULT_HTTP_DENY )
+        AddressRule.parse( "$private", null, Action.DENY.toPartial() ),
-            .map( x -> AddressRule.parse( x, null, Action.DENY.toPartial() ) )
+        AddressRule.parse( "*", null, Action.ALLOW.toPartial() )
-            .filter( Objects::nonNull ),
+    ) );
-        Stream.of( DEFAULT_HTTP_ALLOW )
-            .map( x -> AddressRule.parse( x, null, Action.ALLOW.toPartial() ) )
-            .filter( Objects::nonNull )
-    ).collect( Collectors.toList() ) );
 
     public static int httpMaxRequests = 16;
     public static int httpMaxWebsockets = 4;

src/main/java/dan200/computercraft/core/apis/http/options/AddressPredicate.java

@@ -0,0 +1,149 @@
+/*
+ * This file is part of ComputerCraft - http://www.computercraft.info
+ * Copyright Daniel Ratcliffe, 2011-2020. Do not distribute without permission.
+ * Send enquiries to dratcliffe@gmail.com
+ */
+
+package dan200.computercraft.core.apis.http.options;
+
+import com.google.common.net.InetAddresses;
+import dan200.computercraft.ComputerCraft;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.regex.Pattern;
+
+/**
+ * A predicate on an address. Matches against a domain and an ip address.
+ *
+ * @see AddressRule#apply(Iterable, String, InetSocketAddress) for the actual handling of this rule.
+ */
+interface AddressPredicate
+{
+    default boolean matches( String domain )
+    {
+        return false;
+    }
+
+    default boolean matches( InetAddress socketAddress )
+    {
+        return false;
+    }
+
+    final class HostRange implements AddressPredicate
+    {
+        private final byte[] min;
+        private final byte[] max;
+
+        HostRange( byte[] min, byte[] max )
+        {
+            this.min = min;
+            this.max = max;
+        }
+
+        @Override
+        public boolean matches( InetAddress address )
+        {
+            byte[] entry = address.getAddress();
+            if( entry.length != min.length ) return false;
+
+            for( int i = 0; i < entry.length; i++ )
+            {
+                int value = 0xFF & entry[i];
+                if( value < (0xFF & min[i]) || value > (0xFF & max[i]) ) return false;
+            }
+
+            return true;
+        }
+
+        public static HostRange parse( String addressStr, String prefixSizeStr )
+        {
+            int prefixSize;
+            try
+            {
+                prefixSize = Integer.parseInt( prefixSizeStr );
+            }
+            catch( NumberFormatException e )
+            {
+                ComputerCraft.log.error(
+                    "Malformed http whitelist/blacklist entry '{}': Cannot extract size of CIDR mask from '{}'.",
+                    addressStr + '/' + prefixSizeStr, prefixSizeStr
+                );
+                return null;
+            }
+
+            InetAddress address;
+            try
+            {
+                address = InetAddresses.forString( addressStr );
+            }
+            catch( IllegalArgumentException e )
+            {
+                ComputerCraft.log.error(
+                    "Malformed http whitelist/blacklist entry '{}': Cannot extract IP address from '{}'.",
+                    addressStr + '/' + prefixSizeStr, prefixSizeStr
+                );
+                return null;
+            }
+
+            // Mask the bytes of the IP address.
+            byte[] minBytes = address.getAddress(), maxBytes = address.getAddress();
+            int size = prefixSize;
+            for( int i = 0; i < minBytes.length; i++ )
+            {
+                if( size <= 0 )
+                {
+                    minBytes[i] &= 0;
+                    maxBytes[i] |= 0xFF;
+                }
+                else if( size < 8 )
+                {
+                    minBytes[i] &= 0xFF << (8 - size);
+                    maxBytes[i] |= ~(0xFF << (8 - size));
+                }
+
+                size -= 8;
+            }
+
+            return new HostRange( minBytes, maxBytes );
+        }
+    }
+
+    final class DomainPattern implements AddressPredicate
+    {
+        private final Pattern pattern;
+
+        DomainPattern( Pattern pattern )
+        {
+            this.pattern = pattern;
+        }
+
+        @Override
+        public boolean matches( String domain )
+        {
+            return pattern.matcher( domain ).matches();
+        }
+
+        @Override
+        public boolean matches( InetAddress socketAddress )
+        {
+            return pattern.matcher( socketAddress.getHostAddress() ).matches();
+        }
+    }
+
+
+    final class PrivatePattern implements AddressPredicate
+    {
+        static final PrivatePattern INSTANCE = new PrivatePattern();
+
+        @Override
+        public boolean matches( InetAddress socketAddress )
+        {
+            return socketAddress.isAnyLocalAddress()
+                || socketAddress.isLoopbackAddress()
+                || socketAddress.isLinkLocalAddress()
+                || socketAddress.isSiteLocalAddress();
+        }
+    }
+
+}

src/main/java/dan200/computercraft/core/apis/http/options/AddressRule.java

@@ -6,10 +6,13 @@
 package dan200.computercraft.core.apis.http.options;
 
 import com.google.common.net.InetAddresses;
-import dan200.computercraft.ComputerCraft;
+import dan200.computercraft.core.apis.http.options.AddressPredicate.DomainPattern;
+import dan200.computercraft.core.apis.http.options.AddressPredicate.HostRange;
+import dan200.computercraft.core.apis.http.options.AddressPredicate.PrivatePattern;
 
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
+import java.net.Inet4Address;
 import java.net.Inet6Address;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
@@ -25,45 +28,13 @@ public final class AddressRule
     public static final int TIMEOUT = 30_000;
     public static final int WEBSOCKET_MESSAGE = 128 * 1024;
 
-    private static final class HostRange
+    private final AddressPredicate predicate;
-    {
-        private final byte[] min;
-        private final byte[] max;
-
-        private HostRange( byte[] min, byte[] max )
-        {
-            this.min = min;
-            this.max = max;
-        }
-
-        public boolean contains( InetAddress address )
-        {
-            byte[] entry = address.getAddress();
-            if( entry.length != min.length ) return false;
-
-            for( int i = 0; i < entry.length; i++ )
-            {
-                int value = 0xFF & entry[i];
-                if( value < (0xFF & min[i]) || value > (0xFF & max[i]) ) return false;
-            }
-
-            return true;
-        }
-    }
-
-    private final HostRange ip;
-    private final Pattern domainPattern;
     private final Integer port;
     private final PartialOptions partial;
 
-    private AddressRule(
+    private AddressRule( @Nonnull AddressPredicate predicate, @Nullable Integer port, @Nonnull PartialOptions partial )
-        @Nullable HostRange ip,
-        @Nullable Pattern domainPattern,
-        @Nullable Integer port,
-        @Nonnull PartialOptions partial )
     {
-        this.ip = ip;
+        this.predicate = predicate;
-        this.domainPattern = domainPattern;
         this.partial = partial;
         this.port = port;
     }
@@ -76,103 +47,50 @@ public final class AddressRule
         {
             String addressStr = filter.substring( 0, cidr );
             String prefixSizeStr = filter.substring( cidr + 1 );
-
+            HostRange range = HostRange.parse( addressStr, prefixSizeStr );
-            int prefixSize;
+            return range == null ? null : new AddressRule( range, port, partial );
-            try
+        }
-            {
+        else if( filter.equalsIgnoreCase( "$private" ) )
-                prefixSize = Integer.parseInt( prefixSizeStr );
+        {
-            }
+            return new AddressRule( PrivatePattern.INSTANCE, port, partial );
-            catch( NumberFormatException e )
-            {
-                ComputerCraft.log.error(
-                    "Malformed http whitelist/blacklist entry '{}': Cannot extract size of CIDR mask from '{}'.",
-                    filter, prefixSizeStr
-                );
-                return null;
-            }
-
-            InetAddress address;
-            try
-            {
-                address = InetAddresses.forString( addressStr );
-            }
-            catch( IllegalArgumentException e )
-            {
-                ComputerCraft.log.error(
-                    "Malformed http whitelist/blacklist entry '{}': Cannot extract IP address from '{}'.",
-                    filter, prefixSizeStr
-                );
-                return null;
-            }
-
-            // Mask the bytes of the IP address.
-            byte[] minBytes = address.getAddress(), maxBytes = address.getAddress();
-            int size = prefixSize;
-            for( int i = 0; i < minBytes.length; i++ )
-            {
-                if( size <= 0 )
-                {
-                    minBytes[i] &= 0;
-                    maxBytes[i] |= 0xFF;
-                }
-                else if( size < 8 )
-                {
-                    minBytes[i] &= 0xFF << (8 - size);
-                    maxBytes[i] |= ~(0xFF << (8 - size));
-                }
-
-                size -= 8;
-            }
-
-            return new AddressRule( new HostRange( minBytes, maxBytes ), null, port, partial );
         }
         else
         {
-            Pattern pattern = Pattern.compile( "^\\Q" + filter.replaceAll( "\\*", "\\\\E.*\\\\Q" ) + "\\E$" );
+            Pattern pattern = Pattern.compile( "^\\Q" + filter.replaceAll( "\\*", "\\\\E.*\\\\Q" ) + "\\E$", Pattern.CASE_INSENSITIVE );
-            return new AddressRule( null, pattern, port, partial );
+            return new AddressRule( new DomainPattern( pattern ), port, partial );
         }
     }
 
     /**
      * Determine whether the given address matches a series of patterns.
      *
-     * @param domain        The domain to match
+     * @param domain      The domain to match
-     * @param socketAddress The address to check.
+     * @param port        The port of the address.
+     * @param address     The address to check.
+     * @param ipv4Address An ipv4 version of the address, if the original was an ipv6 address.
      * @return Whether it matches any of these patterns.
      */
-    private boolean matches( String domain, InetSocketAddress socketAddress )
+    private boolean matches( String domain, int port, InetAddress address, Inet4Address ipv4Address )
     {
-        InetAddress address = socketAddress.getAddress();
+        if( this.port != null && this.port != port ) return false;
-        if( port != null && port != socketAddress.getPort() ) return false;
+        return predicate.matches( domain )
-
+            || predicate.matches( address )
-        if( domainPattern != null )
+            || (ipv4Address != null && predicate.matches( ipv4Address ));
-        {
-            if( domainPattern.matcher( domain ).matches() ) return true;
-            if( domainPattern.matcher( address.getHostName() ).matches() ) return true;
-        }
-
-        // Match the normal address
-        if( matchesAddress( address ) ) return true;
-
-        // If we're an IPv4 address in disguise then let's check that.
-        return address instanceof Inet6Address && InetAddresses.is6to4Address( (Inet6Address) address )
-            && matchesAddress( InetAddresses.get6to4IPv4Address( (Inet6Address) address ) );
     }
 
-    private boolean matchesAddress( InetAddress address )
+    public static Options apply( Iterable<? extends AddressRule> rules, String domain, InetSocketAddress socketAddress )
-    {
-        if( domainPattern != null && domainPattern.matcher( address.getHostAddress() ).matches() ) return true;
-        return ip != null && ip.contains( address );
-    }
-
-    public static Options apply( Iterable<? extends AddressRule> rules, String domain, InetSocketAddress address )
     {
         PartialOptions options = null;
         boolean hasMany = false;
 
+        int port = socketAddress.getPort();
+        InetAddress address = socketAddress.getAddress();
+        Inet4Address ipv4Address = address instanceof Inet6Address && InetAddresses.is6to4Address( (Inet6Address) address )
+            ? InetAddresses.get6to4IPv4Address( (Inet6Address) address ) : null;
+
         for( AddressRule rule : rules )
         {
-            if( !rule.matches( domain, address ) ) continue;
+            if( !rule.matches( domain, port, address, ipv4Address ) ) continue;
 
             if( options == null )
             {

src/main/java/dan200/computercraft/shared/Config.java

@@ -21,12 +21,12 @@ import net.minecraftforge.fml.ModLoadingContext;
 import net.minecraftforge.fml.common.Mod;
 import net.minecraftforge.fml.config.ModConfig;
 
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import static net.minecraftforge.common.ForgeConfigSpec.Builder;
 import static net.minecraftforge.common.ForgeConfigSpec.ConfigValue;
@@ -180,12 +180,10 @@ public final class Config
                     "Each rule is an item with a 'host' to match against, and a series of properties. " +
                     "The host may be a domain name (\"pastebin.com\"),\n" +
                     "wildcard (\"*.pastebin.com\") or CIDR notation (\"127.0.0.0/8\"). If no rules, the domain is blocked." )
-                .defineList( "rules",
+                .defineList( "rules", Arrays.asList(
-                    Stream.concat(
+                    AddressRuleConfig.makeRule( "$private", Action.DENY ),
-                        Stream.of( ComputerCraft.DEFAULT_HTTP_DENY ).map( x -> AddressRuleConfig.makeRule( x, Action.DENY ) ),
+                    AddressRuleConfig.makeRule( "*", Action.ALLOW )
-                        Stream.of( ComputerCraft.DEFAULT_HTTP_ALLOW ).map( x -> AddressRuleConfig.makeRule( x, Action.ALLOW ) )
+                ), x -> x instanceof UnmodifiableConfig && AddressRuleConfig.checkRule( (UnmodifiableConfig) x ) );
-                    ).collect( Collectors.toList() ),
-                    x -> x instanceof UnmodifiableConfig && AddressRuleConfig.checkRule( (UnmodifiableConfig) x ) );
 
             httpMaxRequests = builder
                 .comment( "The number of http requests a computer can make at one time. Additional requests will be queued, and sent when the running requests have finished. Set to 0 for unlimited." )

src/test/java/dan200/computercraft/core/apis/http/options/AddressRuleTest.java

@@ -6,7 +6,10 @@
 
 package dan200.computercraft.core.apis.http.options;
 
+import dan200.computercraft.ComputerCraft;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 import java.net.InetSocketAddress;
 import java.util.Collections;
@@ -27,6 +30,17 @@ public class AddressRuleTest
         assertEquals( apply( rules, "localhost", 8081 ).action, Action.DENY );
     }
 
+    @ParameterizedTest
+    @ValueSource( strings = {
+        "0.0.0.0", "[::]",
+        "localhost", "lvh.me", "127.0.0.1", "[::1]",
+        "172.17.0.1", "192.168.1.114", "[0:0:0:0:0:ffff:c0a8:172]", "10.0.0.1"
+    } )
+    public void blocksLocalDomains( String domain )
+    {
+        assertEquals( apply( ComputerCraft.httpRules, domain, 80 ).action, Action.DENY );
+    }
+
     private Options apply( Iterable<AddressRule> rules, String host, int port )
     {
         return AddressRule.apply( rules, host, new InetSocketAddress( host, port ) );

src/test/resources/test-rom/spec/apis/http_spec.lua

@@ -12,6 +12,8 @@ describe("The http library", function()
         end)
 
         it("rejects local domains", function()
+            -- Note, this is tested more thoroughly in AddressRuleTest. We've just got this here
+            -- to ensure the general control flow works.
             expect({ http.checkURL("http://localhost") }):same({ false, "Domain not permitted" })
             expect({ http.checkURL("http://127.0.0.1") }):same({ false, "Domain not permitted" })
         end)