Trap for overly large input to XmlRPCPP (#2065)

* Trap for overly large input to XmlRPCPP which could cause problems with int <-> size_t conversions.

 - In XmlRpcClient, XmlRpcServerConnection and XmlRpcSocket, recognize when incoming or outgoing data is too large, generate an error and discard the data when practical.
 - Use the safe strtol() rather than atoi() to decode an incoming content-length header, and generate an error if the length is invalid or too large.
 - In XmlRpcUtil, prevent attempts to parse overly large XML input.
 - Add tests where they can reasonably be inserted into existing test routines.

Although this fix could be cleaner the update is written to make the update ABI compatible.

This fix addresses CVE-2020-16124 / Integer overflow in ros_comm.

Signed-off-by: Sid Faber <sid.faber@canonical.com>

* Trap for memory allocation error in tests

Signed-off-by: Sid Faber <sid.faber@canonical.com>

* Revert earlier change

Signed-off-by: Sid Faber <sid.faber@canonical.com>

* Update tests

Replace call to GTEST_SKIP with output to stderr. Remove the
redResponseOversize test since out-of-memory errors during
testing cannot easily be handled within the existing test objects.

Signed-off-by: Sid Faber <sid.faber@canonical.com>

* Improve test error handling

Use GTEST_SKIP if available, otherwise print to stderr. Remove test
that's being killed because it takes too long to handle the oversize
test values

utilities/xmlrpcpp/src/XmlRpcClient.cpp

@@ -281,6 +281,13 @@ XmlRpcClient::generateRequest(const char* methodName, XmlRpcValue const& params)
                   header.length(), body.length());
 
   _request = header + body;
+  // Limit the size of the request to avoid integer overruns
+  if (_request.length() > size_t(__INT_MAX__)) {
+    XmlRpcUtil::error("XmlRpcClient::generateRequest: request length (%u) exceeds maximum allowed size (%u).",
+                      _request.length(), __INT_MAX__);
+    _request.clear();
+    return false;
+  }
   return true;
 }
 
@@ -394,11 +401,14 @@ XmlRpcClient::readHeader()
     return false;   // We could try to figure it out by parsing as we read, but for now...
   }
 
-  _contentLength = atoi(lp);
-  if (_contentLength <= 0) {
-    XmlRpcUtil::error("Error in XmlRpcClient::readHeader: Invalid Content-length specified (%d).", _contentLength);
+  // avoid overly large or improperly formatted content-length
+  long int clength = 0;
+  clength = strtol(lp, NULL, 10);
+  if ((clength <= 0) || (clength > __INT_MAX__)) {
+    XmlRpcUtil::error("Error in XmlRpcClient::readHeader: Invalid Content-length specified.");
     return false;
   }
+  _contentLength = int(clength);
 
   XmlRpcUtil::log(4, "client read content length: %d", _contentLength);
 
@@ -419,6 +429,14 @@ XmlRpcClient::readResponse()
       return false;
     }
 
