Merge "Ignore DHCP packet sent from non-68 client port"

3 files changed, 53 insertions, 29 deletions

diff --git a/services/net/java/android/net/dhcp/DhcpPacketListener.java b/services/net/java/android/net/dhcp/DhcpPacketListener.java
index 498fd93fff5..6f620c5ce30 100644
--- a/services/net/java/android/net/dhcp/DhcpPacketListener.java
+++ b/services/net/java/android/net/dhcp/DhcpPacketListener.java
@@ -16,15 +16,14 @@
 
 package android.net.dhcp;
 
+import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.net.util.FdEventsReader;
-import android.net.util.PacketReader;
 import android.os.Handler;
 import android.system.Os;
 
 import java.io.FileDescriptor;
 import java.net.Inet4Address;
-import java.net.InetAddress;
 import java.net.InetSocketAddress;
 
 /**
@@ -35,19 +34,20 @@ abstract class DhcpPacketListener extends FdEventsReader<DhcpPacketListener.Payl
     static final class Payload {
         final byte[] bytes = new byte[DhcpPacket.MAX_LENGTH];
         Inet4Address srcAddr;
+        int srcPort;
     }
 
-    public DhcpPacketListener(Handler handler) {
+    public DhcpPacketListener(@NonNull Handler handler) {
         super(handler, new Payload());
     }
 
     @Override
-    protected int recvBufSize(Payload buffer) {
+    protected int recvBufSize(@NonNull Payload buffer) {
         return buffer.bytes.length;
     }
 
     @Override
-    protected final void handlePacket(Payload recvbuf, int length) {
+    protected final void handlePacket(@NonNull Payload recvbuf, int length) {
         if (recvbuf.srcAddr == null) {
             return;
         }
@@ -55,30 +55,34 @@ abstract class DhcpPacketListener extends FdEventsReader<DhcpPacketListener.Payl
         try {
             final DhcpPacket packet = DhcpPacket.decodeFullPacket(recvbuf.bytes, length,
                     DhcpPacket.ENCAP_BOOTP);
-            onReceive(packet, recvbuf.srcAddr);
+            onReceive(packet, recvbuf.srcAddr, recvbuf.srcPort);
         } catch (DhcpPacket.ParseException e) {
             logParseError(recvbuf.bytes, length, e);
         }
     }
 
     @Override
-    protected int readPacket(FileDescriptor fd, Payload packetBuffer) throws Exception {
+    protected int readPacket(@NonNull FileDescriptor fd, @NonNull Payload packetBuffer)
+            throws Exception {
         final InetSocketAddress addr = new InetSocketAddress();
         final int read = Os.recvfrom(
                 fd, packetBuffer.bytes, 0, packetBuffer.bytes.length, 0 /* flags */, addr);
 
         // Buffers with null srcAddr will be dropped in handlePacket()
         packetBuffer.srcAddr = inet4AddrOrNull(addr);
+        packetBuffer.srcPort = addr.getPort();
         return read;
     }
 
     @Nullable
-    private static Inet4Address inet4AddrOrNull(InetSocketAddress addr) {
+    private static Inet4Address inet4AddrOrNull(@NonNull InetSocketAddress addr) {
         return addr.getAddress() instanceof Inet4Address
                 ? (Inet4Address) addr.getAddress()
                 : null;
     }
 
-    protected abstract void onReceive(DhcpPacket packet, Inet4Address srcAddr);
-    protected abstract void logParseError(byte[] packet, int length, DhcpPacket.ParseException e);
+    protected abstract void onReceive(@NonNull DhcpPacket packet, @NonNull Inet4Address srcAddr,
+            int srcPort);
+    protected abstract void logParseError(@NonNull byte[] packet, int length,
+            @NonNull DhcpPacket.ParseException e);
 }
