Do not crash on malformed DHCP packets.

This fixes possible crashes with malformed DHCP packets.
It is part of Android Security Bulletin of January 2017 and
mitigates CVE CVE-2017-0389.

Squash of:

Author: Lorenzo Colitti <lorenzo@google.com>
Date:   Wed Mar 02 13:31:52 2016 +0900
    Don't crash if we get a DHCP packet with the wrong port.
    This should only happen if we get a packet in the small time
    window between binding the packet socket and programming the
    BPF filter on it.
    Bug: 26696823
    Change-Id: I481f1bc74bbaeb9646d96e1841d2a69acdb47d62

Author: Hugo Benichi <hugobenichi@google.com>
Date:   Wed Oct 05 18:33:21 2016 +0900
    Catch runtime exceptions when parsing DHCP packets
    This patch adds a try catch all to DHCP packet parsing so that
    DhcpClient does not choke on malformed packets, brinding down with it
    the whole framework.
    Test: added new unit tests catching the issue fixed in this patch.
    Bug: 31850211
    Change-Id: I3c50a149fed6b2cbc4f40bb4f0e5bb2b56859b44

Author: Hugo Benichi <hugobenichi@google.com>
Date:   Wed Oct 05 21:07:19 2016 +0900
    Reject DHCP packets with no magic cookie
    This patch adds an explicit check in the DHCP packet parser for
    rejecting packets without a magic cookie, instead of relying on the
    top-level try-catch-all in the parser.
    This allows to add to DHCP error metrics this specific error.
    It also allows to add two poor man's fuzzing tests that tries to find
    additional gaps in the DHCP packet parser by
     - trying to parse all subslices of a valid offer packet.
     - trying to parse random byte arrays.
    Test: covered by previously introduced malformed DHCP packet unit tests
          + additional fuzzing tests.
    Bug: 31850211
    Change-Id: If53c9ba9df78d7604ec018c9d67c237ae59c4833

Change-Id: Ic5a8fa4feb46cca325cb5c47616ee63b22d2e7c8
mh0rst: Backported to cm-13.0.

2 files changed, 181 insertions, 35 deletions

diff --git a/services/net/java/android/net/dhcp/DhcpPacket.java b/services/net/java/android/net/dhcp/DhcpPacket.java
index f97df83..820a03d 100644
--- a/services/net/java/android/net/dhcp/DhcpPacket.java
+++ b/services/net/java/android/net/dhcp/DhcpPacket.java
@@ -6,6 +6,7 @@ import android.net.NetworkUtils;
 import android.os.Build;
 import android.os.SystemProperties;
 import android.system.OsConstants;
+import com.android.internal.annotations.VisibleForTesting;
 
 import java.io.UnsupportedEncodingException;
 import java.net.Inet4Address;
@@ -13,9 +14,8 @@ import java.net.UnknownHostException;
 import java.nio.BufferUnderflowException;
 import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
-import java.nio.charset.StandardCharsets;
 import java.nio.ShortBuffer;
