fix out-of-bounds read when matching IFS header keywords

io/src/ifs_io.cpp

@@ -42,8 +42,43 @@
 
 #include <cstring>
 #include <cerrno>
+#include <string>
 #include <boost/iostreams/device/mapped_file.hpp> // for mapped_file_source
 
+namespace
+{
+  /** \brief Read a length prefixed string from \a fs and tell whether it equals \a expected.
+    *
+    * The length prefix is taken from the file as is, so it cannot be relied upon to
+    * cover the terminating null character that pcl::IFSWriter appends. The string is
+    * therefore compared over the characters that were actually read instead of with
+    * strcmp. The field is consumed in either case so that the stream stays aligned
+    * with the next element.
+    */
+  bool
+  readAndMatchString (std::istream &fs, const std::string &expected)
+  {
+    std::uint32_t length = 0;
+    if (!fs.read (reinterpret_cast<char*> (&length), sizeof (length)))
+      return (false);
+
+    // Only the keyword with or without its terminating null character can match, any
+    // other length just needs to be skipped
+    if ((length != expected.size ()) && (length != expected.size () + 1))
+    {
+      fs.ignore (length);
+      return (false);
+    }
+
+    std::string actual (length, '\0');
+    if (!fs.read (&actual.front (), length))
+      return (false);
+
+    return ((actual.compare (0, expected.size (), expected) == 0) &&
+            ((actual.size () == expected.size ()) || (actual.back () == '\0')));
+  }
+}
+
 ///////////////////////////////////////////////////////////////////////////////////////////
 int
 pcl::IFSReader::readHeader (const std::string &file_name, pcl::PCLPointCloud2 &cloud,
@@ -83,13 +118,7 @@ pcl::IFSReader::readHeader (const std::string &file_name, pcl::PCLPointCloud2 &c
   }
 
   //Read the magic
-  std::uint32_t length_of_magic;
-  fs.read (reinterpret_cast<char*>(&length_of_magic), sizeof (std::uint32_t));
-  char *magic = new char [length_of_magic];
-  fs.read (magic, sizeof (char) * length_of_magic);
-  const bool file_is_ifs_file = (strcmp (magic, "IFS") == 0);
-  delete[] magic;
-  if (!file_is_ifs_file)
+  if (!readAndMatchString (fs, "IFS"))
   {
     PCL_ERROR ("[pcl::IFSReader::readHeader] File %s is not an IFS file!\n", file_name.c_str ());
     fs.close ();
@@ -111,27 +140,18 @@ pcl::IFSReader::readHeader (const std::string &file_name, pcl::PCLPointCloud2 &c
       return (-1);
     }
 
-  //Read the name
-  std::uint32_t length_of_name;
+  //Read the name, its content is not used
+  std::uint32_t length_of_name = 0;
   fs.read (reinterpret_cast<char*>(&length_of_name), sizeof (std::uint32_t));
-  char *name = new char [length_of_name];
-  fs.read (name, sizeof (char) * length_of_name);
-  delete[] name;
+  fs.ignore (length_of_name);
 
   // Read the header and fill it in with wonderful values
   try
   {
     while (!fs.eof ())
     {
       //Read the keyword
-      std::uint32_t length_of_keyword;
-      fs.read (reinterpret_cast<char*>(&length_of_keyword), sizeof (std::uint32_t));
-      char *keyword = new char [length_of_keyword];
-      fs.read (keyword, sizeof (char) * length_of_keyword);
-
-      const bool keyword_is_vertices = (strcmp (keyword, "VERTICES") == 0);
-      delete[] keyword;
-      if (keyword_is_vertices)
+      if (readAndMatchString (fs, "VERTICES"))
       {
         fs.read (reinterpret_cast<char*>(&nr_points), sizeof (std::uint32_t));
         if ((nr_points == 0) || (nr_points > 10000000))
@@ -286,13 +306,7 @@ pcl::IFSReader::read (const std::string &file_name, pcl::PolygonMesh &mesh, int
   // Jump to the end of cloud data
   fs.seekg (data_size);
   // Read the TRIANGLES keyword
-  std::uint32_t length_of_keyword;
-  fs.read (reinterpret_cast<char*>(&length_of_keyword), sizeof (std::uint32_t));
-  char *keyword = new char [length_of_keyword];
-  fs.read (keyword, sizeof (char) * length_of_keyword);
-  const bool keyword_is_triangles = (strcmp (keyword, "TRIANGLES") == 0);
-  delete[] keyword;
-  if (!keyword_is_triangles)
+  if (!readAndMatchString (fs, "TRIANGLES"))
   {
     PCL_ERROR ("[pcl::IFSReader::read] File %s is does not contain facets!\n", file_name.c_str ());
     fs.close ();

test/io/test_io.cpp

@@ -1832,6 +1832,61 @@ TEST(PCL, IFS)
   remove("test.ifs");
 }
 
+//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+TEST (PCL, IFSUnterminatedStrings)
+{
+  // The strings in an IFS header are length prefixed and the prefix comes straight from
+  // the file, so it need not account for the terminating null character that
+  // pcl::IFSWriter appends. Reading such a header must not run past the end of the
+  // buffer the string was read into.
+  std::vector<char> data;
+  const auto append_bytes = [&data] (const void* bytes, const std::size_t size)
+  {
+    const char* begin = reinterpret_cast<const char*> (bytes);
+    data.insert (data.end (), begin, begin + size);
+  };
+  const auto append_uint32 = [&append_bytes] (const std::uint32_t value)
+  {
+    append_bytes (&value, sizeof (value));
+  };
+  // Write the string without the terminating null character a writer would add
+  const auto append_unterminated = [&data, &append_bytes, &append_uint32] (const std::string& str)
+  {
+    append_uint32 (static_cast<std::uint32_t> (str.size ()));
+    append_bytes (str.data (), str.size ());
+  };
+
+  append_unterminated ("IFS");
+  const float version = 1.0f;
+  append_bytes (&version, sizeof (version));
+  append_unterminated ("");                  // cloud name
+  append_unterminated ("VERTICES");
+  append_uint32 (1);                         // number of vertices
+  for (const float coordinate : {1.0f, 2.0f, 3.0f})
+    append_bytes (&coordinate, sizeof (coordinate));
+  append_unterminated ("TRIANGLES");
+  append_uint32 (1);                         // number of facets
+  for (int i = 0; i < 3; ++i)
+    append_uint32 (0);
+
+  std::ofstream fs ("test_ifs_unterminated.ifs", std::ios::binary);
+  fs.write (data.data (), data.size ());
+  fs.close ();
+
+  PointCloud<PointXYZ> cloud;
+  ASSERT_EQ (loadIFSFile ("test_ifs_unterminated.ifs", cloud), 0);
+  ASSERT_EQ (cloud.size (), 1);
+  EXPECT_EQ (cloud[0].x, 1.0f);
+  EXPECT_EQ (cloud[0].y, 2.0f);
+  EXPECT_EQ (cloud[0].z, 3.0f);
+
+  PolygonMesh mesh;
+  ASSERT_EQ (loadIFSFile ("test_ifs_unterminated.ifs", mesh), 0);
+  ASSERT_EQ (mesh.polygons.size (), 1);
+
+  remove ("test_ifs_unterminated.ifs");
+}
+
 /* ---[ */
 int
   main (int argc, char** argv)