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
2016-10-05 18:33:21 +09:00
parent f4dabe3ecf
commit e0ea7fecd6
3 changed files with 171 additions and 44 deletions
--- a/core/java/android/net/metrics/DhcpErrorEvent.java
+++ b/core/java/android/net/metrics/DhcpErrorEvent.java
@@ -53,6 +53,8 @@ public final class DhcpErrorEvent implements Parcelable {

    public static final int BUFFER_UNDERFLOW           = makeErrorCode(MISC_ERROR, 1);
    public static final int RECEIVE_ERROR              = makeErrorCode(MISC_ERROR, 2);
+    /** {@hide} */
+    public static final int PARSING_ERROR              = makeErrorCode(MISC_ERROR, 3);

    public final String ifName;
    // error code byte format (MSB to LSB):
@@ -115,8 +117,9 @@ public final class DhcpErrorEvent implements Parcelable {
    }

    final static class Decoder {
-        static final SparseArray<String> constants =
-                MessageUtils.findMessageNames(new Class[]{DhcpErrorEvent.class},
-                new String[]{"L2_", "L3_", "L4_", "BOOTP_", "DHCP_", "BUFFER_", "RECEIVE_"});
+        static final SparseArray<String> constants = MessageUtils.findMessageNames(
+                new Class[]{DhcpErrorEvent.class},
+                new String[]{"L2_", "L3_", "L4_", "BOOTP_", "DHCP_", "BUFFER_", "RECEIVE_",
+                "PARSING_"});
    }
 }
--- a/services/net/java/android/net/dhcp/DhcpPacket.java
+++ b/services/net/java/android/net/dhcp/DhcpPacket.java
@@ -7,6 +7,7 @@ import android.net.metrics.DhcpErrorEvent;
 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;
