joy/android_packages_modules_Connectivity
Watch
1
Fork
You've already forked android_packages_modules_Connectivity
 0
Code Issues Pull requests Projects Releases Wiki Activity

Limit the label count when parsing a mDns packet

Browse source 
If some bytes in the mDNS packet are incorrect, such as the
service name, the MdnsPacketReader will enter an infinite loop
and crash with an out-of-memory error when parsing the data. This
issue can be resolved by limiting the label count when parsing
the mDNS packet. If the label count exceeds the limit, the packet
should be discarded as it is likely corrupted.

Bug: 307475137
Test: Manually test with problematic mnds packet raw data
      atest FrameworksNetTests android.net.cts.NsdManagerTest
Ignore-AOSP-First: Security fix.
Change-Id: I48b1dc26f41549ec4afc71f87e98a02ac773430f
This commit is contained in:
Paul Hu 2023-11-01 16:32:45 +08:00
commit 8813f74f52
12 changed files with 111 additions and 26 deletions
Download patch file Download diff file Expand all files Collapse all files

4
service-t/src/com/android/server/NsdService.java
View file

@ -1647,10 +1647,12 @@ public class NsdService extends INsdManager.Stub {
mContext, MdnsFeatureFlags.INCLUDE_INET_ADDRESS_RECORDS_IN_PROBING))
.setIsExpiredServicesRemovalEnabled(mDeps.isTrunkStableFeatureEnabled(
MdnsFeatureFlags.NSD_EXPIRED_SERVICES_REMOVAL))
.setIsLabelCountLimitEnabled(mDeps.isTetheringFeatureNotChickenedOut(
mContext, MdnsFeatureFlags.NSD_LIMIT_LABEL_COUNT))
.build();
mMdnsSocketClient =
new MdnsMultinetworkSocketClient(handler.getLooper(), mMdnsSocketProvider,
LOGGER.forSubComponent("MdnsMultinetworkSocketClient"));
LOGGER.forSubComponent("MdnsMultinetworkSocketClient"), flags);
mMdnsDiscoveryManager = deps.makeMdnsDiscoveryManager(new ExecutorProvider(),
mMdnsSocketClient, LOGGER.forSubComponent("MdnsDiscoveryManager"), flags);
handler.post(() -> mMdnsSocketClient.setCallback(mMdnsDiscoveryManager));

29
service-t/src/com/android/server/connectivity/mdns/MdnsFeatureFlags.java
View file

@ -36,6 +36,11 @@ public class MdnsFeatureFlags {
public static final String NSD_EXPIRED_SERVICES_REMOVAL =
"nsd_expired_services_removal";
/**
* A feature flag to control whether the label count limit should be enabled.
*/
public static final String NSD_LIMIT_LABEL_COUNT = "nsd_limit_label_count";
// Flag for offload feature
public final boolean mIsMdnsOffloadFeatureEnabled;
@ -45,14 +50,20 @@ public class MdnsFeatureFlags {
// Flag for expired services removal
public final boolean mIsExpiredServicesRemovalEnabled;
// Flag for label count limit
public final boolean mIsLabelCountLimitEnabled;
/**
* The constructor for {@link MdnsFeatureFlags}.
*/
public MdnsFeatureFlags(boolean isOffloadFeatureEnabled,
boolean includeInetAddressRecordsInProbing, boolean isExpiredServicesRemovalEnabled) {
boolean includeInetAddressRecordsInProbing,
boolean isExpiredServicesRemovalEnabled,
boolean isLabelCountLimitEnabled) {
mIsMdnsOffloadFeatureEnabled = isOffloadFeatureEnabled;
mIncludeInetAddressRecordsInProbing = includeInetAddressRecordsInProbing;
mIsExpiredServicesRemovalEnabled = isExpiredServicesRemovalEnabled;
mIsLabelCountLimitEnabled = isLabelCountLimitEnabled;
}
@ -67,6 +78,7 @@ public class MdnsFeatureFlags {
private boolean mIsMdnsOffloadFeatureEnabled;
private boolean mIncludeInetAddressRecordsInProbing;
private boolean mIsExpiredServicesRemovalEnabled;
private boolean mIsLabelCountLimitEnabled;
/**
* The constructor for {@link Builder}.
@ -75,6 +87,7 @@ public class MdnsFeatureFlags {
mIsMdnsOffloadFeatureEnabled = false;
mIncludeInetAddressRecordsInProbing = false;
mIsExpiredServicesRemovalEnabled = true; // Default enabled.
mIsLabelCountLimitEnabled = true; // Default enabled.
}
/**
@ -108,12 +121,24 @@ public class MdnsFeatureFlags {
return this;
}
/**
* Set whether the label count limit is enabled.
*
* @see #NSD_LIMIT_LABEL_COUNT
*/
public Builder setIsLabelCountLimitEnabled(boolean isLabelCountLimitEnabled) {
mIsLabelCountLimitEnabled = isLabelCountLimitEnabled;
return this;
}
/**
* Builds a {@link MdnsFeatureFlags} with the arguments supplied to this builder.
*/
public MdnsFeatureFlags build() {
return new MdnsFeatureFlags(mIsMdnsOffloadFeatureEnabled,
mIncludeInetAddressRecordsInProbing, mIsExpiredServicesRemovalEnabled);
mIncludeInetAddressRecordsInProbing,
mIsExpiredServicesRemovalEnabled,
mIsLabelCountLimitEnabled);
}
}
}

