From e0ea7fecd63002ac2d3df18490fd1c16578d1935 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Wed, 5 Oct 2016 18:33:21 +0900 Subject: [PATCH] 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 --- core/java/android/net/metrics/DhcpErrorEvent.java | 9 +- services/net/java/android/net/dhcp/DhcpPacket.java | 15 +- .../src/android/net/dhcp/DhcpPacketTest.java | 191 +++++++++++++++++---- 3 files changed, 171 insertions(+), 44 deletions(-) diff --git a/core/java/android/net/metrics/DhcpErrorEvent.java b/core/java/android/net/metrics/DhcpErrorEvent.java index 59c5fb61b211..1972b9ea8a77 100644 --- 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 constants = - MessageUtils.findMessageNames(new Class[]{DhcpErrorEvent.class}, - new String[]{"L2_", "L3_", "L4_", "BOOTP_", "DHCP_", "BUFFER_", "RECEIVE_"}); + static final SparseArray constants = MessageUtils.findMessageNames( + new Class[]{DhcpErrorEvent.class}, + new String[]{"L2_", "L3_", "L4_", "BOOTP_", "DHCP_", "BUFFER_", "RECEIVE_", + "PARSING_"}); } } diff --git a/services/net/java/android/net/dhcp/DhcpPacket.java b/services/net/java/android/net/dhcp/DhcpPacket.java index 9aa66fe0ffa8..fc7cf2e204f0 100644 --- 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()); + } } /** diff --git a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java index f8eaf7d0ee26..b4e9db77f99f 100644 --- 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); -- 2.11.0