drgreen/android_frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix two parsing bugs in new DHCP client.

Browse Source 
1. We don't parse PAD options properly, leading in failure to
   parse packets sent by DHCP servers that put the end of options
   marker after pad options and at an odd offset.
2. We get the DhcpResults vendorInfo from the wrong option type
   (60 instead of 43).

Fix these and add unit tests for the offer packets sent by a few
different DHCP servers.

Bug: 21955617
Bug: 22281295
Change-Id: I5d13f1a6a3ff0b53112f18f3db8792fa32ad2da3
This commit is contained in:
Lorenzo Colitti
2015-07-06 12:29:43 +09:00
parent 738a8dfcdc
commit b0b3d0bcfb
2 changed files with 169 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
services/net/java/android/net/dhcp/DhcpPacket.java
View File

@ -154,6 +154,12 @@ abstract class DhcpPacket {
protected static final byte DHCP_BROADCAST_ADDRESS = 28; protected static final byte DHCP_BROADCAST_ADDRESS = 28;
protected Inet4Address mBroadcastAddress; protected Inet4Address mBroadcastAddress;
/**
* DHCP Optional Type: Vendor specific information
*/
protected static final byte DHCP_VENDOR_INFO = 43;
protected String mVendorInfo;
/** /**
* DHCP Optional Type: DHCP Requested IP Address * DHCP Optional Type: DHCP Requested IP Address
*/ */
@ -226,6 +232,16 @@ abstract class DhcpPacket {
*/ */
protected static final byte DHCP_CLIENT_IDENTIFIER = 61; protected static final byte DHCP_CLIENT_IDENTIFIER = 61;
/**
* DHCP zero-length option code: pad
*/
protected static final byte DHCP_OPTION_PAD = 0x00;
/**
* DHCP zero-length option code: end of options
*/
protected static final byte DHCP_OPTION_END = (byte) 0xff;
/** /**
* The transaction identifier used in this particular DHCP negotiation * The transaction identifier used in this particular DHCP negotiation
*/ */
@ -676,6 +692,7 @@ abstract class DhcpPacket {
Inet4Address netMask = null; Inet4Address netMask = null;
String message = null; String message = null;
String vendorId = null; String vendorId = null;
String vendorInfo = null;
byte[] expectedParams = null; byte[] expectedParams = null;
String hostName = null; String hostName = null;
String domainName = null; String domainName = null;
@ -810,8 +827,10 @@ abstract class DhcpPacket {
try { try {
byte optionType = packet.get(); byte optionType = packet.get();
if (optionType == (byte) 0xFF) { if (optionType == DHCP_OPTION_END) {
notFinishedOptions = false; notFinishedOptions = false;
} else if (optionType == DHCP_OPTION_PAD) {
// The pad option doesn't have a length field. Nothing to do.
} else { } else {
int optionLen = packet.get() & 0xFF; int optionLen = packet.get() & 0xFF;
int expectedLen = 0; int expectedLen = 0;
@ -885,6 +904,7 @@ abstract class DhcpPacket {
break; break;
case DHCP_VENDOR_CLASS_ID: case DHCP_VENDOR_CLASS_ID:
expectedLen = optionLen; expectedLen = optionLen;
// Embedded nulls are safe as this does not get passed to netd.
vendorId = readAsciiString(packet, optionLen, true); vendorId = readAsciiString(packet, optionLen, true);
break; break;
case DHCP_CLIENT_IDENTIFIER: { // Client identifier case DHCP_CLIENT_IDENTIFIER: { // Client identifier
@ -892,6 +912,11 @@ abstract class DhcpPacket {
packet.get(id); packet.get(id);
expectedLen = optionLen; expectedLen = optionLen;
} break; } break;
case DHCP_VENDOR_INFO:
expectedLen = optionLen;
// Embedded nulls are safe as this does not get passed to netd.
vendorInfo = readAsciiString(packet, optionLen, true);
break;
default: default:
// ignore any other parameters // ignore any other parameters
for (int i = 0; i < optionLen; i++) { for (int i = 0; i < optionLen; i++) {
@ -965,6 +990,7 @@ abstract class DhcpPacket {
newPacket.mT1 = T1; newPacket.mT1 = T1;
newPacket.mT2 = T2; newPacket.mT2 = T2;
newPacket.mVendorId = vendorId; newPacket.mVendorId = vendorId;
newPacket.mVendorInfo = vendorInfo;
return newPacket; return newPacket;
} }
@ -1011,7 +1037,7 @@ abstract class DhcpPacket {
results.dnsServers.addAll(mDnsServers); results.dnsServers.addAll(mDnsServers);
results.domains = mDomainName; results.domains = mDomainName;
results.serverAddress = mServerIdentifier; results.serverAddress = mServerIdentifier;
results.vendorInfo = mVendorId; results.vendorInfo = mVendorInfo;
results.leaseDuration = (mLeaseTime != null) ? mLeaseTime : INFINITE_LEASE; results.leaseDuration = (mLeaseTime != null) ? mLeaseTime : INFINITE_LEASE;
return results; return results;
} }

150
services/tests/servicestests/src/android/net/dhcp/DhcpPacketTest.java
View File

@ -25,22 +25,27 @@ import junit.framework.TestCase;
import java.net.Inet4Address; import java.net.Inet4Address;
import java.nio.ByteBuffer; import java.nio.ByteBuffer;
import java.util.ArrayList;
import libcore.util.HexEncoding;
import static android.net.dhcp.DhcpPacket.*; import static android.net.dhcp.DhcpPacket.*;
public class DhcpPacketTest extends TestCase { public class DhcpPacketTest extends TestCase {
private static Inet4Address SERVER_ADDR = private static Inet4Address SERVER_ADDR = v4Address("192.0.2.1");
(Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.1"); private static Inet4Address CLIENT_ADDR = v4Address("192.0.2.234");
private static Inet4Address CLIENT_ADDR =
(Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.234");
// Use our own empty address instead of Inet4Address.ANY or INADDR_ANY to ensure that the code // Use our own empty address instead of Inet4Address.ANY or INADDR_ANY to ensure that the code
// doesn't use == instead of equals when comparing addresses. // doesn't use == instead of equals when comparing addresses.
private static Inet4Address ANY = (Inet4Address) NetworkUtils.numericToInetAddress("0.0.0.0"); private static Inet4Address ANY = (Inet4Address) v4Address("0.0.0.0");
private static byte[] CLIENT_MAC = new byte[] { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05 }; private static byte[] CLIENT_MAC = new byte[] { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05 };
private static final Inet4Address v4Address(String addrString) throws IllegalArgumentException {
return (Inet4Address) NetworkUtils.numericToInetAddress(addrString);
}
class TestDhcpPacket extends DhcpPacket { class TestDhcpPacket extends DhcpPacket {
private byte mType; private byte mType;
// TODO: Make this a map of option numbers to bytes instead. // TODO: Make this a map of option numbers to bytes instead.
@ -89,7 +94,7 @@ public class DhcpPacketTest extends TestCase {
addTlv(buffer, DHCP_DOMAIN_NAME, mDomainBytes); addTlv(buffer, DHCP_DOMAIN_NAME, mDomainBytes);
} }
if (mVendorInfoBytes != null) { if (mVendorInfoBytes != null) {
addTlv(buffer, DHCP_VENDOR_CLASS_ID, mVendorInfoBytes); addTlv(buffer, DHCP_VENDOR_INFO, mVendorInfoBytes);
} }
if (mLeaseTimeBytes != null) { if (mLeaseTimeBytes != null) {
addTlv(buffer, DHCP_LEASE_TIME, mLeaseTimeBytes); addTlv(buffer, DHCP_LEASE_TIME, mLeaseTimeBytes);
@ -118,7 +123,7 @@ public class DhcpPacketTest extends TestCase {
.build(); .build();
DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_BOOTP); DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_BOOTP);
assertEquals(expectedDomain, offerPacket.mDomainName); assertEquals(expectedDomain, offerPacket.mDomainName);
assertEquals(expectedVendorInfo, offerPacket.mVendorId); assertEquals(expectedVendorInfo, offerPacket.mVendorInfo);
} }
@SmallTest @SmallTest
@ -221,8 +226,8 @@ public class DhcpPacketTest extends TestCase {
byte[] slash11Netmask = new byte[] { (byte) 0xff, (byte) 0xe0, 0x00, 0x00 }; byte[] slash11Netmask = new byte[] { (byte) 0xff, (byte) 0xe0, 0x00, 0x00 };
byte[] slash24Netmask = new byte[] { (byte) 0xff, (byte) 0xff, (byte) 0xff, 0x00 }; byte[] slash24Netmask = new byte[] { (byte) 0xff, (byte) 0xff, (byte) 0xff, 0x00 };
byte[] invalidNetmask = new byte[] { (byte) 0xff, (byte) 0xfb, (byte) 0xff, 0x00 }; byte[] invalidNetmask = new byte[] { (byte) 0xff, (byte) 0xfb, (byte) 0xff, 0x00 };
Inet4Address example1 = (Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.1"); Inet4Address example1 = v4Address("192.0.2.1");
Inet4Address example2 = (Inet4Address) NetworkUtils.numericToInetAddress("192.0.2.43"); Inet4Address example2 = v4Address("192.0.2.43");
// A packet without any addresses is not valid. // A packet without any addresses is not valid.
checkIpAddress(null, ANY, ANY, slash24Netmask); checkIpAddress(null, ANY, ANY, slash24Netmask);
@ -238,4 +243,131 @@ public class DhcpPacketTest extends TestCase {
// If there is no netmask, implicit netmasks are used. // If there is no netmask, implicit netmasks are used.
checkIpAddress("192.0.2.43/24", ANY, example2, null); checkIpAddress("192.0.2.43/24", ANY, example2, null);
} }
private void assertDhcpResults(String ipAddress, String gateway, String dnsServersString,
String domains, String serverAddress, String vendorInfo, int leaseDuration,
boolean hasMeteredHint, DhcpResults dhcpResults) throws Exception {
assertEquals(new LinkAddress(ipAddress), dhcpResults.ipAddress);
assertEquals(v4Address(gateway), dhcpResults.gateway);
String[] dnsServerStrings = dnsServersString.split(",");
ArrayList dnsServers = new ArrayList();
for (String dnsServerString : dnsServerStrings) {
dnsServers.add(v4Address(dnsServerString));
}
assertEquals(dnsServers, dhcpResults.dnsServers);
assertEquals(domains, dhcpResults.domains);
assertEquals(v4Address(serverAddress), dhcpResults.serverAddress);
assertEquals(vendorInfo, dhcpResults.vendorInfo);
assertEquals(leaseDuration, dhcpResults.leaseDuration);
assertEquals(hasMeteredHint, dhcpResults.hasMeteredHint());
}
@SmallTest
public void testOffer1() throws Exception {
// 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((
// IP header.
"451001480000000080118849c0a89003c0a89ff7" +
// UDP header.
"004300440134dcfa" +
// BOOTP header.
"02010600c997a63b0000000000000000c0a89ff70000000000000000" +
// MAC address.
"30766ff2a90c00000000000000000000" +
// Server name.
"0000000000000000000000000000000000000000000000000000000000000000" +
"0000000000000000000000000000000000000000000000000000000000000000" +
// File.
"0000000000000000000000000000000000000000000000000000000000000000" +
"0000000000000000000000000000000000000000000000000000000000000000" +
"0000000000000000000000000000000000000000000000000000000000000000" +
"0000000000000000000000000000000000000000000000000000000000000000" +
// Options
"638253633501023604c0a89003330400001c200104fffff0000304c0a89ffe06080808080808080404" +
"3a0400000e103b040000189cff00000000000000000000"
).toCharArray(), false));
DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3);
assertTrue(offerPacket instanceof DhcpOfferPacket); // Implicitly checks it's non-null.
DhcpResults dhcpResults = offerPacket.toDhcpResults();
assertDhcpResults("192.168.159.247/20", "192.168.159.254", "8.8.8.8,8.8.4.4",
null, "192.168.144.3", null, 7200, false, dhcpResults);
}
@SmallTest
public void testOffer2() 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));
assertEquals(337, packet.limit());
DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L3);
assertTrue(offerPacket instanceof DhcpOfferPacket); // Implicitly checks it's non-null.
DhcpResults dhcpResults = offerPacket.toDhcpResults();
assertDhcpResults("192.168.43.247/24", "192.168.43.1", "192.168.43.1",
null, "192.168.43.1", "ANDROID_METERED", 3600, true, dhcpResults);
assertTrue(dhcpResults.hasMeteredHint());
}
@SmallTest
public void testPadAndOverloadedOptionsOffer() throws Exception {
// A packet observed in the real world that is interesting for two reasons:
//
// 1. It uses pad bytes, which we previously didn't support correctly.
// 2. It uses DHCP option overloading, which we don't currently support (but it doesn't
// store any information in the overloaded fields).
//
// For now, we just check that it parses correctly.
final ByteBuffer packet = ByteBuffer.wrap(HexEncoding.decode((
// Ethernet header.
"b4cef6000000e80462236e300800" +
// IP header.
"4500014c00000000ff11741701010101ac119876" +
// UDP header. TODO: fix invalid checksum (due to MAC address obfuscation).
"004300440138ae5a" +
// BOOTP header.
"020106000fa0059f0000000000000000ac1198760000000000000000" +
// MAC address.
"b4cef600000000000000000000000000" +
// Server name.
"ff00000000000000000000000000000000000000000000000000000000000000" +
"0000000000000000000000000000000000000000000000000000000000000000" +
// File.
"ff00000000000000000000000000000000000000000000000000000000000000" +
"0000000000000000000000000000000000000000000000000000000000000000" +
"0000000000000000000000000000000000000000000000000000000000000000" +
"0000000000000000000000000000000000000000000000000000000000000000" +
// Options
"638253633501023604010101010104ffff000033040000a8c03401030304ac1101010604ac110101" +
"0000000000000000000000000000000000000000000000ff000000"
).toCharArray(), false));
DhcpPacket offerPacket = DhcpPacket.decodeFullPacket(packet, ENCAP_L2);
assertTrue(offerPacket instanceof DhcpOfferPacket);
DhcpResults dhcpResults = offerPacket.toDhcpResults();
assertDhcpResults("172.17.152.118/16", "172.17.1.1", "172.17.1.1",
null, "1.1.1.1", null, 43200, false, dhcpResults);
}
} }