6
service-t/src/com/android/server/connectivity/mdns/MdnsInterfaceAdvertiser.java
View file

@ -65,11 +65,12 @@ public class MdnsInterfaceAdvertiser implements MulticastPacketReader.PacketHand
private final MdnsProber mProber;
@NonNull
private final MdnsReplySender mReplySender;
@NonNull
private final SharedLog mSharedLog;
@NonNull
private final byte[] mPacketCreationBuffer;
@NonNull
private final MdnsFeatureFlags mMdnsFeatureFlags;
/**
* Callbacks called by {@link MdnsInterfaceAdvertiser} to report status updates.
@ -213,6 +214,7 @@ public class MdnsInterfaceAdvertiser implements MulticastPacketReader.PacketHand
mProber = deps.makeMdnsProber(sharedLog.getTag(), looper, mReplySender, mProbingCallback,
sharedLog);
mSharedLog = sharedLog;
mMdnsFeatureFlags = mdnsFeatureFlags;
}
/**
@ -351,7 +353,7 @@ public class MdnsInterfaceAdvertiser implements MulticastPacketReader.PacketHand
public void handlePacket(byte[] recvbuf, int length, InetSocketAddress src) {
final MdnsPacket packet;
try {
packet = MdnsPacket.parse(new MdnsPacketReader(recvbuf, length));
packet = MdnsPacket.parse(new MdnsPacketReader(recvbuf, length, mMdnsFeatureFlags));
} catch (MdnsPacket.ParseException e) {
mSharedLog.e("Error parsing mDNS packet", e);
if (DBG) {

8
service-t/src/com/android/server/connectivity/mdns/MdnsMultinetworkSocketClient.java
View file

@ -50,6 +50,7 @@ public class MdnsMultinetworkSocketClient implements MdnsSocketClientBase {
@NonNull private final Handler mHandler;
@NonNull private final MdnsSocketProvider mSocketProvider;
@NonNull private final SharedLog mSharedLog;
@NonNull private final MdnsFeatureFlags mMdnsFeatureFlags;
private final ArrayMap<MdnsServiceBrowserListener, InterfaceSocketCallback> mSocketRequests =
new ArrayMap<>();
@ -58,11 +59,12 @@ public class MdnsMultinetworkSocketClient implements MdnsSocketClientBase {
private int mReceivedPacketNumber = 0;
public MdnsMultinetworkSocketClient(@NonNull Looper looper,
@NonNull MdnsSocketProvider provider,
@NonNull SharedLog sharedLog) {
@NonNull MdnsSocketProvider provider, @NonNull SharedLog sharedLog,
@NonNull MdnsFeatureFlags mdnsFeatureFlags) {
mHandler = new Handler(looper);
mSocketProvider = provider;
mSharedLog = sharedLog;
mMdnsFeatureFlags = mdnsFeatureFlags;
}
private class InterfaceSocketCallback implements MdnsSocketProvider.SocketCallback {
@ -239,7 +241,7 @@ public class MdnsMultinetworkSocketClient implements MdnsSocketClientBase {
final MdnsPacket response;
try {
response = MdnsResponseDecoder.parseResponse(recvbuf, length);
response = MdnsResponseDecoder.parseResponse(recvbuf, length, mMdnsFeatureFlags);
} catch (MdnsPacket.ParseException e) {
if (e.code != MdnsResponseErrorCode.ERROR_NOT_RESPONSE_MESSAGE) {
mSharedLog.e(e.getMessage(), e);

20
service-t/src/com/android/server/connectivity/mdns/MdnsPacketReader.java
View file

@ -16,6 +16,7 @@
package com.android.server.connectivity.mdns;
import android.annotation.NonNull;
import android.annotation.Nullable;
import android.util.SparseArray;
@ -30,24 +31,31 @@ import java.util.Locale;
/** Simple decoder for mDNS packets. */
public class MdnsPacketReader {
// The total length in bytes should be less than 255 bytes anyway (including labels and label
// length) per RFC9267, so limit the number of labels to 128 (each label is 2 bytes with the
// length).
// https://www.rfc-editor.org/rfc/rfc9267.html#name-label-and-name-length-valid
private static final int LABEL_COUNT_LIMIT = 128;
private final byte[] buf;
private final int count;
private final SparseArray<LabelEntry> labelDictionary;
private final MdnsFeatureFlags mMdnsFeatureFlags;
private int pos;
private int limit;
/** Constructs a reader for the given packet. */
public MdnsPacketReader(DatagramPacket packet) {
this(packet.getData(), packet.getLength());
this(packet.getData(), packet.getLength(), MdnsFeatureFlags.newBuilder().build());
}
/** Constructs a reader for the given packet. */
public MdnsPacketReader(byte[] buffer, int length) {
public MdnsPacketReader(byte[] buffer, int length, @NonNull MdnsFeatureFlags mdnsFeatureFlags) {
buf = buffer;
count = length;
pos = 0;
limit = -1;
labelDictionary = new SparseArray<>(16);
mMdnsFeatureFlags = mdnsFeatureFlags;
}
/**
@ -137,6 +145,7 @@ public class MdnsPacketReader {
public String[] readLabels() throws IOException {
List<String> result = new ArrayList<>(5);
LabelEntry previousEntry = null;
int tracingHops = 0;
while (getRemaining() > 0) {
byte nextByte = peekByte();
@ -161,6 +170,10 @@ public class MdnsPacketReader {
// Follow the chain of labels starting at this pointer, adding all of them onto the
// result.
while (labelOffset != 0) {
if (mMdnsFeatureFlags.mIsLabelCountLimitEnabled
&& tracingHops > LABEL_COUNT_LIMIT) {
throw new IOException("Invalid MDNS response packet: Too many labels.");
}
LabelEntry entry = labelDictionary.get(labelOffset);
if (entry == null) {
throw new IOException(
@ -169,6 +182,7 @@ public class MdnsPacketReader {
}
result.add(entry.label);
labelOffset = entry.nextOffset;
tracingHops++;
}
break;
} else {
@ -269,4 +283,4 @@ public class MdnsPacketReader {
this.label = label;
}
}
}
}

6
service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java
View file

@ -84,9 +84,9 @@ public class MdnsResponseDecoder {
* @throws MdnsPacket.ParseException if a response packet could not be parsed.
*/
@NonNull
public static MdnsPacket parseResponse(@NonNull byte[] recvbuf, int length)
throws MdnsPacket.ParseException {
MdnsPacketReader reader = new MdnsPacketReader(recvbuf, length);
public static MdnsPacket parseResponse(@NonNull byte[] recvbuf, int length,
@NonNull MdnsFeatureFlags mdnsFeatureFlags) throws MdnsPacket.ParseException {
final MdnsPacketReader reader = new MdnsPacketReader(recvbuf, length, mdnsFeatureFlags);
final MdnsPacket mdnsPacket;
try {

7
service-t/src/com/android/server/connectivity/mdns/MdnsSocketClient.java
View file

@ -105,9 +105,10 @@ public class MdnsSocketClient implements MdnsSocketClientBase {
private AtomicInteger packetsCount;
@Nullable private Timer checkMulticastResponseTimer;
private final SharedLog sharedLog;
@NonNull private final MdnsFeatureFlags mdnsFeatureFlags;
public MdnsSocketClient(@NonNull Context context, @NonNull MulticastLock multicastLock,
SharedLog sharedLog) {
SharedLog sharedLog, @NonNull MdnsFeatureFlags mdnsFeatureFlags) {
this.sharedLog = sharedLog;
this.context = context;
this.multicastLock = multicastLock;
@ -116,6 +117,7 @@ public class MdnsSocketClient implements MdnsSocketClientBase {
} else {
unicastReceiverBuffer = null;
}
this.mdnsFeatureFlags = mdnsFeatureFlags;
}
@Override
@ -454,7 +456,8 @@ public class MdnsSocketClient implements MdnsSocketClientBase {
final MdnsPacket response;
try {
response = MdnsResponseDecoder.parseResponse(packet.getData(), packet.getLength());
response = MdnsResponseDecoder.parseResponse(
packet.getData(), packet.getLength(), mdnsFeatureFlags);
} catch (MdnsPacket.ParseException e) {
sharedLog.w(String.format("Error while decoding %s packet (%d): %d",
responseType, packetNumber, e.code));

4
tests/unit/java/com/android/server/connectivity/mdns/MdnsMultinetworkSocketClientTest.java
View file

@ -82,8 +82,8 @@ public class MdnsMultinetworkSocketClientTest {
mHandlerThread.start();
mHandler = new Handler(mHandlerThread.getLooper());
mSocketKey = new SocketKey(1000 /* interfaceIndex */);
mSocketClient = new MdnsMultinetworkSocketClient(
mHandlerThread.getLooper(), mProvider, mSharedLog);
mSocketClient = new MdnsMultinetworkSocketClient(mHandlerThread.getLooper(), mProvider,
mSharedLog, MdnsFeatureFlags.newBuilder().build());
mHandler.post(() -> mSocketClient.setCallback(mCallback));
}

19
tests/unit/java/com/android/server/connectivity/mdns/MdnsPacketReaderTests.java
View file

@ -19,8 +19,10 @@ package com.android.server.connectivity.mdns;
import static com.android.testutils.DevSdkIgnoreRuleKt.SC_V2;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.fail;
import com.android.net.module.util.HexDump;
import com.android.testutils.DevSdkIgnoreRule;
import com.android.testutils.DevSdkIgnoreRunner;
@ -75,7 +77,7 @@ public class MdnsPacketReaderTests {
+ "the packet length");
} catch (IOException e) {
// Expected
} catch (Exception e) {
} catch (RuntimeException e) {
fail(String.format(
Locale.ROOT,
"Should not have thrown any other exception except " + "for IOException: %s",
@ -83,4 +85,17 @@ public class MdnsPacketReaderTests {
}
assertEquals(data.length, packetReader.getRemaining());
}
}
@Test
public void testInfinitePtrLoop() {
// Fake mdns response packet label portion which has infinite ptr loop.
final byte[] infinitePtrLoopData = HexDump.hexStringToByteArray(
"054C4142454C" // label "LABEL"
+ "0454455354" // label "TEST"
+ "C006"); // PTR to second label.
MdnsPacketReader packetReader = new MdnsPacketReader(
infinitePtrLoopData, infinitePtrLoopData.length,
MdnsFeatureFlags.newBuilder().setIsLabelCountLimitEnabled(true).build());
assertThrows(IOException.class, packetReader::readLabels);
}
}

19
tests/unit/java/com/android/server/connectivity/mdns/MdnsPacketTest.kt
View file

@ -21,12 +21,16 @@ import com.android.net.module.util.HexDump
import com.android.testutils.DevSdkIgnoreRunner
import kotlin.test.assertContentEquals
import kotlin.test.assertEquals
import kotlin.test.assertFailsWith
import kotlin.test.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
@RunWith(DevSdkIgnoreRunner::class)
class MdnsPacketTest {
private fun makeFlags(isLabelCountLimitEnabled: Boolean = false): MdnsFeatureFlags =
MdnsFeatureFlags.newBuilder()
.setIsLabelCountLimitEnabled(isLabelCountLimitEnabled).build()
@Test
fun testParseQuery() {
// Probe packet with 1 question for Android.local, and 4 additionalRecords with 4 addresses
@ -38,7 +42,7 @@ class MdnsPacketTest {
"010db8000000000000000000000789"
val bytes = HexDump.hexStringToByteArray(packetHex)
val reader = MdnsPacketReader(bytes, bytes.size)
val reader = MdnsPacketReader(bytes, bytes.size, makeFlags())
val packet = MdnsPacket.parse(reader)
assertEquals(123, packet.transactionId)
@ -68,4 +72,17 @@ class MdnsPacketTest {
assertEquals(InetAddresses.parseNumericAddress("2001:db8::789"),
(packet.authorityRecords[3] as MdnsInetAddressRecord).inet6Address!!)
}
@Test
fun testParseQueryWithLabelLoop_ThrowsParseException() {
val packetWithErrorHex = "000084000000000100000000054C4142454C0454455354C006000C800100000" +
"07800140454455354056C6F63616C00"
val bytes = HexDump.hexStringToByteArray(packetWithErrorHex)
val reader = MdnsPacketReader(
bytes, bytes.size, makeFlags(isLabelCountLimitEnabled = true))
assertFailsWith<MdnsPacket.ParseException> {
MdnsPacket.parse(reader)
}
}
}

8
tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java
View file

@ -17,8 +17,10 @@
package com.android.server.connectivity.mdns;
import static android.net.InetAddresses.parseNumericAddress;
import static com.android.server.connectivity.mdns.util.MdnsUtils.Clock;
import static com.android.testutils.DevSdkIgnoreRuleKt.SC_V2;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
@ -337,7 +339,8 @@ public class MdnsResponseDecoderTests {
packet.setSocketAddress(
new InetSocketAddress(MdnsConstants.getMdnsIPv6Address(), MdnsConstants.MDNS_PORT));
final MdnsPacket parsedPacket = MdnsResponseDecoder.parseResponse(data6, data6.length);
final MdnsPacket parsedPacket = MdnsResponseDecoder.parseResponse(
data6, data6.length, MdnsFeatureFlags.newBuilder().build());
assertNotNull(parsedPacket);
final Network network = mock(Network.class);
@ -636,7 +639,8 @@ public class MdnsResponseDecoderTests {
private ArraySet<MdnsResponse> decode(MdnsResponseDecoder decoder, byte[] data,
Collection<MdnsResponse> existingResponses) throws MdnsPacket.ParseException {
final MdnsPacket parsedPacket = MdnsResponseDecoder.parseResponse(data, data.length);
final MdnsPacket parsedPacket = MdnsResponseDecoder.parseResponse(
data, data.length, MdnsFeatureFlags.newBuilder().build());
assertNotNull(parsedPacket);
return new ArraySet<>(decoder.augmentResponses(parsedPacket,

7
tests/unit/java/com/android/server/connectivity/mdns/MdnsSocketClientTests.java
View file

@ -78,6 +78,7 @@ public class MdnsSocketClientTests {
@Mock private SharedLog sharedLog;
private MdnsSocketClient mdnsClient;
private MdnsFeatureFlags flags = MdnsFeatureFlags.newBuilder().build();
@Before
public void setup() throws RuntimeException, IOException {
@ -86,7 +87,7 @@ public class MdnsSocketClientTests {
when(mockWifiManager.createMulticastLock(ArgumentMatchers.anyString()))
.thenReturn(mockMulticastLock);
mdnsClient = new MdnsSocketClient(mContext, mockMulticastLock, sharedLog) {
mdnsClient = new MdnsSocketClient(mContext, mockMulticastLock, sharedLog, flags) {
@Override
MdnsSocket createMdnsSocket(int port, SharedLog sharedLog) throws IOException {
if (port == MdnsConstants.MDNS_PORT) {
@ -515,7 +516,7 @@ public class MdnsSocketClientTests {
//MdnsConfigsFlagsImpl.allowNetworkInterfaceIndexPropagation.override(true);
when(mockMulticastSocket.getInterfaceIndex()).thenReturn(21);
mdnsClient = new MdnsSocketClient(mContext, mockMulticastLock, sharedLog) {
mdnsClient = new MdnsSocketClient(mContext, mockMulticastLock, sharedLog, flags) {
@Override
MdnsSocket createMdnsSocket(int port, SharedLog sharedLog) {
if (port == MdnsConstants.MDNS_PORT) {
@ -538,7 +539,7 @@ public class MdnsSocketClientTests {
//MdnsConfigsFlagsImpl.allowNetworkInterfaceIndexPropagation.override(false);
when(mockMulticastSocket.getInterfaceIndex()).thenReturn(21);
mdnsClient = new MdnsSocketClient(mContext, mockMulticastLock, sharedLog) {
mdnsClient = new MdnsSocketClient(mContext, mockMulticastLock, sharedLog, flags) {
@Override
MdnsSocket createMdnsSocket(int port, SharedLog sharedLog) {
if (port == MdnsConstants.MDNS_PORT) {