From 52514454189d8d8b49d1b5b2b37c08654feb64bc Mon Sep 17 00:00:00 2001
From: Even Rouault <even.rouault@spatialys.com>
Date: Thu, 7 Mar 2024 01:33:03 +0100
Subject: [PATCH] [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?
---
 src/server/qgsfcgiserverrequest.cpp        | 60 +++++++++++++---------
 tests/src/python/test_qgsserver_request.py | 43 ++++++++++++++++
 2 files changed, 80 insertions(+), 23 deletions(-)

diff --git a/src/server/qgsfcgiserverrequest.cpp b/src/server/qgsfcgiserverrequest.cpp
index 6d395c053a80..03d7d2ba9ef2 100644
--- 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
index ec83cd2d3b65..a5ffe7876919 100644
--- 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>')