From 006e0613016c1a0e0627f992f5a93a7b7198edba Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Wed, 5 Oct 2016 21:07:19 +0900 Subject: [PATCH] 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 --- .../android/net/metrics/DhcpErrorEvent.java | 2 + .../net/java/android/net/dhcp/DhcpPacket.java | 6 ++- .../src/android/net/dhcp/DhcpPacketTest.java | 52 ++++++++++++++++++- 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/core/java/android/net/metrics/DhcpErrorEvent.java b/core/java/android/net/metrics/DhcpErrorEvent.java index 1972b9ea8a779..c3abcf7610b4e 100644 --- a/core/java/android/net/metrics/DhcpErrorEvent.java +++ b/core/java/android/net/metrics/DhcpErrorEvent.java @@ -50,6 +50,8 @@ public final class DhcpErrorEvent implements Parcelable { public static final int DHCP_INVALID_OPTION_LENGTH = makeErrorCode(DHCP_ERROR, 3); public static final int DHCP_NO_MSG_TYPE = makeErrorCode(DHCP_ERROR, 4); public static final int DHCP_UNKNOWN_MSG_TYPE = makeErrorCode(DHCP_ERROR, 5); + /** {@hide} */ + public static final int DHCP_NO_COOKIE = makeErrorCode(DHCP_ERROR, 6); public static final int BUFFER_UNDERFLOW = makeErrorCode(MISC_ERROR, 1); public static final int RECEIVE_ERROR = makeErrorCode(MISC_ERROR, 2); diff --git a/services/net/java/android/net/dhcp/DhcpPacket.java b/services/net/java/android/net/dhcp/DhcpPacket.java index fc7cf2e204f0b..ef4bc024c1533 100644 --- a/services/net/java/android/net/dhcp/DhcpPacket.java +++ b/services/net/java/android/net/dhcp/DhcpPacket.java @@ -895,8 +895,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(DhcpErrorEvent.DHCP_NO_COOKIE, "not a DHCP message"); + } + int dhcpMagicCookie = packet.getInt(); if (dhcpMagicCookie != DHCP_MAGIC_COOKIE) { throw new ParseException(DhcpErrorEvent.DHCP_BAD_MAGIC_COOKIE, "Bad magic cookie 0x%08x, should be 0x%08x", diff --git a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java index b4e9db77f99f9..bc8baa12a45b4 100644 --- a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java +++ b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java @@ -27,6 +27,7 @@ import java.net.Inet4Address; import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Arrays; +import java.util.Random; import junit.framework.TestCase; import static android.net.dhcp.DhcpPacket.*; @@ -430,7 +431,7 @@ public class DhcpPacketTest extends TestCase { try { DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, packet.length, ENCAP_L3); } catch (DhcpPacket.ParseException expected) { - assertDhcpErrorCodes(DhcpErrorEvent.PARSING_ERROR, expected.errorCode); + assertDhcpErrorCodes(DhcpErrorEvent.DHCP_NO_COOKIE, expected.errorCode); return; } fail("Dhcp packet parsing should have failed"); @@ -472,6 +473,55 @@ public class DhcpPacketTest extends TestCase { assertEquals(Integer.toHexString(expected), Integer.toHexString(got)); } + public void testTruncatedOfferPackets() 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" + + // Options + "638253633501023604c0a82b01330400000e103a04000007083b0400000c4e0104ffffff00" + + "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff"); + + for (int len = 0; len < packet.length; len++) { + try { + DhcpPacket.decodeFullPacket(packet, len, ENCAP_L3); + } catch (ParseException e) { + if (e.errorCode == DhcpErrorEvent.PARSING_ERROR) { + fail(String.format("bad truncated packet of length %d", len)); + } + } + } + } + + public void testRandomPackets() throws Exception { + final int maxRandomPacketSize = 512; + final Random r = new Random(); + for (int i = 0; i < 10000; i++) { + byte[] packet = new byte[r.nextInt(maxRandomPacketSize + 1)]; + r.nextBytes(packet); + try { + DhcpPacket.decodeFullPacket(packet, packet.length, ENCAP_L3); + } catch (ParseException e) { + if (e.errorCode == DhcpErrorEvent.PARSING_ERROR) { + fail("bad packet: " + HexDump.toHexString(packet)); + } + } + } + } + private byte[] mtuBytes(int mtu) { // 0x1a02: option 26, length 2. 0xff: no more options. if (mtu > Short.MAX_VALUE - Short.MIN_VALUE) {