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 fd317b932e5107350865952e1c15a08c44710991..dc49cb64f60125004a15fabc6d30bcf850af37c0 100644 (file)
--- 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 426a6d55594cbb47690df8c0b12a886f95a78386..a9b75ac8dba6118e80005557f81ef276689dfdd5 100644 (file)
--- 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 72e5c08060c1f0cab75325fd9c942e90905411f5..451aa57b7dab41b9fc1aa2bf72a08d34dab38419 100644 (file)
--- a/pdns/rcpgenerator.cc
+++ b/pdns/rcpgenerator.cc
@@ -95,9 +95,11 @@ void RecordTextReader::xfrTime(uint32_t &val)
 
   tmp<<itmp;
 
-  sscanf(tmp.str().c_str(), "%04d%02d%02d" "%02d%02d%02d", 
-         &tm.tm_year, &tm.tm_mon, &tm.tm_mday, 
-         &tm.tm_hour, &tm.tm_min, &tm.tm_sec);
+  if (sscanf(tmp.str().c_str(), "%04d%02d%02d" "%02d%02d%02d",
+             &tm.tm_year, &tm.tm_mon, &tm.tm_mday,
+             &tm.tm_hour, &tm.tm_min, &tm.tm_sec) != 6) {
+    throw RecordTextException("unable to parse '"+std::to_string(itmp)+"' into a valid time at position "+std::to_string(d_pos)+" in '"+d_string+"'");
+  }
 
   tm.tm_year-=1900;
   tm.tm_mon-=1;

diff --git a/pdns/test-zoneparser_tng_cc.cc b/pdns/test-zoneparser_tng_cc.cc
index f300efbf65761a8c6f5f2fc2bac62cd2f7324c49..84d902620e30e3770b75ea85de4b001f69723f36 100644 (file)
--- a/pdns/test-zoneparser_tng_cc.cc
+++ b/pdns/test-zoneparser_tng_cc.cc
@@ -61,38 +61,97 @@ BOOST_AUTO_TEST_CASE(test_tng_record_generate) {
   if(!p)
     p = ".";
   pathbuf << p << "/../regression-tests/zones/unit2.test";
-  ZoneParserTNG zp(pathbuf.str(), DNSName("unit2.test"));
-
-  vector<string> 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<string> 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<std::string>({"$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<string> 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<std::string>({"$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<std::string>({"$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<std::string>({"$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<std::string>({"$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<std::string>({"$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 5a4f8336f87c822c2b00120cf93302204e4dea06..a334a5cb8c5dd484135cf9871676068337e3731d 100644 (file)
--- a/pdns/zoneparser-tng.cc
+++ b/pdns/zoneparser-tng.cc
@@ -35,6 +35,7 @@
 #include <deque>
 #include <boost/algorithm/string.hpp>
 #include <system_error>
+#include <cinttypes>
 
 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");