harden session parsing/validation and read_file bounds with regressio…

source/jst_internal.c

@@ -1,5 +1,5 @@
 /*
  If not stated otherwise in this file or this component's Licenses.txt file the
  following copyright and licenses apply:
 
  Copyright 2018 RDK Management
@@ -164,6 +164,7 @@
 {
   FILE* pf;
   size_t size;
+  long tell_result;
   size_t rc;
   char* buf;
 
@@ -179,9 +180,29 @@
     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)

source/jst_session.c

@@ -1,5 +1,5 @@
 /*
  If not stated otherwise in this file or this component's Licenses.txt file the
  following copyright and licenses apply:
 
  Copyright 2018 RDK Management
@@ -102,6 +102,9 @@
     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 @@
     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((unsigned char)sesid_copy[idx])) {
                       CosaPhpExtLog("Invalid SessionID\n");
                       isvalid = 0;
                       break;
@@ -128,26 +146,27 @@
            }
            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");
       }
     }
   }
   if(!session_identifier[0])
   {
    CosaPhpExtLog("Invalid Session\n");
+    free(session_identifier);
+    session_identifier = NULL;
    RETURN_FALSE;
   }
 
@@ -287,7 +306,7 @@
           /*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;

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)

tests/parser_test.cpp

@@ -1,5 +1,5 @@
 /*
  If not stated otherwise in this file or this component's Licenses.txt file the
  following copyright and licenses apply:
 
  Copyright 2018 RDK Management
@@ -20,12 +20,19 @@
 #include <string>
 #include <fstream>
 #include <streambuf>
+#include <cstdio>
+#include <cstdlib>
 #include "jst.h"
 #include <dirent.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <unistd.h>
 
+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
@@ -41,6 +48,34 @@
   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<string>& files, const string& match)
 {
   DIR *dir;
@@ -106,6 +141,102 @@
   }
 }
 
+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)
+{
+  EnvVarGuard http_cookie_guard("HTTP_COOKIE");
+
+  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)
+{
+  EnvVarGuard http_cookie_guard("HTTP_COOKIE");
+
+  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);