Skip to content

Commit 15d1e92

Browse files
authored
[multicast_dns] Rewrite _readFQDN to avoid recursion (#8390)
Speculatively fixes flutter/flutter#155433. Read [one of my later comments](flutter/flutter#155433 (comment)) on that thread for my current understanding of this issue. In summary, this PR rewrites `_readFQDN` to avoid recursion. **For reviewers: For a simplified diff, see the first commit, [change](https://github.com/flutter/packages/commit/7afa1db54e07f024b2a4550d160b4d4a9f67f2c7).** This commit avoids formatting make sure the diff only highlights changes to logic rather than formatting. <details> <summary> Pre-launch Checklist </summary> - x ] I signed the [CLA]. </details>
1 parent 99f79d9 commit 15d1e92

File tree

4 files changed

+89
-68
lines changed

4 files changed

+89
-68
lines changed

packages/multicast_dns/CHANGELOG.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
## NEXT
1+
## 0.3.2+8
22

3+
* Fixes stack overflows ocurring during the parsing of domain names in MDNS messages.
34
* Updates minimum supported SDK version to Flutter 3.22/Dart 3.4.
45

56
## 0.3.2+7

packages/multicast_dns/lib/src/packet.dart

Lines changed: 46 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import 'dart:convert';
66
import 'dart:io';
7+
import 'dart:math';
78
import 'dart:typed_data';
89

910
import 'constants.dart';
@@ -134,76 +135,55 @@ _FQDNReadResult _readFQDN(
134135

135136
final List<String> parts = <String>[];
136137
final int prevOffset = offset;
137-
while (true) {
138-
// At least one byte is required.
139-
checkLength(offset + 1);
140-
141-
// Check for compressed.
142-
if (data[offset] & 0xc0 == 0xc0) {
143-
// At least two bytes are required for a compressed FQDN.
144-
checkLength(offset + 2);
145-
146-
// A compressed FQDN has a new offset in the lower 14 bits.
147-
final _FQDNReadResult result = _readFQDN(
148-
data, byteData, byteData.getUint16(offset) & ~0xc000, length);
149-
parts.addAll(result.fqdnParts);
150-
offset += 2;
151-
break;
152-
} else {
153-
// A normal FQDN part has a length and a UTF-8 encoded name
154-
// part. If the length is 0 this is the end of the FQDN.
155-
final int partLength = data[offset];
156-
offset++;
157-
if (partLength > 0) {
158-
checkLength(offset + partLength);
159-
final Uint8List partBytes =
160-
Uint8List.view(data.buffer, offset, partLength);
161-
offset += partLength;
162-
// According to the RFC, this is supposed to be utf-8 encoded, but
163-
// we should continue decoding even if it isn't to avoid dropping the
164-
// rest of the data, which might still be useful.
165-
parts.add(utf8.decode(partBytes, allowMalformed: true));
166-
} else {
138+
final List<int> offsetsToVisit = <int>[offset];
139+
int upperLimitOffset = offset;
140+
int highestOffsetRead = offset;
141+
142+
while (offsetsToVisit.isNotEmpty) {
143+
offset = offsetsToVisit.removeLast();
144+
145+
while (true) {
146+
// At least one byte is required.
147+
checkLength(offset + 1);
148+
// Check for compressed.
149+
if (data[offset] & 0xc0 == 0xc0) {
150+
// At least two bytes are required for a compressed FQDN (see RFC1035 section 4.1.4).
151+
checkLength(offset + 2);
152+
153+
// A compressed FQDN has a new offset in the lower 14 bits.
154+
final int pointerDest = byteData.getUint16(offset) & ~0xc000;
155+
// Pointers can only point to prior occurances of some name.
156+
// This check also guards against pointers that form loops.
157+
if (pointerDest >= upperLimitOffset) {
158+
throw MDnsDecodeException(offset);
159+
}
160+
upperLimitOffset = pointerDest;
161+
offsetsToVisit.add(pointerDest);
162+
highestOffsetRead = max(highestOffsetRead, offset + 2);
167163
break;
164+
} else {
165+
// A normal FQDN part has a length and a UTF-8 encoded name
166+
// part. If the length is 0 this is the end of the FQDN.
167+
final int partLength = data[offset];
168+
offset++;
169+
if (partLength > 0) {
170+
checkLength(offset + partLength);
171+
final Uint8List partBytes =
172+
Uint8List.view(data.buffer, offset, partLength);
173+
offset += partLength;
174+
// According to the RFC, this is supposed to be utf-8 encoded, but
175+
// we should continue decoding even if it isn't to avoid dropping the
176+
// rest of the data, which might still be useful.
177+
parts.add(utf8.decode(partBytes, allowMalformed: true));
178+
highestOffsetRead = max(highestOffsetRead, offset);
179+
} else {
180+
highestOffsetRead = max(highestOffsetRead, offset);
181+
break;
182+
}
168183
}
169184
}
170185
}
171-
return _FQDNReadResult(parts, offset - prevOffset);
172-
}
173-
174-
/// Decode an mDNS query packet.
175-
///
176-
/// If decoding fails (e.g. due to an invalid packet), `null` is returned.
177-
///
178-
/// See https://tools.ietf.org/html/rfc1035 for format.
179-
ResourceRecordQuery? decodeMDnsQuery(List<int> packet) {
180-
final int length = packet.length;
181-
if (length < _kHeaderSize) {
182-
return null;
183-
}
184-
185-
final Uint8List data =
186-
packet is Uint8List ? packet : Uint8List.fromList(packet);
187-
final ByteData packetBytes = ByteData.view(data.buffer);
188-
189-
// Check whether it's a query.
190-
final int flags = packetBytes.getUint16(_kFlagsOffset);
191-
if (flags != 0) {
192-
return null;
193-
}
194-
final int questionCount = packetBytes.getUint16(_kQdcountOffset);
195-
if (questionCount == 0) {
196-
return null;
197-
}
198-
199-
final _FQDNReadResult fqdn =
200-
_readFQDN(data, packetBytes, _kHeaderSize, data.length);
201-
202-
int offset = _kHeaderSize + fqdn.bytesRead;
203-
final int type = packetBytes.getUint16(offset);
204-
offset += 2;
205-
final int queryType = packetBytes.getUint16(offset) & 0x8000;
206-
return ResourceRecordQuery(type, fqdn.fqdn, queryType);
186+
return _FQDNReadResult(parts, highestOffsetRead - prevOffset);
207187
}
208188

209189
/// Decode an mDNS response packet.

packages/multicast_dns/pubspec.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ name: multicast_dns
22
description: Dart package for performing mDNS queries (e.g. Bonjour, Avahi).
33
repository: https://github.com/flutter/packages/tree/main/packages/multicast_dns
44
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+multicast_dns%22
5-
version: 0.3.2+7
5+
version: 0.3.2+8
66

77
environment:
88
sdk: ^3.4.0

packages/multicast_dns/test/decode_test.dart

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,10 @@ void testBadPackages() {
192192
}
193193
}
194194
});
195+
196+
test('Detects cyclic pointers and returns null', () {
197+
expect(decodeMDnsResponse(cycle), isNull);
198+
});
195199
}
196200

197201
void testPTRRData() {
@@ -584,6 +588,42 @@ const List<int> package3 = <int>[
584588
0x2c
585589
];
586590

591+
/// Contains compressed domain names where a there is a cycle amongst the
592+
/// offset pointers.
593+
const List<int> cycle = <int>[
594+
0x00,
595+
0x00,
596+
0x84,
597+
0x00,
598+
0x00,
599+
0x00,
600+
0x00,
601+
0x01,
602+
0x00,
603+
0x00,
604+
0x00,
605+
0x00,
606+
0x07, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, // "example"
607+
0xC0, 0x16, // Pointer to "com"
608+
0x03, 0x63, 0x6f, 0x6d, // "com"
609+
0xC0, 0x0c, // Pointer to "example"
610+
0x00,
611+
0x00,
612+
0x01,
613+
0x80,
614+
0x01,
615+
0x00,
616+
0x00,
617+
0x00,
618+
0x78,
619+
0x00,
620+
0x04,
621+
0xc0,
622+
0xa8,
623+
0x01,
624+
0xbf
625+
];
626+
587627
const List<int> packagePtrResponse = <int>[
588628
0x00,
589629
0x00,

0 commit comments

Comments
 (0)