From 4e4d59eeef86fa8560d35c00168869b0a066f962 Mon Sep 17 00:00:00 2001
From: Lorenzo Colitti <lorenzo@google.com>
Date: Thu, 10 Sep 2015 18:12:18 +0900
Subject: [PATCH] Connect the DHCP UDP socket to the server.

This makes it so that the socket cannot receive datagrams from
anybody except the DHCP server. This does not improve security,
because we never read from the UDP socket anyway, but it does
make ListeningPortsTest pass.

Bug: 23906864
Bug: 23933386
Change-Id: Ib090273a417f7eb2ac1ee3309260249b72fb8345
---
 core/java/android/net/DhcpResults.java        |  9 +++----
 .../net/java/android/net/dhcp/DhcpClient.java | 26 ++++++++++++++-----
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/core/java/android/net/DhcpResults.java b/core/java/android/net/DhcpResults.java
index 87c063ff3eb0f..97bd5d284c711 100644
--- a/core/java/android/net/DhcpResults.java
+++ b/core/java/android/net/DhcpResults.java
@@ -21,7 +21,6 @@ import android.os.Parcel;
 import android.text.TextUtils;
 import android.util.Log;
 
-import java.net.InetAddress;
 import java.net.Inet4Address;
 import java.util.Objects;
 
@@ -34,7 +33,7 @@ import java.util.Objects;
 public class DhcpResults extends StaticIpConfiguration {
     private static final String TAG = "DhcpResults";
 
-    public InetAddress serverAddress;
+    public Inet4Address serverAddress;
 
     /** Vendor specific information (from RFC 2132). */
     public String vendorInfo;
@@ -142,7 +141,7 @@ public class DhcpResults extends StaticIpConfiguration {
     private static void readFromParcel(DhcpResults dhcpResults, Parcel in) {
         StaticIpConfiguration.readFromParcel(dhcpResults, in);
         dhcpResults.leaseDuration = in.readInt();
-        dhcpResults.serverAddress = NetworkUtils.unparcelInetAddress(in);
+        dhcpResults.serverAddress = (Inet4Address) NetworkUtils.unparcelInetAddress(in);
         dhcpResults.vendorInfo = in.readString();
     }
 
@@ -183,8 +182,8 @@ public class DhcpResults extends StaticIpConfiguration {
 
     public boolean setServerAddress(String addrString) {
         try {
-            serverAddress = NetworkUtils.numericToInetAddress(addrString);
-        } catch (IllegalArgumentException e) {
+            serverAddress = (Inet4Address) NetworkUtils.numericToInetAddress(addrString);
+        } catch (IllegalArgumentException|ClassCastException e) {
             Log.e(TAG, "setServerAddress failed with addrString " + addrString);
             return true;
         }
diff --git a/services/net/java/android/net/dhcp/DhcpClient.java b/services/net/java/android/net/dhcp/DhcpClient.java
index 3bf117f3040dc..28cb114ccf6bf 100644
--- a/services/net/java/android/net/dhcp/DhcpClient.java
+++ b/services/net/java/android/net/dhcp/DhcpClient.java
@@ -299,6 +299,7 @@ public class DhcpClient extends BaseDhcpStateMachine {
             Os.setsockoptInt(mUdpSock, SOL_SOCKET, SO_REUSEADDR, 1);
             Os.setsockoptIfreq(mUdpSock, SOL_SOCKET, SO_BINDTODEVICE, mIfaceName);
             Os.setsockoptInt(mUdpSock, SOL_SOCKET, SO_BROADCAST, 1);
+            Os.setsockoptInt(mUdpSock, SOL_SOCKET, SO_RCVBUF, 0);
             Os.bind(mUdpSock, Inet4Address.ANY, DhcpPacket.DHCP_CLIENT);
             NetworkUtils.protectFromVpn(mUdpSock);
         } catch(SocketException|ErrnoException e) {
@@ -308,6 +309,16 @@ public class DhcpClient extends BaseDhcpStateMachine {
         return true;
     }
 
+    private boolean connectUdpSock(Inet4Address to) {
+        try {
+            Os.connect(mUdpSock, to, DhcpPacket.DHCP_SERVER);
+            return true;
+        } catch (SocketException|ErrnoException e) {
+            Log.e(TAG, "Error connecting UDP socket", e);
+            return false;
+        }
+    }
+
     private static void closeQuietly(FileDescriptor fd) {
         try {
             IoBridge.closeAndSignalBlockedThreads(fd);
@@ -325,7 +336,7 @@ public class DhcpClient extends BaseDhcpStateMachine {
         try {
             mNMService.setInterfaceConfig(mIfaceName, ifcg);
         } catch (RemoteException|IllegalStateException e) {
-            Log.e(TAG, "Error configuring IP address : " + e);
+            Log.e(TAG, "Error configuring IP address " + address + ": ", e);
             return false;
         }
         return true;
@@ -377,8 +388,10 @@ public class DhcpClient extends BaseDhcpStateMachine {
                 maybeLog("Broadcasting " + description);
                 Os.sendto(mPacketSock, buf.array(), 0, buf.limit(), 0, mInterfaceBroadcastAddr);
             } else {
-                maybeLog("Unicasting " + description + " to " + to.getHostAddress());
-                Os.sendto(mUdpSock, buf, 0, to, DhcpPacket.DHCP_SERVER);
+                // It's safe to call getpeername here, because we only send unicast packets if we
+                // have an IP address, and we connect the UDP socket in DhcpHaveAddressState#enter.
+                maybeLog("Unicasting " + description + " to " + Os.getpeername(mUdpSock));
+                Os.write(mUdpSock, buf);
             }
         } catch(ErrnoException|IOException e) {
             Log.e(TAG, "Can't send packet: ", e);
@@ -790,6 +803,7 @@ public class DhcpClient extends BaseDhcpStateMachine {
                     transitionTo(mDhcpBoundState);
                 }
             } else if (packet instanceof DhcpNakPacket) {
+                // TODO: Wait a while before returning into INIT state.
                 Log.d(TAG, "Received NAK, returning to INIT");
                 mOffer = null;
                 transitionTo(mDhcpInitState);
@@ -807,10 +821,8 @@ public class DhcpClient extends BaseDhcpStateMachine {
         @Override
         public void enter() {
             super.enter();
-            if (setIpAddress(mDhcpLease.ipAddress)) {
-                maybeLog("Configured IP address " + mDhcpLease.ipAddress);
-            } else {
-                Log.e(TAG, "Failed to configure IP address " + mDhcpLease.ipAddress);
+            if (!setIpAddress(mDhcpLease.ipAddress) ||
+                    !connectUdpSock((mDhcpLease.serverAddress))) {
                 notifyFailure();
                 // There's likely no point in going into DhcpInitState here, we'll probably just
                 // repeat the transaction, get the same IP address as before, and fail.