[Server] QgsFcgiServerRequest::readData(): more validation

and enhance perf of reading from stdin. Note that for that later mode, I'm not sure if there are unit tests, but hopefully the OGC integration tests should cover this?
rouault · Mar 7, 2024 · 5251445 · 5251445
1 parent 5f1465f
commit 5251445
Show file tree

Hide file tree

Showing 2 changed files with 80 additions and 23 deletions.
diff --git a/src/server/qgsfcgiserverrequest.cpp b/src/server/qgsfcgiserverrequest.cpp
@@ -173,35 +173,49 @@ void QgsFcgiServerRequest::readData()
   if ( lengthstr )
   {
     bool success = false;
-    int length = QString( lengthstr ).toInt( &success );
-    // Note: REQUEST_BODY is not part of CGI standard, and it is not
-    // normally passed by any CGI web server and it is implemented only
-    // to allow unit tests to inject a request body and simulate a POST
-    // request
-    const char *request_body  = getenv( "REQUEST_BODY" );
-    if ( success && request_body )
+    const int length = QString( lengthstr ).toInt( &success );
+    if ( !success || length < 0 )
     {
-      QString body( request_body );
-      body.truncate( length );
-      mData.append( body.toUtf8() );
-      length = 0;
+      QgsMessageLog::logMessage( "fcgi: Invalid CONTENT_LENGTH",
+                                 QStringLiteral( "Server" ), Qgis::MessageLevel::Critical );
+      mHasError = true;
     }
+    else
+    {
+      // Note: REQUEST_BODY is not part of CGI standard, and it is not
+      // normally passed by any CGI web server and it is implemented only
+      // to allow unit tests to inject a request body and simulate a POST
+      // request
+      const char *request_body = getenv( "REQUEST_BODY" );
+
 #ifdef QGISDEBUG
-    qDebug() << "fcgi: reading " << lengthstr << " bytes from " << ( request_body ? "REQUEST_BODY" : "stdin" );
+      qDebug() << "fcgi: reading " << lengthstr << " bytes from " << ( request_body ? "REQUEST_BODY" : "stdin" );
 #endif
-    if ( success )
-    {
-      // XXX This not efficient at all  !!
-      for ( int i = 0; i < length; ++i )
+
+      if ( request_body )
       {
-        mData.append( getchar() );
+        const size_t request_body_length = strlen( request_body );
+        const int actual_length = static_cast<int>( std::min<size_t>( length, request_body_length ) );
+        if ( static_cast<size_t>( actual_length ) < request_body_length )
+        {
+          QgsMessageLog::logMessage( "fcgi: CONTENT_LENGTH is larger than actual length of REQUEST_BODY",
+                                     QStringLiteral( "Server" ), Qgis::MessageLevel::Critical );
+          mHasError = true;
+        }
+        mData = QByteArray::fromRawData( request_body, actual_length );
+      }
+      else
+      {
+        mData.resize( length );
+        const int actual_length = static_cast<int>( fread( mData.data(), 1, length, stdin ) );
+        if ( actual_length < length )
+        {
+          mData.resize( actual_length );
+          QgsMessageLog::logMessage( "fcgi: CONTENT_LENGTH is larger than actual length of stdin",
+                                     QStringLiteral( "Server" ), Qgis::MessageLevel::Critical );
+          mHasError = true;
+        }
       }
-    }
-    else
-    {
-      QgsMessageLog::logMessage( "fcgi: Failed to parse CONTENT_LENGTH",
-                                 QStringLiteral( "Server" ), Qgis::MessageLevel::Critical );
-      mHasError = true;
     }
   }
   else

diff --git a/tests/src/python/test_qgsserver_request.py b/tests/src/python/test_qgsserver_request.py
@@ -197,6 +197,48 @@ def _check_links(params, method='GET'):
         _check_links(params)
         _check_links(params, 'POST')
 
+    def test_fcgiRequestPOST_invalid_length_not_an_integer(self):
+        """Test post request handler with wrong CONTENT_LENGTH"""
+
+        data = '<Literal>+1</Literal>'
+        self._set_env({
+            'SERVER_NAME': 'www.myserver.com',
+            'SERVICE': 'WFS',
+            'REQUEST_BODY': data,
+            'CONTENT_LENGTH': "not an integer",
+            'REQUEST_METHOD': 'POST',
+        })
+        request = QgsFcgiServerRequest()
+        self.assertTrue(request.hasError())
+
+    def test_fcgiRequestPOST_invalid_length_negative(self):
+        """Test post request handler with wrong CONTENT_LENGTH"""
+
+        data = '<Literal>+1</Literal>'
+        self._set_env({
+            'SERVER_NAME': 'www.myserver.com',
+            'SERVICE': 'WFS',
+            'REQUEST_BODY': data,
+            'CONTENT_LENGTH': "-1",
+            'REQUEST_METHOD': 'POST',
+        })
+        request = QgsFcgiServerRequest()
+        self.assertTrue(request.hasError())
+
+    def test_fcgiRequestPOST_too_short_length(self):
+        """Test post request handler with wrong CONTENT_LENGTH"""
+
+        data = '<Literal>+1</Literal>'
+        self._set_env({
+            'SERVER_NAME': 'www.myserver.com',
+            'SERVICE': 'WFS',
+            'REQUEST_BODY': data,
+            'CONTENT_LENGTH': str(len(data) - 1),
+            'REQUEST_METHOD': 'POST',
+        })
+        request = QgsFcgiServerRequest()
+        self.assertTrue(request.hasError())
+
     def test_fcgiRequestBody(self):
         """Test request body"""
         data = '<Literal>+1</Literal>'
@@ -208,6 +250,7 @@ def test_fcgiRequestBody(self):
             'REQUEST_METHOD': 'POST',
         })
         request = QgsFcgiServerRequest()
+        self.assertFalse(request.hasError())
         response = QgsBufferServerResponse()
         self.server.handleRequest(request, response)
         self.assertEqual(request.parameter('REQUEST_BODY'), '<Literal>+1</Literal>')