From 5473db84e8f51e2e169c725a82c764c78bbbf7b2 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Reddy B Date: Tue, 23 Jun 2026 16:37:54 +0530 Subject: [PATCH 1/7] harden session parsing/validation and read_file bounds with regression tests --- source/jst_internal.c | 27 ++++++++++-- source/jst_session.c | 33 +++++++++++---- tests/CMakeLists.txt | 1 + tests/parser_test.cpp | 99 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 11 deletions(-) diff --git a/source/jst_internal.c b/source/jst_internal.c index e7312f0..ba0320b 100644 --- a/source/jst_internal.c +++ b/source/jst_internal.c @@ -164,6 +164,7 @@ int read_file(const char *filename, char** bufout, size_t* lenout) { FILE* pf; size_t size; + long tell_result; size_t rc; char* buf; @@ -179,9 +180,29 @@ int read_file(const char *filename, char** bufout, size_t* lenout) return 0; } - fseek(pf, 0, SEEK_END); - size = ftell(pf); - rewind(pf); + if(fseek(pf, 0, SEEK_END) != 0) + { + fclose(pf); + fprintf(stderr, "Error: seek failed %s\n", filename); + return 0; + } + + tell_result = ftell(pf); + if(tell_result < 0) + { + fclose(pf); + fprintf(stderr, "Error: tell failed %s\n", filename); + return 0; + } + + size = (size_t)tell_result; + + if(fseek(pf, 0, SEEK_SET) != 0) + { + fclose(pf); + fprintf(stderr, "Error: rewind failed %s\n", filename); + return 0; + } buf = (char*)calloc(size+1, 1); if(!buf) diff --git a/source/jst_session.c b/source/jst_session.c index ceec720..370707a 100644 --- a/source/jst_session.c +++ b/source/jst_session.c @@ -102,6 +102,9 @@ static duk_ret_t session_start(duk_context *ctx) CosaPhpExtLog("%s: cookie %s\n", __PRETTY_FUNCTION__, cookie); /*load session id from cookie*/ const char* sesid = NULL; + const char* sesid_end = NULL; + size_t sesid_len = 0; + char sesid_copy[SESSION_ID_LENGTH + 1]; const char* tmp = cookie; while (tmp = strstr(tmp, "DUKSID=")) { @@ -112,14 +115,29 @@ static duk_ret_t session_start(duk_context *ctx) if(sesid) { sesid += 7; - int len = strlen(sesid); - if(len >= SESSION_ID_LENGTH) + sesid_end = strchr(sesid, ';'); + if(sesid_end) + sesid_len = (size_t)(sesid_end - sesid); + else + sesid_len = strlen(sesid); + + if(sesid_len == SESSION_ID_LENGTH) { + memcpy(sesid_copy, sesid, SESSION_ID_LENGTH); + sesid_copy[SESSION_ID_LENGTH] = '\0'; + int idx = SESSION_PREFIX_LEN; int isvalid = 1; + + if(strncmp(sesid_copy, SESSION_PREFIX, SESSION_PREFIX_LEN) != 0) + { + CosaPhpExtLog("Invalid SessionID prefix\n"); + isvalid = 0; + } + /* Validate session ID*/ while ( idx < SESSION_ID_LENGTH) { - if (!isalnum(sesid[idx])) { + if (!isalnum(sesid_copy[idx])) { CosaPhpExtLog("Invalid SessionID\n"); isvalid = 0; break; @@ -128,20 +146,19 @@ static duk_ret_t session_start(duk_context *ctx) } if(isvalid) { - sesid = strtok(sesid, ";"); - const char filename[SESSION_FILE_MAX_PATH]; - snprintf(filename, SESSION_FILE_MAX_PATH, "%s/%s", SESSION_TMP_DIR, sesid); + char filename[SESSION_FILE_MAX_PATH]; + snprintf(filename, SESSION_FILE_MAX_PATH, "%s/%s", SESSION_TMP_DIR, sesid_copy); CosaPhpExtLog("%s: Checking for Session file %s\n", __PRETTY_FUNCTION__, filename); if (access(filename, F_OK) == 0) { CosaPhpExtLog("%s: Session file %s exists\n", __PRETTY_FUNCTION__, filename); - strncpy(session_identifier, sesid, SESSION_ID_LENGTH); + strncpy(session_identifier, sesid_copy, SESSION_ID_LENGTH); } else { CosaPhpExtLog("%s: Failed to read Session file %s\n", __PRETTY_FUNCTION__, filename); } } } else { - CosaPhpExtLog("Invalid SessionID Entropy\n"); + CosaPhpExtLog("Invalid SessionID Entropy\n"); } } } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2628f71..013f6ef 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -73,6 +73,7 @@ add_executable( parser_test ../tests/parser_test.cpp ../source/jst_parser.c + ../source/jst_session.c ../source/jst_internal.c ../source/duktape/duktape.c) target_link_libraries(parser_test libgtest libgmock -pthread) diff --git a/tests/parser_test.cpp b/tests/parser_test.cpp index cd97b54..6d95aa0 100644 --- a/tests/parser_test.cpp +++ b/tests/parser_test.cpp @@ -20,12 +20,19 @@ #include #include #include +#include +#include #include "jst.h" #include #include #include #include +extern "C" { + int read_file(const char *filename, char** bufout, size_t* lenout); + duk_ret_t ccsp_session_module_open(duk_context *ctx); +} + using namespace std; class BufferFreer @@ -106,6 +113,98 @@ TEST(general, parser) { } } +TEST(general, read_file_directory_input_returns_failure) +{ + char* buffer = NULL; + size_t length = 0; + + EXPECT_EQ(read_file("//", &buffer, &length), 0); + EXPECT_EQ(buffer, nullptr); +} + +TEST(general, session_start_does_not_modify_cookie_env) +{ + const char* sesid = "jst_sessABCDEFGHIJKLMNOPQRSTUVWXYZ012345"; + char session_file_path[128]; + char cookie[256]; + + snprintf(session_file_path, sizeof(session_file_path), "/tmp/%s", sesid); + FILE* f = fopen(session_file_path, "w"); + ASSERT_NE(f, nullptr); + fclose(f); + + snprintf(cookie, sizeof(cookie), "a=1; DUKSID=%s; b=2", sesid); + ASSERT_EQ(setenv("HTTP_COOKIE", cookie, 1), 0); + + duk_context* ctx = duk_create_heap_default(); + ASSERT_NE(ctx, nullptr); + + duk_push_c_function(ctx, ccsp_session_module_open, 0); + duk_call(ctx, 0); + duk_put_global_string(ctx, "ccsp_session"); + + duk_get_global_string(ctx, "ccsp_session"); + duk_get_prop_string(ctx, -1, "destroy"); + ASSERT_EQ(duk_pcall(ctx, 0), DUK_EXEC_SUCCESS); + duk_pop_2(ctx); + + duk_get_global_string(ctx, "ccsp_session"); + duk_get_prop_string(ctx, -1, "start"); + ASSERT_EQ(duk_pcall(ctx, 0), DUK_EXEC_SUCCESS); + ASSERT_TRUE(duk_get_boolean(ctx, -1)); + duk_pop_2(ctx); + + const char* cookie_after = getenv("HTTP_COOKIE"); + ASSERT_NE(cookie_after, nullptr); + EXPECT_STREQ(cookie_after, cookie); + + duk_get_global_string(ctx, "ccsp_session"); + duk_get_prop_string(ctx, -1, "destroy"); + ASSERT_EQ(duk_pcall(ctx, 0), DUK_EXEC_SUCCESS); + duk_pop_2(ctx); + + duk_destroy_heap(ctx); + + remove(session_file_path); +} + +TEST(general, session_start_rejects_invalid_session_prefix) +{ + const char* invalid_sesid = "bad_prefABCDEFGHIJKLMNOPQRSTUVWXYZ012345"; + char session_file_path[128]; + char cookie[256]; + + snprintf(session_file_path, sizeof(session_file_path), "/tmp/%s", invalid_sesid); + FILE* f = fopen(session_file_path, "w"); + ASSERT_NE(f, nullptr); + fclose(f); + + snprintf(cookie, sizeof(cookie), "DUKSID=%s", invalid_sesid); + ASSERT_EQ(setenv("HTTP_COOKIE", cookie, 1), 0); + + duk_context* ctx = duk_create_heap_default(); + ASSERT_NE(ctx, nullptr); + + duk_push_c_function(ctx, ccsp_session_module_open, 0); + duk_call(ctx, 0); + duk_put_global_string(ctx, "ccsp_session"); + + duk_get_global_string(ctx, "ccsp_session"); + duk_get_prop_string(ctx, -1, "destroy"); + ASSERT_EQ(duk_pcall(ctx, 0), DUK_EXEC_SUCCESS); + duk_pop_2(ctx); + + duk_get_global_string(ctx, "ccsp_session"); + duk_get_prop_string(ctx, -1, "start"); + ASSERT_EQ(duk_pcall(ctx, 0), DUK_EXEC_SUCCESS); + EXPECT_FALSE(duk_get_boolean(ctx, -1)); + duk_pop_2(ctx); + + duk_destroy_heap(ctx); + + remove(session_file_path); +} + int main(int argc, char* argv[]) { ::testing::InitGoogleTest(&argc, argv); From 940605427ddf2813419fa13da512592280da35aa Mon Sep 17 00:00:00 2001 From: Pavan Kumar Reddy B Date: Tue, 23 Jun 2026 17:03:57 +0530 Subject: [PATCH 2/7] fix(session/exec/tests): close leak/state bugs and harden process+temp handling --- source/jst_session.c | 6 ++++-- tests/parser_test.cpp | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/source/jst_session.c b/source/jst_session.c index 370707a..ef91fad 100644 --- a/source/jst_session.c +++ b/source/jst_session.c @@ -137,7 +137,7 @@ static duk_ret_t session_start(duk_context *ctx) /* Validate session ID*/ while ( idx < SESSION_ID_LENGTH) { - if (!isalnum(sesid_copy[idx])) { + if (!isalnum((unsigned char)sesid_copy[idx])) { CosaPhpExtLog("Invalid SessionID\n"); isvalid = 0; break; @@ -165,6 +165,8 @@ static duk_ret_t session_start(duk_context *ctx) if(!session_identifier[0]) { CosaPhpExtLog("Invalid Session\n"); + free(session_identifier); + session_identifier = NULL; RETURN_FALSE; } @@ -304,7 +306,7 @@ static duk_ret_t session_get_data(duk_context *ctx) /*check if we are at end of file content, ignoring whitespace*/ for(j = i+1; j < content_len; ++j) { - if(!isspace(s1[j])) + if(!isspace((unsigned char)s1[j])) { /*more content found*/ break; diff --git a/tests/parser_test.cpp b/tests/parser_test.cpp index 6d95aa0..e6fb424 100644 --- a/tests/parser_test.cpp +++ b/tests/parser_test.cpp @@ -48,6 +48,34 @@ class BufferFreer char* buffer_; }; +class EnvVarGuard +{ +public: + explicit EnvVarGuard(const char* name) + : name_(name), had_value_(false) + { + const char* value = getenv(name_); + if (value) + { + old_value_ = value; + had_value_ = true; + } + } + + ~EnvVarGuard() + { + if (had_value_) + setenv(name_, old_value_.c_str(), 1); + else + unsetenv(name_); + } + +private: + const char* name_; + std::string old_value_; + bool had_value_; +}; + int recurseDirectory(const string& path, vector& files, const string& match) { DIR *dir; @@ -124,6 +152,8 @@ TEST(general, read_file_directory_input_returns_failure) TEST(general, session_start_does_not_modify_cookie_env) { + EnvVarGuard http_cookie_guard("HTTP_COOKIE"); + const char* sesid = "jst_sessABCDEFGHIJKLMNOPQRSTUVWXYZ012345"; char session_file_path[128]; char cookie[256]; @@ -170,6 +200,8 @@ TEST(general, session_start_does_not_modify_cookie_env) TEST(general, session_start_rejects_invalid_session_prefix) { + EnvVarGuard http_cookie_guard("HTTP_COOKIE"); + const char* invalid_sesid = "bad_prefABCDEFGHIJKLMNOPQRSTUVWXYZ012345"; char session_file_path[128]; char cookie[256]; From 6ee747762a7b3de158a582fd3837df599236765c Mon Sep 17 00:00:00 2001 From: Pavan Kumar Reddy B <57708013+pavankumar464@users.noreply.github.com> Date: Tue, 14 Jul 2026 18:59:09 +0530 Subject: [PATCH 3/7] Potential fix for pull request finding Fix to report failures cleanly. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- tests/parser_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/parser_test.cpp b/tests/parser_test.cpp index 5a6a58d..528044f 100644 --- a/tests/parser_test.cpp +++ b/tests/parser_test.cpp @@ -250,7 +250,7 @@ TEST(general, session_start_does_not_modify_cookie_env) ASSERT_NE(ctx, nullptr); duk_push_c_function(ctx, ccsp_session_module_open, 0); - duk_call(ctx, 0); + ASSERT_EQ(duk_pcall(ctx, 0), DUK_EXEC_SUCCESS); duk_put_global_string(ctx, "ccsp_session"); duk_get_global_string(ctx, "ccsp_session"); From a87f4b8c37a1d3c3f4ef9b1e5d4f788697145ca6 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Reddy B <57708013+pavankumar464@users.noreply.github.com> Date: Tue, 14 Jul 2026 18:59:37 +0530 Subject: [PATCH 4/7] Potential fix for pull request finding Using duk_call() will throw longjmp/fatal on a Duktape error, which can crash the test process. Prefer duk_pcall() and assert DUK_EXEC_SUCCESS so failures are reported cleanly. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- tests/parser_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/parser_test.cpp b/tests/parser_test.cpp index 528044f..cdb84ff 100644 --- a/tests/parser_test.cpp +++ b/tests/parser_test.cpp @@ -298,7 +298,7 @@ TEST(general, session_start_rejects_invalid_session_prefix) ASSERT_NE(ctx, nullptr); duk_push_c_function(ctx, ccsp_session_module_open, 0); - duk_call(ctx, 0); + ASSERT_EQ(duk_pcall(ctx, 0), DUK_EXEC_SUCCESS); duk_put_global_string(ctx, "ccsp_session"); duk_get_global_string(ctx, "ccsp_session"); From d64fdf81482309d20f3896ea268c78e640c0a038 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Reddy B <57708013+pavankumar464@users.noreply.github.com> Date: Tue, 14 Jul 2026 19:00:47 +0530 Subject: [PATCH 5/7] Potential fix for pull request finding Mark EnvVarGuard non-copyable Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- tests/parser_test.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/parser_test.cpp b/tests/parser_test.cpp index cdb84ff..620b499 100644 --- a/tests/parser_test.cpp +++ b/tests/parser_test.cpp @@ -71,6 +71,9 @@ class EnvVarGuard unsetenv(name_); } + EnvVarGuard(const EnvVarGuard&) = delete; + EnvVarGuard& operator=(const EnvVarGuard&) = delete; + private: const char* name_; std::string old_value_; From d60dfa16fd8f955518902757735306136b777404 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Reddy B <57708013+pavankumar464@users.noreply.github.com> Date: Tue, 14 Jul 2026 19:01:54 +0530 Subject: [PATCH 6/7] Potential fix for pull request finding Added truncation/overflow guard before allocating. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- source/jst_internal.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/source/jst_internal.c b/source/jst_internal.c index ba0320b..ec00e24 100644 --- a/source/jst_internal.c +++ b/source/jst_internal.c @@ -196,6 +196,12 @@ int read_file(const char *filename, char** bufout, size_t* lenout) } size = (size_t)tell_result; + if ((long)size != tell_result || size == (size_t)-1) + { + fclose(pf); + fprintf(stderr, "Error: file too large %s\n", filename); + return 0; + } if(fseek(pf, 0, SEEK_SET) != 0) { From a5197b81def70eda9efbc6a6cbcb6d8d5a87a931 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Reddy B Date: Tue, 14 Jul 2026 19:05:38 +0530 Subject: [PATCH 7/7] fix(read_file): initialize out params before early returns --- source/jst_internal.c | 15 +++++++++++++++ tests/parser_test.cpp | 5 +++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/source/jst_internal.c b/source/jst_internal.c index ec00e24..3de8de2 100644 --- a/source/jst_internal.c +++ b/source/jst_internal.c @@ -168,8 +168,23 @@ int read_file(const char *filename, char** bufout, size_t* lenout) size_t rc; char* buf; + if(!bufout || !lenout) + { + CosaPhpExtLog("read_file invalid output buffer for %s\n", filename ? filename : "(null)"); + return 0; + } + + *bufout = NULL; + *lenout = 0; + CosaPhpExtLog( "read_file %s\n", filename ); + if(!filename) + { + fprintf(stderr, "Error: invalid filename\n"); + return 0; + } + errno = 0; pf = fopen(filename, "r"); if(!pf) diff --git a/tests/parser_test.cpp b/tests/parser_test.cpp index 620b499..b523dad 100644 --- a/tests/parser_test.cpp +++ b/tests/parser_test.cpp @@ -147,11 +147,12 @@ TEST(general, parser) { TEST(general, read_file_directory_input_returns_failure) { - char* buffer = NULL; - size_t length = 0; + char* buffer = reinterpret_cast(0x1); + size_t length = 123; EXPECT_EQ(read_file("//", &buffer, &length), 0); EXPECT_EQ(buffer, nullptr); + EXPECT_EQ(length, 0u); } TEST(general, session_create_multiple_calls_succeed)