diff --git a/services/net/java/android/net/dhcp/DhcpServer.java b/services/net/java/android/net/dhcp/DhcpServer.java
index 45d0dbe32ac..095a5eb7228 100644
--- a/services/net/java/android/net/dhcp/DhcpServer.java
+++ b/services/net/java/android/net/dhcp/DhcpServer.java
@@ -19,6 +19,7 @@ package android.net.dhcp;
 import static android.net.NetworkUtils.getBroadcastAddress;
 import static android.net.NetworkUtils.getPrefixMaskAsInet4Address;
 import static android.net.TrafficStats.TAG_SYSTEM_DHCP_SERVER;
+import static android.net.dhcp.DhcpPacket.DHCP_CLIENT;
 import static android.net.dhcp.DhcpPacket.DHCP_SERVER;
 import static android.net.dhcp.DhcpPacket.ENCAP_BOOTP;
 import static android.net.dhcp.DhcpPacket.INFINITE_LEASE;
@@ -238,8 +239,14 @@ public class DhcpServer {
     }
 
     @VisibleForTesting
-    void processPacket(@NonNull DhcpPacket packet) {
-        mLog.log("Received packet of type " + packet.getClass().getSimpleName());
+    void processPacket(@NonNull DhcpPacket packet, int srcPort) {
+        final String packetType = packet.getClass().getSimpleName();
+        if (srcPort != DHCP_CLIENT) {
+            mLog.logf("Ignored packet of type %s sent from client port %d", packetType, srcPort);
+            return;
+        }
+
+        mLog.log("Received packet of type " + packetType);
         final Inet4Address sid = packet.mServerIdentifier;
         if (sid != null && !sid.equals(mServingParams.serverAddr.getAddress())) {
             mLog.log("Packet ignored due to wrong server identifier: " + sid);
@@ -469,8 +476,8 @@ public class DhcpServer {
         }
 
         @Override
-        protected void onReceive(DhcpPacket packet, Inet4Address srcAddr) {
-            processPacket(packet);
+        protected void onReceive(DhcpPacket packet, Inet4Address srcAddr, int srcPort) {
+            processPacket(packet, srcPort);
         }
 
         @Override
diff --git a/tests/net/java/android/net/dhcp/DhcpServerTest.java b/tests/net/java/android/net/dhcp/DhcpServerTest.java
index 70f85460b4d..66db5a8ddc5 100644
--- a/tests/net/java/android/net/dhcp/DhcpServerTest.java
+++ b/tests/net/java/android/net/dhcp/DhcpServerTest.java
@@ -16,6 +16,7 @@
 
 package android.net.dhcp;
 
+import static android.net.dhcp.DhcpPacket.DHCP_CLIENT;
 import static android.net.dhcp.DhcpPacket.ENCAP_BOOTP;
 import static android.net.dhcp.DhcpPacket.INADDR_ANY;
 import static android.net.dhcp.DhcpPacket.INADDR_BROADCAST;
@@ -27,9 +28,11 @@ import static junit.framework.Assert.assertNull;
 import static junit.framework.Assert.assertTrue;
 
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.ArgumentMatchers.isNull;
 import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -178,7 +181,7 @@ public class DhcpServerTest {
         final DhcpDiscoverPacket discover = new DhcpDiscoverPacket(TEST_TRANSACTION_ID,
                 (short) 0 /* secs */, INADDR_ANY /* relayIp */, TEST_CLIENT_MAC_BYTES,
                 false /* broadcast */, INADDR_ANY /* srcIp */);
-        mServer.processPacket(discover);
+        mServer.processPacket(discover, DHCP_CLIENT);
 
         assertResponseSentTo(TEST_CLIENT_ADDR);
         final DhcpOfferPacket packet = assertOffer(getPacket());
@@ -194,13 +197,22 @@ public class DhcpServerTest {
         final DhcpDiscoverPacket discover = new DhcpDiscoverPacket(TEST_TRANSACTION_ID,
                 (short) 0 /* secs */, INADDR_ANY /* relayIp */, TEST_CLIENT_MAC_BYTES,
                 false /* broadcast */, INADDR_ANY /* srcIp */);
-        mServer.processPacket(discover);
+        mServer.processPacket(discover, DHCP_CLIENT);
 
         assertResponseSentTo(INADDR_BROADCAST);
         final DhcpNakPacket packet = assertNak(getPacket());
         assertMatchesClient(packet);
     }
 
+    private DhcpRequestPacket makeRequestSelectingPacket() {
+        final DhcpRequestPacket request = new DhcpRequestPacket(TEST_TRANSACTION_ID,
+                (short) 0 /* secs */, INADDR_ANY /* clientIp */, INADDR_ANY /* relayIp */,
+                TEST_CLIENT_MAC_BYTES, false /* broadcast */);
+        request.mServerIdentifier = TEST_SERVER_ADDR;
+        request.mRequestedIp = TEST_CLIENT_ADDR;
+        return request;
+    }
+
     @Test
     public void testRequest_Selecting_Ack() throws Exception {
         when(mRepository.requestLease(isNull() /* clientId */, eq(TEST_CLIENT_MAC),
@@ -208,12 +220,8 @@ public class DhcpServerTest {
                 eq(true) /* sidSet */, isNull() /* hostname */))
                 .thenReturn(TEST_LEASE);
 
-        final DhcpRequestPacket request = new DhcpRequestPacket(TEST_TRANSACTION_ID,
-                (short) 0 /* secs */, INADDR_ANY /* clientIp */, INADDR_ANY /* relayIp */,
-                TEST_CLIENT_MAC_BYTES, false /* broadcast */);
-        request.mServerIdentifier = TEST_SERVER_ADDR;
-        request.mRequestedIp = TEST_CLIENT_ADDR;
-        mServer.processPacket(request);
+        final DhcpRequestPacket request = makeRequestSelectingPacket();
+        mServer.processPacket(request, DHCP_CLIENT);
 
         assertResponseSentTo(TEST_CLIENT_ADDR);
         final DhcpAckPacket packet = assertAck(getPacket());
@@ -227,12 +235,8 @@ public class DhcpServerTest {
                 eq(true) /* sidSet */, isNull() /* hostname */))
                 .thenThrow(new InvalidAddressException("Test error"));
 
-        final DhcpRequestPacket request = new DhcpRequestPacket(TEST_TRANSACTION_ID,
-                (short) 0 /* secs */, INADDR_ANY /* clientIp */, INADDR_ANY /* relayIp */,
-                TEST_CLIENT_MAC_BYTES, false /* broadcast */);
-        request.mServerIdentifier = TEST_SERVER_ADDR;
-        request.mRequestedIp = TEST_CLIENT_ADDR;
-        mServer.processPacket(request);
+        final DhcpRequestPacket request = makeRequestSelectingPacket();
+        mServer.processPacket(request, DHCP_CLIENT);
 
         assertResponseSentTo(INADDR_BROADCAST);
         final DhcpNakPacket packet = assertNak(getPacket());
@@ -240,11 +244,20 @@ public class DhcpServerTest {
     }
 
     @Test
+    public void testRequest_Selecting_WrongClientPort() throws Exception {
+        final DhcpRequestPacket request = makeRequestSelectingPacket();
+        mServer.processPacket(request, 50000);
+
+        verify(mRepository, never()).requestLease(any(), any(), any(), any(), anyBoolean(), any());
+        verify(mDeps, never()).sendPacket(any(), any(), any());
+    }
+
+    @Test
     public void testRelease() throws Exception {
         final DhcpReleasePacket release = new DhcpReleasePacket(TEST_TRANSACTION_ID,
                 TEST_SERVER_ADDR, TEST_CLIENT_ADDR,
                 INADDR_ANY /* relayIp */, TEST_CLIENT_MAC_BYTES);
-        mServer.processPacket(release);
+        mServer.processPacket(release, DHCP_CLIENT);
 
         verify(mRepository, times(1))
                 .releaseLease(isNull(), eq(TEST_CLIENT_MAC), eq(TEST_CLIENT_ADDR));