Skip to content

Commit

Permalink
DefaultDnsRecordDecoder compression and index decode bug
Browse files Browse the repository at this point in the history
Motivation:
8cf90f0 switch a duplicate opreration to a slice operation. Typically this would be fine but DNS supports a compression (https://www.ietf.org/rfc/rfc1035 4.1.4. Message compression) where the payload contains absolute indexes which refer back to previously referenced content. Using a slice will break the ability for the indexes in the payload to correctly self reference to the index of the originial payload, and thus decoding may fail.

Modifications:
- Use duplicate instead of slice so DNS message compression and index references are preserved.

Result:
Fixes DefaultDnsRecordDecoder regression
  • Loading branch information
Scottmitch committed Oct 20, 2016
1 parent efca9d1 commit cfa5b85
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,16 @@ protected DnsRecord decodeRecord(
String name, DnsRecordType type, int dnsClass, long timeToLive,
ByteBuf in, int offset, int length) throws Exception {

// DNS message compression means that domain names may contain "pointers" to other positions in the packet
// to build a full message. This means the indexes are meaningful and we need the ability to reference the
// indexes un-obstructed, and thus we cannot use a slice here.
// See https://www.ietf.org/rfc/rfc1035 [4.1.4. Message compression]
if (type == DnsRecordType.PTR) {
return new DefaultDnsPtrRecord(name, dnsClass, timeToLive, decodeName0(in.slice(offset, length)));
return new DefaultDnsPtrRecord(
name, dnsClass, timeToLive, decodeName0(in.duplicate().setIndex(offset, offset + length)));
}
return new DefaultDnsRawRecord(
name, type, dnsClass, timeToLive, in.retainedSlice(offset, length));
name, type, dnsClass, timeToLive, in.retainedDuplicate().setIndex(offset, offset + length));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,72 @@ public void testDecodePtrRecord() throws Exception {
buffer.release();
}
}

@Test
public void testDecodeMessageCompression() throws Exception {
// See https://www.ietf.org/rfc/rfc1035 [4.1.4. Message compression]
DefaultDnsRecordDecoder decoder = new DefaultDnsRecordDecoder();
byte[] rfcExample = new byte[] { 1, 'F', 3, 'I', 'S', 'I', 4, 'A', 'R', 'P', 'A',
0, 3, 'F', 'O', 'O',
(byte) 0xC0, 0, // this is 20 in the example
(byte) 0xC0, 6, // this is 26 in the example
};
DefaultDnsRawRecord rawPlainRecord = null;
DefaultDnsRawRecord rawUncompressedRecord = null;
DefaultDnsRawRecord rawUncompressedIndexedRecord = null;
ByteBuf buffer = Unpooled.wrappedBuffer(rfcExample);
try {
// First lets test that our utility funciton can correctly handle index references and decompression.
String plainName = DefaultDnsRecordDecoder.decodeName(buffer.duplicate());
assertEquals("F.ISI.ARPA.", plainName);
String uncompressedPlainName = DefaultDnsRecordDecoder.decodeName(buffer.duplicate().setIndex(16, 20));
assertEquals(plainName, uncompressedPlainName);
String uncompressedIndexedName = DefaultDnsRecordDecoder.decodeName(buffer.duplicate().setIndex(12, 20));
assertEquals("FOO." + plainName, uncompressedIndexedName);

// Now lets make sure out object parsing produces the same results for non PTR type (just use CNAME).
rawPlainRecord = (DefaultDnsRawRecord) decoder.decodeRecord(
plainName, DnsRecordType.CNAME, DnsRecord.CLASS_IN, 60, buffer, 0, 11);
assertEquals(plainName, rawPlainRecord.name());
assertEquals(plainName, DefaultDnsRecordDecoder.decodeName(rawPlainRecord.content()));

rawUncompressedRecord = (DefaultDnsRawRecord) decoder.decodeRecord(
uncompressedPlainName, DnsRecordType.CNAME, DnsRecord.CLASS_IN, 60, buffer, 16, 4);
assertEquals(uncompressedPlainName, rawUncompressedRecord.name());
assertEquals(uncompressedPlainName, DefaultDnsRecordDecoder.decodeName(rawUncompressedRecord.content()));

rawUncompressedIndexedRecord = (DefaultDnsRawRecord) decoder.decodeRecord(
uncompressedIndexedName, DnsRecordType.CNAME, DnsRecord.CLASS_IN, 60, buffer, 12, 8);
assertEquals(uncompressedIndexedName, rawUncompressedIndexedRecord.name());
assertEquals(uncompressedIndexedName,
DefaultDnsRecordDecoder.decodeName(rawUncompressedIndexedRecord.content()));

// Now lets make sure out object parsing produces the same results for PTR type.
DnsPtrRecord ptrRecord = (DnsPtrRecord) decoder.decodeRecord(
plainName, DnsRecordType.PTR, DnsRecord.CLASS_IN, 60, buffer, 0, 11);
assertEquals(plainName, ptrRecord.name());
assertEquals(plainName, ptrRecord.hostname());

ptrRecord = (DnsPtrRecord) decoder.decodeRecord(
uncompressedPlainName, DnsRecordType.PTR, DnsRecord.CLASS_IN, 60, buffer, 16, 4);
assertEquals(uncompressedPlainName, ptrRecord.name());
assertEquals(uncompressedPlainName, ptrRecord.hostname());

ptrRecord = (DnsPtrRecord) decoder.decodeRecord(
uncompressedIndexedName, DnsRecordType.PTR, DnsRecord.CLASS_IN, 60, buffer, 12, 8);
assertEquals(uncompressedIndexedName, ptrRecord.name());
assertEquals(uncompressedIndexedName, ptrRecord.hostname());
} finally {
if (rawPlainRecord != null) {
rawPlainRecord.release();
}
if (rawUncompressedRecord != null) {
rawUncompressedRecord.release();
}
if (rawUncompressedIndexedRecord != null) {
rawUncompressedIndexedRecord.release();
}
buffer.release();
}
}
}

0 comments on commit cfa5b85

Please sign in to comment.