@@ -14,9 +15,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;
@@ -725,7 +725,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;
@@ -1090,7 +1091,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(DhcpErrorEvent.PARSING_ERROR, e.getMessage());
+        }
    }

    /**
--- a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
+++ b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
@@ -16,24 +16,21 @@

 package android.net.dhcp;

-import android.net.NetworkUtils;
 import android.net.DhcpResults;
 import android.net.LinkAddress;
+import android.net.NetworkUtils;
+import android.net.metrics.DhcpErrorEvent;
 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 java.util.Arrays;
+import junit.framework.TestCase;

 import static android.net.dhcp.DhcpPacket.*;

-
 public class DhcpPacketTest extends TestCase {

    private static Inet4Address SERVER_ADDR = v4Address("192.0.2.1");
@@ -285,7 +282,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.
@@ -304,8 +301,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.
@@ -316,7 +312,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.
@@ -335,8 +331,7 @@ public class DhcpPacketTest extends TestCase {
            "0000000000000000000000000000000000000000000000000000000000000000" +
            // Options
            "638253633501023604c0a82b01330400000e103a04000007083b0400000c4e0104ffffff00" +
-            "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff"
-        ).toCharArray(), false));
+            "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff"));

        assertEquals(337, packet.limit());
        DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3);
@@ -347,6 +342,136 @@ public class DhcpPacketTest extends TestCase {
        assertTrue(dhcpResults.hasMeteredHint());
    }

+    @SmallTest
+    public void testBadIpPacket() throws Exception {
+        final byte[] packet = HexDump.hexStringToByteArray(
+            // IP header.
+            "450001518d0600004011144dc0a82b01c0a82bf7");
+
+        try {
+            DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, packet.length, ENCAP_L3);
+        } catch (DhcpPacket.ParseException expected) {
+            assertDhcpErrorCodes(DhcpErrorEvent.L3_TOO_SHORT, expected.errorCode);
+            return;
+        }
+        fail("Dhcp packet parsing should have failed");
+    }
+
+    @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);
+        } catch (DhcpPacket.ParseException expected) {
+            assertDhcpErrorCodes(DhcpErrorEvent.L3_TOO_SHORT, expected.errorCode);
+            return;
+        }
+        fail("Dhcp packet parsing should have failed");
+    }
+
+    @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);
+        } catch (DhcpPacket.ParseException expected) {
+            assertDhcpErrorCodes(DhcpErrorEvent.L3_TOO_SHORT, expected.errorCode);
+            return;
+        }
+        fail("Dhcp packet parsing should have failed");
+    }
+
+    @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);
+        } catch (DhcpPacket.ParseException expected) {
+            assertDhcpErrorCodes(DhcpErrorEvent.PARSING_ERROR, expected.errorCode);
+            return;
+        }
+        fail("Dhcp packet parsing should have failed");
+    }
+
+    @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);
+        } catch (DhcpPacket.ParseException expected) {
+            assertDhcpErrorCodes(DhcpErrorEvent.DHCP_BAD_MAGIC_COOKIE, expected.errorCode);
+            return;
+        }
+        fail("Dhcp packet parsing should have failed");
+    }
+
+    private void assertDhcpErrorCodes(int expected, int got) {
+        assertEquals(Integer.toHexString(expected), Integer.toHexString(got));
+    }
+
    private byte[] mtuBytes(int mtu) {
        // 0x1a02: option 26, length 2. 0xff: no more options.
        if (mtu > Short.MAX_VALUE - Short.MIN_VALUE) {
@@ -354,7 +479,7 @@ public class DhcpPacketTest extends TestCase {
                String.format("Invalid MTU %d, must be 16-bit unsigned", mtu));
        }
        String hexString = String.format("1a02%04xff", mtu);
-        return HexEncoding.decode(hexString.toCharArray(), false);
+        return HexDump.hexStringToByteArray(hexString);
    }

    private void checkMtu(ByteBuffer packet, int expectedMtu, byte[] mtuBytes) throws Exception {
@@ -372,7 +497,7 @@ public class DhcpPacketTest extends TestCase {

    @SmallTest
    public void testMtu() throws Exception {
-        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
            // IP header.
            "451001480000000080118849c0a89003c0a89ff7" +
            // UDP header.
@@ -391,8 +516,7 @@ public class DhcpPacketTest extends TestCase {
            "0000000000000000000000000000000000000000000000000000000000000000" +
            // Options
            "638253633501023604c0a89003330400001c200104fffff0000304c0a89ffe06080808080808080404" +
-            "3a0400000e103b040000189cff00000000"
-        ).toCharArray(), false));
+            "3a0400000e103b040000189cff00000000"));

        checkMtu(packet, 0, null);
        checkMtu(packet, 0, mtuBytes(1501));
@@ -409,7 +533,7 @@ public class DhcpPacketTest extends TestCase {

    @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.
@@ -428,8 +552,7 @@ public class DhcpPacketTest extends TestCase {
            "0000000000000000000000000000000000000000000000000000000000000000" +
            // Options
            "638253633501023604c0a82b01330400000e103a04000007083b0400000c4e0104ffffff00" +
-            "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff"
-        ).toCharArray(), false));
+            "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff"));
        String expectedClientMac = "30766FF2A90C";

        final int hwAddrLenOffset = 20 + 8 + 2;
@@ -486,7 +609,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.
@@ -507,8 +630,7 @@ public class DhcpPacketTest extends TestCase {
            "0000000000000000000000000000000000000000000000000000000000000000" +
            // Options
            "638253633501023604010101010104ffff000033040000a8c03401030304ac1101010604ac110101" +
-            "0000000000000000000000000000000000000000000000ff000000"
-        ).toCharArray(), false));
+            "0000000000000000000000000000000000000000000000ff000000"));

        DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
        assertTrue(offerPacket instanceof DhcpOfferPacket);
@@ -519,7 +641,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).
@@ -538,8 +660,7 @@ public class DhcpPacketTest extends TestCase {
            "0000000000000000000000000000000000000000000000000000000000000000" +
            // Options.
            "638253633501023604c00002fe33040000bfc60104fffff00003040a3f50010608c0000201c0000202" +
-            "0f0f646f6d61696e3132332e636f2e756b0000000000ff00000000"
-        ).toCharArray(), false));
+            "0f0f646f6d61696e3132332e636f2e756b0000000000ff00000000"));

        DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3);
        assertTrue(offerPacket instanceof DhcpOfferPacket);
@@ -550,7 +671,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.
@@ -571,8 +692,7 @@ public class DhcpPacketTest extends TestCase {
            "0000000000000000000000000000000000000000000000000000000000000000" +
            // Options.
            "6382536335010236040a20ff80330400001c200104fffff00003040a20900106089458413494584135" +
-            "0f0b6c616e63732e61632e756b000000000000000000ff00000000"
-        ).toCharArray(), false));
+            "0f0b6c616e63732e61632e756b000000000000000000ff00000000"));

        DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
        assertTrue(offerPacket instanceof DhcpOfferPacket);
@@ -584,7 +704,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.
@@ -606,8 +726,7 @@ public class DhcpPacketTest extends TestCase {
            "0000000000000000000000000000000000000000000000000000000000000000" +
            // Options.
            "6382536335010236040a0169fc3304000151800104ffff000003040a0fc817060cd1818003d1819403" +
-            "d18180060f0777766d2e6564751c040a0fffffff000000"
-        ).toCharArray(), false));
+            "d18180060f0777766d2e6564751c040a0fffffff000000"));

        DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
        assertTrue(offerPacket instanceof DhcpOfferPacket);
@@ -620,7 +739,7 @@ public class DhcpPacketTest extends TestCase {

    @SmallTest
    public void testUdpInvalidDstPort() throws Exception {
-        final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
+        final ByteBuffer packet = ByteBuffer.wrap(HexDump.hexStringToByteArray(
            // Ethernet header.
            "9cd917000000001c2e0000000800" +
            // IP header.
@@ -642,8 +761,7 @@ public class DhcpPacketTest extends TestCase {
            "0000000000000000000000000000000000000000000000000000000000000000" +
            // Options.
            "6382536335010236040a0169fc3304000151800104ffff000003040a0fc817060cd1818003d1819403" +
-            "d18180060f0777766d2e6564751c040a0fffffff000000"
-        ).toCharArray(), false));
+            "d18180060f0777766d2e6564751c040a0fffffff000000"));

        try {
            DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
@@ -653,7 +771,7 @@ public class DhcpPacketTest extends TestCase {

    @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.
@@ -674,8 +792,7 @@ public class DhcpPacketTest extends TestCase {
            "0000000000000000000000000000000000000000000000000000000000000000" +
            // Options.
            "638253633501023604c0abbd023304000070803a04000038403b04000062700104ffffff00" +
-            "0308c0a8bd01ffffff0006080808080808080404ff000000000000"
-        ).toCharArray(), false));
+            "0308c0a8bd01ffffff0006080808080808080404ff000000000000"));

        DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
        assertTrue(offerPacket instanceof DhcpOfferPacket);