From: Remi Gacogne Date: Thu, 24 Sep 2020 14:17:58 +0000 (+0200) Subject: Always check sscanf's return value X-Git-Tag: auth-4.4.0-alpha2~69^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F9519%2Fhead;p=thirdparty%2Fpdns.git Always check sscanf's return value These remaining cases are not in any way a security issue since the variables are initialized before the call. This mostly improves the reporting, and make sure we fail earlier. --- diff --git a/ext/yahttp/yahttp/reqresp.cpp b/ext/yahttp/yahttp/reqresp.cpp index fd317b932e..dc49cb64f6 100644 --- a/ext/yahttp/yahttp/reqresp.cpp +++ b/ext/yahttp/yahttp/reqresp.cpp @@ -173,7 +173,9 @@ namespace YaHTTP { buffer.copy(buf, pos); buf[pos]=0; // just in case... buffer.erase(buffer.begin(), buffer.begin()+pos+1); // remove line from buffer - sscanf(buf, "%x", &chunk_size); + if (sscanf(buf, "%x", &chunk_size) != 1) { + throw ParseError("Unable to parse chunk size"); + } if (chunk_size == 0) { state = 3; break; } // last chunk } else { int crlf=1; diff --git a/pdns/misc.cc b/pdns/misc.cc index 426a6d5559..a9b75ac8db 100644 --- a/pdns/misc.cc +++ b/pdns/misc.cc @@ -1107,7 +1107,9 @@ string getMACAddress(const ComboAddress& ca) if(parts.size() < 4) return ret; unsigned int tmp[6]; - sscanf(parts[3].c_str(), "%02x:%02x:%02x:%02x:%02x:%02x", tmp, tmp+1, tmp+2, tmp+3, tmp+4, tmp+5); + if (sscanf(parts[3].c_str(), "%02x:%02x:%02x:%02x:%02x:%02x", tmp, tmp+1, tmp+2, tmp+3, tmp+4, tmp+5) != 6) { + return ret; + } for(int i = 0 ; i< 6 ; ++i) ret.append(1, (char)tmp[i]); return ret; diff --git a/pdns/rcpgenerator.cc b/pdns/rcpgenerator.cc index 72e5c08060..451aa57b7d 100644 --- a/pdns/rcpgenerator.cc +++ b/pdns/rcpgenerator.cc @@ -95,9 +95,11 @@ void RecordTextReader::xfrTime(uint32_t &val) tmp< expected = { - "0.01.0003.000005.00000007.unit2.test.", - "1.02.0004.000006.00000008.unit2.test.", - "2.03.0005.000007.00000009.unit2.test.", - "3.04.0006.000008.0000000a.unit2.test.", - "4.05.0007.000009.0000000b.unit2.test.", - "5.06.0008.00000A.0000000c.unit2.test.", - "6.07.0009.00000B.0000000d.unit2.test.", - "7.10.0010.00000C.0000000e.unit2.test.", - "8.11.0011.00000D.0000000f.unit2.test.", - "9.12.0012.00000E.00000010.unit2.test.", - "10.13.0013.00000F.00000011.unit2.test.", - "11.14.0014.000010.00000012.unit2.test.", - "12.15.0015.000011.00000013.unit2.test.", - "13.16.0016.000012.00000014.unit2.test.", - "14.17.0017.000013.00000015.unit2.test.", - "15.20.0018.000014.00000016.unit2.test.", - "16.21.0019.000015.00000017.unit2.test." - }; - - for (auto const & exp : expected) { + + { + /* simple case */ + ZoneParserTNG zp(pathbuf.str(), DNSName("unit2.test")); + + const vector expected = { + "0.01.0003.000005.00000007.unit2.test.", + "1.02.0004.000006.00000008.unit2.test.", + "2.03.0005.000007.00000009.unit2.test.", + "3.04.0006.000008.0000000a.unit2.test.", + "4.05.0007.000009.0000000b.unit2.test.", + "5.06.0008.00000A.0000000c.unit2.test.", + "6.07.0009.00000B.0000000d.unit2.test.", + "7.10.0010.00000C.0000000e.unit2.test.", + "8.11.0011.00000D.0000000f.unit2.test.", + "9.12.0012.00000E.00000010.unit2.test.", + "10.13.0013.00000F.00000011.unit2.test.", + "11.14.0014.000010.00000012.unit2.test.", + "12.15.0015.000011.00000013.unit2.test.", + "13.16.0016.000012.00000014.unit2.test.", + "14.17.0017.000013.00000015.unit2.test.", + "15.20.0018.000014.00000016.unit2.test.", + "16.21.0019.000015.00000017.unit2.test." + }; + + for (auto const & exp : expected) { + DNSResourceRecord rr; + zp.get(rr); + BOOST_CHECK_EQUAL(rr.qname.toString(), exp); + BOOST_CHECK_EQUAL(rr.ttl, 86400U); + BOOST_CHECK_EQUAL(rr.qclass, 1U); + BOOST_CHECK_EQUAL(rr.qtype.getName(), "A"); + BOOST_CHECK_EQUAL(rr.content, "1.2.3.4"); + } + } + + { + /* GENERATE with a step of 2, and the template radix defaulting to 'd' */ + ZoneParserTNG zp(std::vector({"$GENERATE 0-4/2 $.${1,2,o}.${3,4}.${5,6,X}.${7,8,x} 86400 IN A 1.2.3.4"}), DNSName("unit2.test")); + + const vector expected = { + "0.01.0003.000005.00000007.unit2.test.", + "2.03.0005.000007.00000009.unit2.test.", + "4.05.0007.000009.0000000b.unit2.test.", + }; + + for (auto const & exp : expected) { + DNSResourceRecord rr; + zp.get(rr); + BOOST_CHECK_EQUAL(rr.qname.toString(), exp); + BOOST_CHECK_EQUAL(rr.ttl, 86400U); + BOOST_CHECK_EQUAL(rr.qclass, 1U); + BOOST_CHECK_EQUAL(rr.qtype.getName(), "A"); + BOOST_CHECK_EQUAL(rr.content, "1.2.3.4"); + } + } + + { + /* test invalid generate parameters: stop greater than start */ + ZoneParserTNG zp(std::vector({"$GENERATE 5-4 $.${1,2,o}.${3,4,d}.${5,6,X}.${7,8,x} 86400 IN A 1.2.3.4"}), DNSName("test")); + DNSResourceRecord rr; + BOOST_CHECK_THROW(zp.get(rr), std::exception); + } + + { + /* test invalid generate parameters: no stop */ + ZoneParserTNG zp(std::vector({"$GENERATE 5 $.${1,2,o}.${3,4,d}.${5,6,X}.${7,8,x} 86400 IN A 1.2.3.4"}), DNSName("test")); + DNSResourceRecord rr; + BOOST_CHECK_THROW(zp.get(rr), std::exception); + } + + { + /* test invalid generate parameters: invalid step */ + ZoneParserTNG zp(std::vector({"$GENERATE 0-4/0 $.${1,2,o}.${3,4,d}.${5,6,X}.${7,8,x} 86400 IN A 1.2.3.4"}), DNSName("test")); DNSResourceRecord rr; - zp.get(rr); - BOOST_CHECK_EQUAL(rr.qname.toString(), exp); - BOOST_CHECK_EQUAL(rr.ttl, 86400U); - BOOST_CHECK_EQUAL(rr.qclass, 1U); - BOOST_CHECK_EQUAL(rr.qtype.getName(), "A"); - BOOST_CHECK_EQUAL(rr.content, "1.2.3.4"); + BOOST_CHECK_THROW(zp.get(rr), std::exception); } + { + /* test invalid generate parameters: no offset */ + ZoneParserTNG zp(std::vector({"$GENERATE 0-4/1 $.${}.${3,4,d}.${5,6,X}.${7,8,x} 86400 IN A 1.2.3.4"}), DNSName("test")); + DNSResourceRecord rr; + BOOST_CHECK_THROW(zp.get(rr), PDNSException); + } + + { + /* test invalid generate parameters: invalid offset */ + ZoneParserTNG zp(std::vector({"$GENERATE 0-4/1 $.${a,2,o}.${3,4,d}.${5,6,X}.${7,8,x} 86400 IN A 1.2.3.4"}), DNSName("test")); + DNSResourceRecord rr; + BOOST_CHECK_THROW(zp.get(rr), PDNSException); + } } BOOST_AUTO_TEST_SUITE_END(); diff --git a/pdns/zoneparser-tng.cc b/pdns/zoneparser-tng.cc index 5a4f8336f8..a334a5cb8c 100644 --- a/pdns/zoneparser-tng.cc +++ b/pdns/zoneparser-tng.cc @@ -35,6 +35,7 @@ #include #include #include +#include static string g_INstr("IN"); @@ -148,9 +149,9 @@ bool ZoneParserTNG::getTemplateLine() string part=makeString(d_templateline, *iter); /* a part can contain a 'naked' $, an escaped $ (\$), or ${offset,width,radix}, with width defaulting to 0, - and radix being 'd', 'o', 'x' or 'X', defaulting to 'd'. + and radix being 'd', 'o', 'x' or 'X', defaulting to 'd' (so ${offset} is valid). - The width is zero-padded, so if the counter is at 1, the offset is 15, with is 3, and the radix is 'x', + The width is zero-padded, so if the counter is at 1, the offset is 15, width is 3, and the radix is 'x', output will be '010', from the input of ${15,3,x} */ @@ -191,7 +192,11 @@ bool ZoneParserTNG::getTemplateLine() string spec(part.c_str() + startPos, part.c_str() + pos); int offset=0, width=0; char radix='d'; - sscanf(spec.c_str(), "%d,%d,%c", &offset, &width, &radix); // parse format specifier + // parse format specifier + int extracted = sscanf(spec.c_str(), "%d,%d,%c", &offset, &width, &radix); + if (extracted < 1) { + throw PDNSException("Unable to parse offset, width and radix for $GENERATE's lhs from '"+spec+"' "+getLineOfFile()); + } char tmp[80]; switch (radix) { @@ -324,10 +329,19 @@ bool ZoneParserTNG::get(DNSResourceRecord& rr, std::string* comment) throw exception("$GENERATE is not allowed in this zone"); } // $GENERATE 1-127 $ CNAME $.0 + // The range part can be one of two forms: start-stop or start-stop/step. If the first + // form is used, then step is set to 1. start, stop and step must be positive + // integers between 0 and (2^31)-1. start must not be larger than stop. string range=makeString(d_line, d_parts[1]); d_templatestep=1; d_templatestop=0; - sscanf(range.c_str(),"%u-%u/%u", &d_templatecounter, &d_templatestop, &d_templatestep); + int extracted = sscanf(range.c_str(),"%" SCNu32 "-%" SCNu32 "/%" SCNu32, &d_templatecounter, &d_templatestop, &d_templatestep); + if (extracted == 2) { + d_templatestep=1; + } + else if (extracted != 3) { + throw exception("Invalid range from $GENERATE parameters '" + range + "'"); + } if (d_templatestep < 1 || d_templatestop < d_templatecounter) { throw exception("Invalid $GENERATE parameters");