Skip to content

Commit 400e28d

Browse files
committed
Fix incorrect length check in DNSName when extracting qtype or qclass
In `DNSName::packetParser()`, the length check might have been incorrect when the caller asked for the `qtype` and/or the `qclass` to be extracted. The `pos + labellen + 2 > end` check was wrong because `pos` might have already been incremented by `labellen`. There are 3 ways to exit the main loop: * `labellen` is 0, the most common case, and in that case the check is valid * `pos >= end`, meaning that `pos + labellen + 2 > end` will be true regardless of the value of `labellen` since it cannot be negative * if `uncompress` is set and a compressed label is found, the main loop is broken out of, and `labellen` still holds a now irrelevant, possibly non-zero value corresponding to the first byte of the compressed label length & ~0xc0. In that last case, if the compressed label points to a position > 255 the check is wrong and might have rejected a valid packet. A quick look throught the code didn't show any place where we request decompression and ask for `qtype` and/or `qclass` in a response, but I might have missed one. Reported by Houssam El Hajoui (thanks!). (cherry picked from commit 7b9c052)
1 parent 1dde8cc commit 400e28d

File tree

2 files changed

+43
-4
lines changed

2 files changed

+43
-4
lines changed

pdns/dnsname.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -141,15 +141,15 @@ void DNSName::packetParser(const char* qpos, int len, int offset, bool uncompres
141141
if(consumed)
142142
*consumed = pos - opos - offset;
143143
if(qtype) {
144-
if (pos + labellen + 2 > end) {
145-
throw std::range_error("Trying to read qtype past the end of the buffer ("+std::to_string((pos - opos) + labellen + 2)+ " > "+std::to_string(len)+")");
144+
if (pos + 2 > end) {
145+
throw std::range_error("Trying to read qtype past the end of the buffer ("+std::to_string((pos - opos) + 2)+ " > "+std::to_string(len)+")");
146146
}
147147
*qtype=(*(const unsigned char*)pos)*256 + *((const unsigned char*)pos+1);
148148
}
149149
pos+=2;
150150
if(qclass) {
151-
if (pos + labellen + 2 > end) {
152-
throw std::range_error("Trying to read qclass past the end of the buffer ("+std::to_string((pos - opos) + labellen + 2)+ " > "+std::to_string(len)+")");
151+
if (pos + 2 > end) {
152+
throw std::range_error("Trying to read qclass past the end of the buffer ("+std::to_string((pos - opos) + 2)+ " > "+std::to_string(len)+")");
153153
}
154154
*qclass=(*(const unsigned char*)pos)*256 + *((const unsigned char*)pos+1);
155155
}

pdns/test-dnsname_cc.cc

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -610,6 +610,45 @@ BOOST_AUTO_TEST_CASE(test_compression) { // Compression test
610610
BOOST_CHECK_EQUAL(dn.toString(), "www.example.com.");
611611
}
612612

613+
BOOST_AUTO_TEST_CASE(test_compression_qtype_qclass) { // Compression test with QClass and QType extraction
614+
615+
uint16_t qtype = 0;
616+
uint16_t qclass = 0;
617+
618+
{
619+
string name("\x03""com\x00""\x07""example\xc0""\x00""\x03""www\xc0""\x05""\x00""\x01""\x00""\x01", 25);
620+
DNSName dn(name.c_str(), name.size(), 15, true, &qtype, &qclass);
621+
BOOST_CHECK_EQUAL(dn.toString(), "www.example.com.");
622+
BOOST_CHECK_EQUAL(qtype, 1);
623+
BOOST_CHECK_EQUAL(qclass, 1);
624+
}
625+
626+
{
627+
/* same but this time we are one byte short for the qclass */
628+
string name("\x03""com\x00""\x07""example\xc0""\x00""\x03""www\xc0""\x05""\x00""\x01""\x00""", 24);
629+
BOOST_CHECK_THROW(DNSName dn(name.c_str(), name.size(), 15, true, &qtype, &qclass), std::range_error);
630+
}
631+
632+
{
633+
/* this time with a compression pointer such as (labellen << 8) != 0, see #4718 */
634+
string name("\x03""com\x00""\x07""example\xc1""\x00""\x03""www\xc1""\x05""\x00""\x01""\x00""\x01", 25);
635+
name.insert(0, 256, '0');
636+
637+
DNSName dn(name.c_str(), name.size(), 271, true, &qtype, &qclass);
638+
BOOST_CHECK_EQUAL(dn.toString(), "www.example.com.");
639+
BOOST_CHECK_EQUAL(qtype, 1);
640+
BOOST_CHECK_EQUAL(qclass, 1);
641+
}
642+
643+
{
644+
/* same but this time we are one byte short for the qclass */
645+
string name("\x03""com\x00""\x07""example\xc1""\x00""\x03""www\xc1""\x05""\x00""\x01""\x00", 24);
646+
name.insert(0, 256, '0');
647+
648+
BOOST_CHECK_THROW(DNSName dn(name.c_str(), name.size(), 271, true, &qtype, &qclass), std::range_error);;
649+
}
650+
}
651+
613652
BOOST_AUTO_TEST_CASE(test_pointer_pointer_root) { // Pointer to pointer to root
614653

615654
string name("\x00""\xc0""\x00""\x03""com\xc0""\x01",9);

0 commit comments

Comments
 (0)