From d64144a37ca0a6f341c0092cc04271831ff5f90d Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 3 Sep 2015 17:36:20 +0900 Subject: [PATCH] Don't crash on (invalid) hardware address lengths > 127. These would cause us to crash with a NegativeArraySizeException when trying to create the clientMac array. Instead, if the length is > 16 (invalid, because the field is only 16 bytes long), fudge it to 6 (Ethernet / wifi). This is a bit less liberal than the legacy client, which doesn't check the length at all. Bug: 23725795 Change-Id: I83f47bfc400ffa8ce85dd9d1b8eb96be5afe51a5 --- .../net/java/android/net/dhcp/DhcpPacket.java | 15 +++- .../src/android/net/dhcp/DhcpPacketTest.java | 70 +++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/services/net/java/android/net/dhcp/DhcpPacket.java b/services/net/java/android/net/dhcp/DhcpPacket.java index a91ddb8e41852..cbf8fc21b809b 100644 --- a/services/net/java/android/net/dhcp/DhcpPacket.java +++ b/services/net/java/android/net/dhcp/DhcpPacket.java @@ -55,6 +55,7 @@ abstract class DhcpPacket { public static final int MIN_PACKET_LENGTH_L3 = MIN_PACKET_LENGTH_BOOTP + 20 + 8; public static final int MIN_PACKET_LENGTH_L2 = MIN_PACKET_LENGTH_L3 + 14; + public static final int HWADDR_LEN = 16; public static final int MAX_OPTION_LEN = 255; /** * IP layer definitions. @@ -399,7 +400,7 @@ abstract class DhcpPacket { buf.put(mRelayIp.getAddress()); buf.put(mClientMac); buf.position(buf.position() + - (16 - mClientMac.length) // pad addr to 16 bytes + (HWADDR_LEN - mClientMac.length) // pad addr to 16 bytes + 64 // empty server host name (64 bytes) + 128); // empty boot file name (128 bytes) buf.putInt(0x63825363); // magic number @@ -786,7 +787,7 @@ abstract class DhcpPacket { byte type = packet.get(); byte hwType = packet.get(); - byte addrLen = packet.get(); + int addrLen = packet.get() & 0xff; byte hops = packet.get(); transactionId = packet.getInt(); secs = packet.getShort(); @@ -807,6 +808,16 @@ abstract class DhcpPacket { return null; } + // Some DHCP servers have been known to announce invalid client hardware address values such + // as 0xff. The legacy DHCP client accepted these becuause it does not check the length at + // all but only checks that the interface MAC address matches the first bytes of the address + // in the packets. We're a bit stricter: if the length is obviously invalid (i.e., bigger + // than the size of the field), we fudge it to 6 (Ethernet). http://b/23725795 + // TODO: evaluate whether to make this test more liberal. + if (addrLen > HWADDR_LEN) { + addrLen = ETHER_BROADCAST.length; + } + clientMac = new byte[addrLen]; packet.get(clientMac); diff --git a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java index da1df1accdb00..cd3b8bb7e0b98 100644 --- a/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java +++ b/services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java @@ -332,6 +332,76 @@ public class DhcpPacketTest extends TestCase { assertTrue(dhcpResults.hasMeteredHint()); } + @SmallTest + public void testBadHwaddrLength() throws Exception { + final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode(( + // IP header. + "450001518d0600004011144dc0a82b01c0a82bf7" + + // UDP header. + "00430044013d9ac7" + + // BOOTP header. + "02010600dfc23d1f0002000000000000c0a82bf7c0a82b0100000000" + + // MAC address. + "30766ff2a90c00000000000000000000" + + // Server name. + "0000000000000000000000000000000000000000000000000000000000000000" + + "0000000000000000000000000000000000000000000000000000000000000000" + + // File. + "0000000000000000000000000000000000000000000000000000000000000000" + + "0000000000000000000000000000000000000000000000000000000000000000" + + "0000000000000000000000000000000000000000000000000000000000000000" + + "0000000000000000000000000000000000000000000000000000000000000000" + + // Options + "638253633501023604c0a82b01330400000e103a04000007083b0400000c4e0104ffffff00" + + "1c04c0a82bff0304c0a82b010604c0a82b012b0f414e44524f49445f4d455445524544ff" + ).toCharArray(), false)); + String expectedClientMac = "30766FF2A90C"; + + final int hwAddrLenOffset = 20 + 8 + 2; + assertEquals(6, packet.get(hwAddrLenOffset)); + + // Expect the expected. + DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3); + assertNotNull(offerPacket); + assertEquals(6, offerPacket.getClientMac().length); + assertEquals(expectedClientMac, HexDump.toHexString(offerPacket.getClientMac())); + + // Reduce the hardware address length and verify that it shortens the client MAC. + packet.flip(); + packet.put(hwAddrLenOffset, (byte) 5); + offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3); + assertNotNull(offerPacket); + assertEquals(5, offerPacket.getClientMac().length); + assertEquals(expectedClientMac.substring(0, 10), + HexDump.toHexString(offerPacket.getClientMac())); + + packet.flip(); + packet.put(hwAddrLenOffset, (byte) 3); + offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3); + assertNotNull(offerPacket); + assertEquals(3, offerPacket.getClientMac().length); + assertEquals(expectedClientMac.substring(0, 6), + HexDump.toHexString(offerPacket.getClientMac())); + + // Set the the hardware address length to 0xff and verify that we a) don't treat it as -1 + // and crash, and b) hardcode it to 6. + packet.flip(); + packet.put(hwAddrLenOffset, (byte) -1); + offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3); + assertNotNull(offerPacket); + assertEquals(6, offerPacket.getClientMac().length); + assertEquals(expectedClientMac, HexDump.toHexString(offerPacket.getClientMac())); + + // Set the the hardware address length to a positive invalid value (> 16) and verify that we + // hardcode it to 6. + packet.flip(); + packet.put(hwAddrLenOffset, (byte) 17); + offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3); + assertNotNull(offerPacket); + assertEquals(6, offerPacket.getClientMac().length); + assertEquals(expectedClientMac, HexDump.toHexString(offerPacket.getClientMac())); + } + @SmallTest public void testPadAndOverloadedOptionsOffer() throws Exception { // A packet observed in the real world that is interesting for two reasons: