MSSQLSpatial: fix potential SQL injection of value of MSSQLSPATIAL_OG…

…R_FID config option (https://github.com/OSGeo/gdal/security/code-scanning/525)
rouault · Mar 18, 2024 · 8a8b04a · 8a8b04a
1 parent 43beb38
commit 8a8b04a
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 23 deletions.
diff --git a/ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatialtablelayer.cpp b/ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatialtablelayer.cpp
@@ -32,6 +32,7 @@
 #include "ogr_p.h"
 
 #include <memory>
+#include <string_view>
 
 #define UNSUPPORTED_OP_READ_ONLY                                               \
     "%s : unsupported operation on a read-only datasource."
@@ -466,6 +467,26 @@ void OGRMSSQLSpatialTableLayer::DropSpatialIndex()
     }
 }
 
+/************************************************************************/
+/*                     GetBracketEscapedIdentifier()                    */
+/************************************************************************/
+
+static std::string GetBracketEscapedIdentifier(const std::string_view &osStr)
+{
+    std::string osRet("[");
+    osRet.reserve(osStr.size());
+    for (char ch : osStr)
+    {
+        if (ch == ']')
+        {
+            osRet += ch;
+        }
+        osRet += ch;
+    }
+    osRet += ']';
+    return osRet;
+}
+
 /************************************************************************/
 /*                            BuildFields()                             */
 /*                                                                      */
@@ -484,9 +505,7 @@ CPLString OGRMSSQLSpatialTableLayer::BuildFields()
     if (pszFIDColumn && poFeatureDefn->GetFieldIndex(pszFIDColumn) == -1)
     {
         /* Always get the FID column */
-        osFieldList += "[";
-        osFieldList += pszFIDColumn;
-        osFieldList += "]";
+        osFieldList += GetBracketEscapedIdentifier(pszFIDColumn);
         ++nColumn;
     }
 
@@ -495,29 +514,27 @@ CPLString OGRMSSQLSpatialTableLayer::BuildFields()
         if (nColumn > 0)
             osFieldList += ", ";
 
-        osFieldList += "[";
-        osFieldList += pszGeomColumn;
+        osFieldList += GetBracketEscapedIdentifier(pszGeomColumn);
         if (nGeomColumnType == MSSQLCOLTYPE_GEOMETRY ||
             nGeomColumnType == MSSQLCOLTYPE_GEOGRAPHY)
         {
             if (poDS->GetGeometryFormat() == MSSQLGEOMETRY_WKB)
             {
-                osFieldList += "].STAsBinary() as [";
-                osFieldList += pszGeomColumn;
+                osFieldList += ".STAsBinary() as ";
+                osFieldList += GetBracketEscapedIdentifier(pszGeomColumn);
             }
             else if (poDS->GetGeometryFormat() == MSSQLGEOMETRY_WKT)
             {
-                osFieldList += "].AsTextZM() as [";
-                osFieldList += pszGeomColumn;
+                osFieldList += ".AsTextZM() as ";
+                osFieldList += GetBracketEscapedIdentifier(pszGeomColumn);
             }
             else if (poDS->GetGeometryFormat() == MSSQLGEOMETRY_WKBZM)
             {
                 /* SQL Server 2012 */
-                osFieldList += "].AsBinaryZM() as [";
-                osFieldList += pszGeomColumn;
+                osFieldList += ".AsBinaryZM() as ";
+                osFieldList += GetBracketEscapedIdentifier(pszGeomColumn);
             }
         }
-        osFieldList += "]";
 
         ++nColumn;
     }
@@ -539,9 +556,7 @@ CPLString OGRMSSQLSpatialTableLayer::BuildFields()
             if (nColumn > 0)
                 osFieldList += ", ";
 
-            osFieldList += "[";
-            osFieldList += pszName;
-            osFieldList += "]";
+            osFieldList += GetBracketEscapedIdentifier(pszName);
 
             panFieldOrdinals[i] = nColumn;
 
@@ -578,11 +593,10 @@ OGRMSSQLSpatialTableLayer::BuildStatement(const char *pszColumns)
     CPLODBCStatement *poStatement = new CPLODBCStatement(poDS->GetSession());
     poStatement->Append("select ");
     poStatement->Append(pszColumns);
-    poStatement->Append(" from [");
-    poStatement->Append(pszSchemaName);
-    poStatement->Append("].[");
-    poStatement->Append(pszTableName);
-    poStatement->Append("]");
+    poStatement->Append(" from ");
+    poStatement->Append(GetBracketEscapedIdentifier(pszSchemaName));
+    poStatement->Append(".");
+    poStatement->Append(GetBracketEscapedIdentifier(pszTableName));
 
     /* Append attribute query if we have it */
     if (pszQuery != nullptr)
@@ -600,11 +614,12 @@ OGRMSSQLSpatialTableLayer::BuildStatement(const char *pszColumns)
                 !CPLIsInf(m_sFilterEnvelope.MaxY))
             {
                 if (pszQuery == nullptr)
-                    poStatement->Append(" where");
+                    poStatement->Append(" where ");
                 else
-                    poStatement->Append(" and");
+                    poStatement->Append(" and ");
 
-                poStatement->Appendf(" [%s].STIntersects(", pszGeomColumn);
+                poStatement->Append(GetBracketEscapedIdentifier(pszGeomColumn));
+                poStatement->Append(".STIntersects(");
 
                 if (nGeomColumnType == MSSQLCOLTYPE_GEOGRAPHY)
                     poStatement->Append("geography::");

diff --git a/port/cpl_odbc.cpp b/port/cpl_odbc.cpp
@@ -1644,6 +1644,24 @@ void CPLODBCStatement::Append(const char *pszText)
     m_nStatementLen += nTextLen;
 }
 
+/************************************************************************/
+/*                      Append(const std::string &)                     */
+/************************************************************************/
+
+/**
+ * Append text to internal command.
+ *
+ * The passed text is appended to the internal SQL command text.
+ *
+ * @param s text to append.
+ */
+
+void CPLODBCStatement::Append(const std::string &s)
+
+{
+    Append(s.c_str());
+}
+
 /************************************************************************/
 /*                     AppendEscaped(const char *)                      */
 /************************************************************************/

diff --git a/port/cpl_odbc.h b/port/cpl_odbc.h
@@ -320,6 +320,7 @@ class CPL_DLL CPLODBCStatement
     void Clear();
     void AppendEscaped(const char *);
     void Append(const char *);
+    void Append(const std::string &);
     // cppcheck-suppress functionStatic
     void Append(int);
     void Append(double);