+    // Avoid an overly large response
+    if (_response.length() > size_t(__INT_MAX__)) {
+      XmlRpcUtil::error("XmlRpcClient::readResponse: response length (%u) exceeds the maximum allowed size (%u).",
+                        _response.length(), __INT_MAX__);
+      _response.clear();
+      close();
+      return false;
+    }
     // If we haven't gotten the entire _response yet, return (keep reading)
     if (int(_response.length()) < _contentLength) {
       if (_eof) {

utilities/xmlrpcpp/src/XmlRpcServerConnection.cpp

@@ -119,11 +119,14 @@ XmlRpcServerConnection::readHeader()
     return false;   // We could try to figure it out by parsing as we read, but for now...
   }
 
-  _contentLength = atoi(lp);
-  if (_contentLength <= 0) {
-    XmlRpcUtil::error("XmlRpcServerConnection::readHeader: Invalid Content-length specified (%d).", _contentLength);
+  // avoid overly large or improperly formatted content-length
+  long int clength = 0;
+  clength = strtol(lp, NULL, 10);
+  if ((clength < 0) || (clength > __INT_MAX__)) {
+    XmlRpcUtil::error("XmlRpcServerConnection::readHeader: Invalid Content-length specified.");
     return false;
   }
+  _contentLength = int(clength);
 
   XmlRpcUtil::log(3, "XmlRpcServerConnection::readHeader: specified content length is %d.", _contentLength);
 
@@ -157,6 +160,13 @@ XmlRpcServerConnection::readRequest()
       XmlRpcUtil::error("XmlRpcServerConnection::readRequest: read error (%s).",XmlRpcSocket::getErrorMsg().c_str());
       return false;
     }
+    // Avoid an overly large request
+    if (_request.length() > size_t(__INT_MAX__)) {
+      XmlRpcUtil::error("XmlRpcServerConnection::readRequest: request length (%u) exceeds the maximum allowed size (%u)",
+                        _request.length(), __INT_MAX__);
+      _request.resize(__INT_MAX__);
+      return false;
+    }
 
     // If we haven't gotten the entire request yet, return (keep reading)
     if (int(_request.length()) < _contentLength) {
@@ -334,8 +344,16 @@ XmlRpcServerConnection::generateResponse(std::string const& resultXml)
   std::string body = RESPONSE_1 + resultXml + RESPONSE_2;
   std::string header = generateHeader(body);
 
-  _response = header + body;
-  XmlRpcUtil::log(5, "XmlRpcServerConnection::generateResponse:\n%s\n", _response.c_str()); 
+  // Avoid an overly large response
+  if ((header.length() + body.length()) > size_t(__INT_MAX__)) {
+    XmlRpcUtil::error("XmlRpcServerConnection::generateResponse: response length (%u) exceeds the maximum allowed size (%u).",
+                      _response.length(), __INT_MAX__);
+    _response = "";
+  }
+  else {
+    _response = header + body;
+    XmlRpcUtil::log(5, "XmlRpcServerConnection::generateResponse:\n%s\n", _response.c_str());
+  }
 }
 
 // Prepend http headers

utilities/xmlrpcpp/src/XmlRpcSocket.cpp

@@ -302,6 +302,13 @@ XmlRpcSocket::nbRead(int fd, std::string& s, bool *eof)
       return false;   // Error
     }
   }
+  // Watch for integer overrun
+  if (s.length() > size_t(__INT_MAX__)) {
+    XmlRpcUtil::error("XmlRpcSocket::nbRead: text size (%u) exceeds the maximum allowed size (%s).",
+                      s.length(), __INT_MAX__);
+    s.clear();
+    return false;
+  }
   return true;
 }
 
@@ -310,6 +317,12 @@ XmlRpcSocket::nbRead(int fd, std::string& s, bool *eof)
 bool
 XmlRpcSocket::nbWrite(int fd, std::string& s, int *bytesSoFar)
 {
+  // Watch for integer overrun
+  if (s.length() > size_t(__INT_MAX__)) {
+    XmlRpcUtil::error("XmlRpcSocket::nbWrite: text size (%u) exceeds the maximum allowed size(%s)",
+                      s.length(), __INT_MAX__);
+    return false;
+  }
   int nToWrite = int(s.length()) - *bytesSoFar;
   char *sp = const_cast<char*>(s.c_str()) + *bytesSoFar;
   bool wouldBlock = false;

utilities/xmlrpcpp/src/XmlRpcUtil.cpp

@@ -108,6 +108,8 @@ void XmlRpcUtil::error(const char* fmt, ...)
 std::string 
 XmlRpcUtil::parseTag(const char* tag, std::string const& xml, int* offset)
 {
+  // avoid attempting to parse overly long xml input
+  if (xml.length() > size_t(__INT_MAX__)) return std::string();
   if (*offset >= int(xml.length())) return std::string();
   size_t istart = xml.find(tag, *offset);
   if (istart == std::string::npos) return std::string();
@@ -126,6 +128,7 @@ XmlRpcUtil::parseTag(const char* tag, std::string const& xml, int* offset)
 bool 
 XmlRpcUtil::findTag(const char* tag, std::string const& xml, int* offset)
 {
+  if (xml.length() > size_t(__INT_MAX__)) return false;
   if (*offset >= int(xml.length())) return false;
   size_t istart = xml.find(tag, *offset);
   if (istart == std::string::npos)
@@ -141,6 +144,7 @@ XmlRpcUtil::findTag(const char* tag, std::string const& xml, int* offset)
 bool 
 XmlRpcUtil::nextTagIs(const char* tag, std::string const& xml, int* offset)
 {
+  if (xml.length() > size_t(__INT_MAX__)) return false;
   if (*offset >= int(xml.length())) return false;
   const char* cp = xml.c_str() + *offset;
   int nc = 0;
@@ -162,6 +166,7 @@ XmlRpcUtil::nextTagIs(const char* tag, std::string const& xml, int* offset)
 std::string 
 XmlRpcUtil::getNextTag(std::string const& xml, int* offset)
 {
+  if (xml.length() > size_t(__INT_MAX__)) return std::string();
   if (*offset >= int(xml.length())) return std::string();
 
   size_t pos = *offset;