Fix read heap-buffer-overflow on nested /vsitar/ calls

Fixes https://issues.oss-fuzz.com/issues/388868487 This is just a band-aid... The use of CPLFormFilename() and friends is so dangerous with the rotating TLS buffers that may end up overwritten if too many calls are nested...
rouault · Jan 11, 2025 · c80a461 · c80a461
1 parent ea26bd0
commit c80a461
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 11 deletions.
diff --git a/port/cpl_path.cpp b/port/cpl_path.cpp
@@ -801,8 +801,12 @@ const char *CPLFormCIFilename(const char *pszPath, const char *pszBasename,
             pszFilename[i] = static_cast<char>(CPLToupper(pszFilename[i]));
         }
 
-        pszFullPath = CPLFormFilename(pszPath, pszFilename, nullptr);
-        nStatRet = VSIStatExL(pszFullPath, &sStatBuf, VSI_STAT_EXISTS_FLAG);
+        const std::string osTmpPath(
+            CPLFormFilename(pszPath, pszFilename, nullptr));
+        nStatRet =
+            VSIStatExL(osTmpPath.c_str(), &sStatBuf, VSI_STAT_EXISTS_FLAG);
+        if (nStatRet == 0)
+            pszFullPath = CPLFormFilename(pszPath, pszFilename, nullptr);
     }
 
     if (nStatRet != 0)
@@ -813,8 +817,12 @@ const char *CPLFormCIFilename(const char *pszPath, const char *pszBasename,
                 CPLTolower(static_cast<unsigned char>(pszFilename[i])));
         }
 
-        pszFullPath = CPLFormFilename(pszPath, pszFilename, nullptr);
-        nStatRet = VSIStatExL(pszFullPath, &sStatBuf, VSI_STAT_EXISTS_FLAG);
+        const std::string osTmpPath(
+            CPLFormFilename(pszPath, pszFilename, nullptr));
+        nStatRet =
+            VSIStatExL(osTmpPath.c_str(), &sStatBuf, VSI_STAT_EXISTS_FLAG);
+        if (nStatRet == 0)
+            pszFullPath = CPLFormFilename(pszPath, pszFilename, nullptr);
     }
 
     if (nStatRet != 0)

diff --git a/port/cpl_vsil_abstract_archive.cpp b/port/cpl_vsil_abstract_archive.cpp
@@ -452,15 +452,20 @@ char *VSIArchiveFilesystemHandler::SplitFilename(const char *pszFilename,
 
     const std::vector<CPLString> oExtensions = GetExtensions();
     int nAttempts = 0;
-    while (pszFilename[i])
+    // If we are called with pszFilename being one of the TLS buffers returned
+    // by cpl_path.cpp functions, then a call to Stat() in the loop (and its
+    // cascaded calls to other cpl_path.cpp functions) might lead to altering
+    // the buffer to be altered, hence take a copy
+    const std::string osFilenameCopy(pszFilename);
+    while (i < static_cast<int>(osFilenameCopy.size()))
     {
         int nToSkip = 0;
 
         for (std::vector<CPLString>::const_iterator iter = oExtensions.begin();
              iter != oExtensions.end(); ++iter)
         {
             const CPLString &osExtension = *iter;
-            if (EQUALN(pszFilename + i, osExtension.c_str(),
+            if (EQUALN(osFilenameCopy.c_str() + i, osExtension.c_str(),
                        osExtension.size()))
             {
                 nToSkip = static_cast<int>(osExtension.size());
@@ -470,7 +475,8 @@ char *VSIArchiveFilesystemHandler::SplitFilename(const char *pszFilename,
 
 #ifdef DEBUG
         // For AFL, so that .cur_input is detected as the archive filename.
-        if (EQUALN(pszFilename + i, ".cur_input", strlen(".cur_input")))
+        if (EQUALN(osFilenameCopy.c_str() + i, ".cur_input",
+                   strlen(".cur_input")))
         {
             nToSkip = static_cast<int>(strlen(".cur_input"));
         }
@@ -486,7 +492,7 @@ char *VSIArchiveFilesystemHandler::SplitFilename(const char *pszFilename,
                 break;
             }
             VSIStatBufL statBuf;
-            char *archiveFilename = CPLStrdup(pszFilename);
+            char *archiveFilename = CPLStrdup(osFilenameCopy.c_str());
             bool bArchiveFileExists = false;
 
             if (IsEitherSlash(archiveFilename[i + nToSkip]))
@@ -527,10 +533,11 @@ char *VSIArchiveFilesystemHandler::SplitFilename(const char *pszFilename,
 
             if (bArchiveFileExists)
             {
-                if (IsEitherSlash(pszFilename[i + nToSkip]))
+                if (IsEitherSlash(osFilenameCopy[i + nToSkip]) &&
+                    i + nToSkip + 1 < static_cast<int>(osFilenameCopy.size()))
                 {
-                    osFileInArchive =
-                        CompactFilename(pszFilename + i + nToSkip + 1);
+                    osFileInArchive = CompactFilename(osFilenameCopy.c_str() +
+                                                      i + nToSkip + 1);
                 }
                 else
                 {
@@ -545,12 +552,22 @@ char *VSIArchiveFilesystemHandler::SplitFilename(const char *pszFilename,
                         osFileInArchive.pop_back();
                 }
 
+                // Messy! Restore the TLS buffer if it has been altered
+                if (osFilenameCopy != pszFilename)
+                    strcpy(const_cast<char *>(pszFilename),
+                           osFilenameCopy.c_str());
+
                 return archiveFilename;
             }
             CPLFree(archiveFilename);
         }
         i++;
     }
+
+    // Messy! Restore the TLS buffer if it has been altered
+    if (osFilenameCopy != pszFilename)
+        strcpy(const_cast<char *>(pszFilename), osFilenameCopy.c_str());
+
     return nullptr;
 }