Security scan issues for v3.5.1

[sohankunkerkar]

Describe the bug
We have been using the Catch2 package to build the Wasmedge package in Red Hat Enterprise Linux-9. Our internal SAST(Static application security testing) scan is designed to identify vulnerabilities in the code. The scanner detected several issues in Catch v3.5.1.

Expected behavior
The SAST scanner should not detect these issues.

Reproduction steps
Steps to reproduce the bug.
I don't have the exact steps to reproduce these issues, but I can provide the configuration of the tools used for scanning this package.

Platform information:

• OS: Linux
• Compiler+version: latest
• Catch version: v3.5.1

Additional context
Add any other context about the problem here.

[sohankunkerkar]

I came up with this patch. Let me know if this makes sense:

From e3415c1d13232c4ad1286931c1151fc6ae2366d8 Mon Sep 17 00:00:00 2001
From: Sohan Kunkerkar <sohank2602@gmail.com>
Date: Tue, 9 Jan 2024 16:04:19 -0500
Subject: [PATCH] Fix rhel-9 SAST scan issues

---
 src/catch2/catch_test_case_info.cpp                     | 6 +++++-
 src/catch2/internal/catch_random_seed_generation.cpp    | 6 +++---
 src/catch2/internal/catch_random_seed_generation.hpp    | 2 +-
 src/catch2/reporters/catch_reporter_cumulative_base.cpp | 2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/src/catch2/catch_test_case_info.cpp b/src/catch2/catch_test_case_info.cpp
index c38ee55a..9cd76be9 100644
--- a/src/catch2/catch_test_case_info.cpp
+++ b/src/catch2/catch_test_case_info.cpp
@@ -88,8 +88,12 @@ namespace Catch {
             --lastDot;
 
             size_t nameStart = lastDot;
-            while (nameStart > 0 && filename[nameStart - 1] != '/' && filename[nameStart - 1] != '\\') {
+            if (nameStart < filename.size()) {
+               while (nameStart > 0 && filename[nameStart - 1] != '/' && filename[nameStart - 1] != '\\') {
                 --nameStart;
+                }
+            } else {
+                nameStart = 0;
             }
 
             return filename.substr(nameStart, lastDot - nameStart);
diff --git a/src/catch2/internal/catch_random_seed_generation.cpp b/src/catch2/internal/catch_random_seed_generation.cpp
index fdc3fa19..6ae880aa 100644
--- a/src/catch2/internal/catch_random_seed_generation.cpp
+++ b/src/catch2/internal/catch_random_seed_generation.cpp
@@ -16,15 +16,15 @@
 
 namespace Catch {
 
-    std::uint32_t generateRandomSeed( GenerateFrom from ) {
+    std::uint64_t generateRandomSeed( GenerateFrom from ) {
         switch ( from ) {
         case GenerateFrom::Time:
-            return static_cast<std::uint32_t>( std::time( nullptr ) );
+            return static_cast<std::uint64_t>( std::time( nullptr ) );
 
         case GenerateFrom::Default:
         case GenerateFrom::RandomDevice: {
             std::random_device rd;
-            return Detail::fillBitsFrom<std::uint32_t>( rd );
+            return Detail::fillBitsFrom<std::uint64_t>( rd );
         }
 
         default:
diff --git a/src/catch2/internal/catch_random_seed_generation.hpp b/src/catch2/internal/catch_random_seed_generation.hpp
index d0d6fb24..3c4e03d6 100644
--- a/src/catch2/internal/catch_random_seed_generation.hpp
+++ b/src/catch2/internal/catch_random_seed_generation.hpp
@@ -19,7 +19,7 @@ namespace Catch {
         Default
     };
 
-    std::uint32_t generateRandomSeed(GenerateFrom from);
+    std::uint64_t generateRandomSeed(GenerateFrom from);
 
 } // end namespace Catch
 
diff --git a/src/catch2/reporters/catch_reporter_cumulative_base.cpp b/src/catch2/reporters/catch_reporter_cumulative_base.cpp
index 5e106326..e43c3a23 100644
--- a/src/catch2/reporters/catch_reporter_cumulative_base.cpp
+++ b/src/catch2/reporters/catch_reporter_cumulative_base.cpp
@@ -87,8 +87,8 @@ namespace Catch {
             if ( it == parentNode.childSections.end() ) {
                 auto newNode =
                     Detail::make_unique<SectionNode>( incompleteStats );
-                node = newNode.get();
                 parentNode.childSections.push_back( CATCH_MOVE( newNode ) );
+                node = newNode.get();
             } else {
                 node = it->get();
             }
-- 
2.41.0

[sohankunkerkar]

cc @horenmar

[horenmar]

In reverse order:

• The proposed changes for reporter_cumulative_base.cpp might make the warning go away, but they also completely break the section trackers. node exists to paper over getting a non-owning pointer to an object that is owned by parent tracker. There is no UAF there and both warnings from your tool are wrong (and honestly somewhat contradictory. Am I using an internal representation of the object, or am I using a freed pointer? These are obviously different cases)
• The rest of the code uses 32 bit rng seeds, there is no reason to generate more bits to toss them away later.
• Overflowing the search for . in a filename might actually cause an issue and I'll make a note to fix it. I do not consider this priority, because it requires combination of a cpp file without any extension in the name, and -# flag.