-
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
@@ -701,7 +701,8 @@ abstract class DhcpPacket {
      * A subset of the optional parameters are parsed and are stored
      * in object fields.
      */
-    public static DhcpPacket decodeFullPacket(ByteBuffer packet, int pktType) throws ParseException
+    @VisibleForTesting
+    static DhcpPacket decodeFullPacket(ByteBuffer packet, int pktType) throws ParseException
     {
         // bootp parameters
         int transactionId;
@@ -809,7 +810,11 @@ abstract class DhcpPacket {
             // server-to-server packets, e.g. for relays.
             if (!isPacketToOrFromClient(udpSrcPort, udpDstPort) &&
                 !isPacketServerToServer(udpSrcPort, udpDstPort)) {
-                return null;
+                // This should almost never happen because we use SO_ATTACH_FILTER on the packet
+                // socket to drop packets that don't have the right source ports. However, it's
+                // possible that a packet arrives between when the socket is bound and when the
+                // filter is set. http://b/26696823 .
+                throw new ParseException("Unexpected UDP ports %d->%d", udpSrcPort, udpDstPort);
             }
         }
 
@@ -860,8 +865,12 @@ abstract class DhcpPacket {
                         + 64    // skip server host name (64 chars)
                         + 128); // skip boot file name (128 chars)
 
-        int dhcpMagicCookie = packet.getInt();
+        // Ensure this is a DHCP packet with a magic cookie, and not BOOTP. http://b/31850211
+        if (packet.remaining() < 4) {
+            throw new ParseException("DHCP packet without a magic cookie");
+        }
 
+        int dhcpMagicCookie = packet.getInt();
         if (dhcpMagicCookie != DHCP_MAGIC_COOKIE) {
             throw new ParseException("Bad magic cookie 0x%08x, should be 0x%08x", dhcpMagicCookie,
                     DHCP_MAGIC_COOKIE);
@@ -1049,7 +1058,13 @@ abstract class DhcpPacket {
     public static DhcpPacket decodeFullPacket(byte[] packet, int length, int pktType)
             throws ParseException {
         ByteBuffer buffer = ByteBuffer.wrap(packet, 0, length).order(ByteOrder.BIG_ENDIAN);
-        return decodeFullPacket(buffer, pktType);
+        try {
+            return decodeFullPacket(buffer, pktType);
+        } catch (ParseException e) {
+            throw e;
+        } catch (Exception e) {
+            throw new ParseException("DHCP parsing error: %s", e.getMessage());
+        }
     }
 
     /**
diff --git a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
index 7e60bf1..2639e26 100644
--- a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
+++ b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
@@ -16,23 +16,19 @@
 
 package android.net.dhcp;
 
-import android.net.NetworkUtils;
 import android.net.DhcpResults;
 import android.net.LinkAddress;
+import android.net.NetworkUtils;
 import android.system.OsConstants;
 import android.test.suitebuilder.annotation.SmallTest;
 import com.android.internal.util.HexDump;
-
 import java.net.Inet4Address;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
-
 import junit.framework.TestCase;
-import libcore.util.HexEncoding;
 
 import static android.net.dhcp.DhcpPacket.*;
 
-
 public class DhcpPacketTest extends TestCase {
 
     private static Inet4Address SERVER_ADDR = v4Address("192.0.2.1");
@@ -278,7 +274,7 @@ public class DhcpPacketTest extends TestCase {
         // TODO: Turn all of these into golden files. This will probably require modifying
         // Android.mk appropriately, making this into an AndroidTestCase, and adding code to read
         // the golden files from the test APK's assets via mContext.getAssets().
-        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
             // IP header.
             "451001480000000080118849c0a89003c0a89ff7" +
             // UDP header.
@@ -297,8 +293,7 @@ public class DhcpPacketTest extends TestCase {
             "0000000000000000000000000000000000000000000000000000000000000000" +
             // Options
             "638253633501023604c0a89003330400001c200104fffff0000304c0a89ffe06080808080808080404" +
-            "3a0400000e103b040000189cff00000000000000000000"
-        ).toCharArray(), false));
+            "3a0400000e103b040000189cff00000000000000000000"));
 
         DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3);
         assertTrue(offerPacket instanceof DhcpOfferPacket);  // Implicitly checks it's non-null.
@@ -309,7 +304,7 @@ public class DhcpPacketTest extends TestCase {
 
     @SmallTest
     public void testOffer2() throws Exception {
-        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
             // IP header.
             "450001518d0600004011144dc0a82b01c0a82bf7" +
             // UDP header.
@@ -328,8 +323,7 @@ public class DhcpPacketTest extends TestCase {
             "0000000000000000000000000000000000000000000000000000000000000000" +
             // Options
             "638253633501023604c0a82b01330400000e103a04000007083b0400000c4e0104ffffff00" +
-            "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff"
-        ).toCharArray(), false));
+            "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff"));
 
         assertEquals(337, packet.limit());
         DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3);
@@ -341,8 +335,119 @@ public class DhcpPacketTest extends TestCase {
     }
 
     @SmallTest
+    public void testBadIpPacket() throws Exception {
+        final byte[] packet = HexDump.hexStringToByteArray(
+            // IP header.
+            "450001518d0600004011144dc0a82b01c0a82bf7");
+
+        try {
+            DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, packet.length, ENCAP_L3);
+            fail("Dhcp packet parsing should have failed");
+        } catch (DhcpPacket.ParseException expected) {}
+    }
+
+    @SmallTest
+    public void testBadDhcpPacket() throws Exception {
+        final byte[] packet = HexDump.hexStringToByteArray(
+            // IP header.
+            "450001518d0600004011144dc0a82b01c0a82bf7" +
+            // UDP header.
+            "00430044013d9ac7" +
+            // BOOTP header.
+            "02010600dfc23d1f0002000000000000c0a82bf7c0a82b0100000000");
+
+        try {
+            DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, packet.length, ENCAP_L3);
+            fail("Dhcp packet parsing should have failed");
+        } catch (DhcpPacket.ParseException expected) {}
+    }
+
+    @SmallTest
+    public void testBadTruncatedOffer() throws Exception {
+        final byte[] packet = HexDump.hexStringToByteArray(
+            // IP header.
+            "450001518d0600004011144dc0a82b01c0a82bf7" +
+            // UDP header.
+            "00430044013d9ac7" +
+            // BOOTP header.
+            "02010600dfc23d1f0002000000000000c0a82bf7c0a82b0100000000" +
+            // MAC address.
+            "30766ff2a90c00000000000000000000" +
+            // Server name.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // File, missing one byte
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "00000000000000000000000000000000000000000000000000000000000000");
+
+        try {
+            DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, packet.length, ENCAP_L3);
+            fail("Dhcp packet parsing should have failed");
+        } catch (DhcpPacket.ParseException expected) {}
+    }
+
+    @SmallTest
+    public void testBadOfferWithoutACookie() throws Exception {
+        final byte[] packet = HexDump.hexStringToByteArray(
+            // IP header.
+            "450001518d0600004011144dc0a82b01c0a82bf7" +
+            // UDP header.
+            "00430044013d9ac7" +
+            // BOOTP header.
+            "02010600dfc23d1f0002000000000000c0a82bf7c0a82b0100000000" +
+            // MAC address.
+            "30766ff2a90c00000000000000000000" +
+            // Server name.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // File.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000"
+            // No options
+            );
+
+        try {
+            DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, packet.length, ENCAP_L3);
+            fail("Dhcp packet parsing should have failed");
+        } catch (DhcpPacket.ParseException expected) {}
+    }
+
+    @SmallTest
+    public void testOfferWithBadCookie() throws Exception {
+        final byte[] packet = HexDump.hexStringToByteArray(
+            // IP header.
+            "450001518d0600004011144dc0a82b01c0a82bf7" +
+            // UDP header.
+            "00430044013d9ac7" +
+            // BOOTP header.
+            "02010600dfc23d1f0002000000000000c0a82bf7c0a82b0100000000" +
+            // MAC address.
+            "30766ff2a90c00000000000000000000" +
+            // Server name.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // File.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // Bad cookie
+            "DEADBEEF3501023604c0a82b01330400000e103a04000007083b0400000c4e0104ffffff00" +
+            "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff");
+
+        try {
+            DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, packet.length, ENCAP_L3);
+            fail("Dhcp packet parsing should have failed");
+        } catch (DhcpPacket.ParseException expected) {}
+    }
+
+    @SmallTest
     public void testBadHwaddrLength() throws Exception {
-        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
             // IP header.
             "450001518d0600004011144dc0a82b01c0a82bf7" +
             // UDP header.
@@ -361,8 +466,7 @@ public class DhcpPacketTest extends TestCase {
             "0000000000000000000000000000000000000000000000000000000000000000" +
             // Options
             "638253633501023604c0a82b01330400000e103a04000007083b0400000c4e0104ffffff00" +
-            "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff"
-        ).toCharArray(), false));
+            "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff"));
         String expectedClientMac = "30766FF2A90C";
 
         final int hwAddrLenOffset = 20 + 8 + 2;
@@ -419,7 +523,7 @@ public class DhcpPacketTest extends TestCase {
         //    store any information in the overloaded fields).
         //
         // For now, we just check that it parses correctly.
-        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
             // Ethernet header.
             "b4cef6000000e80462236e300800" +
             // IP header.
@@ -440,8 +544,7 @@ public class DhcpPacketTest extends TestCase {
             "0000000000000000000000000000000000000000000000000000000000000000" +
             // Options
             "638253633501023604010101010104ffff000033040000a8c03401030304ac1101010604ac110101" +
-            "0000000000000000000000000000000000000000000000ff000000"
-        ).toCharArray(), false));
+            "0000000000000000000000000000000000000000000000ff000000"));
 
         DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
         assertTrue(offerPacket instanceof DhcpOfferPacket);
@@ -452,7 +555,7 @@ public class DhcpPacketTest extends TestCase {
 
     @SmallTest
     public void testBug2111() throws Exception {
-        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
             // IP header.
             "4500014c00000000ff119beac3eaf3880a3f5d04" +
             // UDP header. TODO: fix invalid checksum (due to MAC address obfuscation).
@@ -471,8 +574,7 @@ public class DhcpPacketTest extends TestCase {
             "0000000000000000000000000000000000000000000000000000000000000000" +
             // Options.
             "638253633501023604c00002fe33040000bfc60104fffff00003040a3f50010608c0000201c0000202" +
-            "0f0f646f6d61696e3132332e636f2e756b0000000000ff00000000"
-        ).toCharArray(), false));
+            "0f0f646f6d61696e3132332e636f2e756b0000000000ff00000000"));
 
         DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3);
         assertTrue(offerPacket instanceof DhcpOfferPacket);
@@ -483,7 +585,7 @@ public class DhcpPacketTest extends TestCase {
 
     @SmallTest
     public void testBug2136() throws Exception {
-        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
             // Ethernet header.
             "bcf5ac000000d0c7890000000800" +
             // IP header.
@@ -504,8 +606,7 @@ public class DhcpPacketTest extends TestCase {
             "0000000000000000000000000000000000000000000000000000000000000000" +
             // Options.
             "6382536335010236040a20ff80330400001c200104fffff00003040a20900106089458413494584135" +
-            "0f0b6c616e63732e61632e756b000000000000000000ff00000000"
-        ).toCharArray(), false));
+            "0f0b6c616e63732e61632e756b000000000000000000ff00000000"));
 
         DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
         assertTrue(offerPacket instanceof DhcpOfferPacket);
@@ -517,7 +618,7 @@ public class DhcpPacketTest extends TestCase {
 
     @SmallTest
     public void testUdpServerAnySourcePort() throws Exception {
-        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
             // Ethernet header.
             "9cd917000000001c2e0000000800" +
             // IP header.
@@ -539,8 +640,7 @@ public class DhcpPacketTest extends TestCase {
             "0000000000000000000000000000000000000000000000000000000000000000" +
             // Options.
             "6382536335010236040a0169fc3304000151800104ffff000003040a0fc817060cd1818003d1819403" +
-            "d18180060f0777766d2e6564751c040a0fffffff000000"
-        ).toCharArray(), false));
+            "d18180060f0777766d2e6564751c040a0fffffff000000"));
 
         DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
         assertTrue(offerPacket instanceof DhcpOfferPacket);
@@ -552,8 +652,40 @@ public class DhcpPacketTest extends TestCase {
     }
 
     @SmallTest
+    public void testUdpInvalidDstPort() throws Exception {
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
+            // Ethernet header.
+            "9cd917000000001c2e0000000800" +
+            // IP header.
+            "45a00148000040003d115087d18194fb0a0f7af2" +
+            // UDP header. TODO: fix invalid checksum (due to MAC address obfuscation).
+            // NOTE: The destination port is a non-DHCP port.
+            "0043aaaa01341268" +
+            // BOOTP header.
+            "02010600d628ba8200000000000000000a0f7af2000000000a0fc818" +
+            // MAC address.
+            "9cd91700000000000000000000000000" +
+            // Server name.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // File.
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            "0000000000000000000000000000000000000000000000000000000000000000" +
+            // Options.
+            "6382536335010236040a0169fc3304000151800104ffff000003040a0fc817060cd1818003d1819403" +
+            "d18180060f0777766d2e6564751c040a0fffffff000000"));
+
+        try {
+            DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
+            fail("Packet with invalid dst port did not throw ParseException");
+        } catch (ParseException expected) {}
+    }
+
+    @SmallTest
     public void testMultipleRouters() throws Exception {
-        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
             // Ethernet header.
             "fc3d93000000" + "081735000000" + "0800" +
             // IP header.
@@ -574,8 +706,7 @@ public class DhcpPacketTest extends TestCase {
             "0000000000000000000000000000000000000000000000000000000000000000" +
             // Options.
             "638253633501023604c0abbd023304000070803a04000038403b04000062700104ffffff00" +
-            "0308c0a8bd01ffffff0006080808080808080404ff000000000000"
-        ).toCharArray(), false));
+            "0308c0a8bd01ffffff0006080808080808080404ff000000000000"));
 
         DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
         assertTrue(offerPacket instanceof DhcpOfferPacket);