From b0762eb3db82486cec9bfe31cb45bf7e20716430 Mon Sep 17 00:00:00 2001
From: Remi NGUYEN VAN <reminv@google.com>
Date: Tue, 28 Aug 2018 11:06:54 +0900
Subject: Ignore DHCP packet sent from non-68 client port

This differs from previous behavior where dnsmasq would reply to port 68
if the client had no configured address (ciaddr empty in request), or
send replies to the client port if the request ciaddr matched the
assigned lease.
Not all DHCP servers preserve this behavior, and there is no good known
use-case for it. Not replying to such packets is less error-prone and
closer to the standard.

Bug: b/109584964
Test: Added test in DhcpServerTest.py passes
Change-Id: I88d467336cc4f4e4c9498c3787ec22fdef5e1cdd
---
 .../java/android/net/dhcp/DhcpPacketListener.java  | 24 +++++++-----
 services/net/java/android/net/dhcp/DhcpServer.java | 15 ++++++--
 .../net/java/android/net/dhcp/DhcpServerTest.java  | 43 ++++++++++++++--------
 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 498fd93fff59..6f620c5ce30e 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 45d0dbe32ace..095a5eb72289 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 70f85460b4de..66db5a8ddc50 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,24 +235,29 @@ 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());
         assertMatchesClient(packet);
     }
 
+    @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));
-- 
cgit v1.2.3-59-g8ed1b