RDKB-65597 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,30 +164,74 @@
 {
   FILE* pf;
   size_t size;
+  long tell_result;
   size_t rc;
   char* buf;
+  const char* safe_filename = (filename && filename[0]) ? filename : "<invalid filename>";
 
-  CosaPhpExtLog( "read_file %s\n", filename );
+  if(!bufout || !lenout)
+  {
+    CosaPhpExtLog("read_file invalid output buffer for %s\n", safe_filename);
+    return 0;
+  }
+
+  *bufout = NULL;
+  *lenout = 0;
+
+  CosaPhpExtLog("read_file %s\n", safe_filename);
+
+  if(!filename || !filename[0])
+  {
+    CosaPhpExtLog("read_file invalid filename %s\n", safe_filename);
+    fprintf(stderr, "Error: invalid filename %s\n", safe_filename);
+    return 0;
+  }
 
   errno = 0;
   pf = fopen(filename, "r");
   if(!pf)
   {
     char * serr = strerror(errno);
-    CosaPhpExtLog( "read_file cannot open file:%s error:%s\n", filename, serr );
-    fprintf(stderr, "Error: cannot open file:%s error:%s\n", filename, serr);
+    CosaPhpExtLog( "read_file cannot open file:%s error:%s\n", safe_filename, serr );
+    fprintf(stderr, "Error: cannot open file:%s error:%s\n", safe_filename, serr);
+    return 0;
+  }
+
+  if(fseek(pf, 0, SEEK_END) != 0)
+  {
+    fclose(pf);
+    fprintf(stderr, "Error: seek failed %s\n", safe_filename);
     return 0;
   }
 
-  fseek(pf, 0, SEEK_END); 
-  size = ftell(pf);
-  rewind(pf);
+  tell_result = ftell(pf);
+  if(tell_result < 0)
+  {
+    fclose(pf);
+    fprintf(stderr, "Error: tell failed %s\n", safe_filename);
+    return 0;
+  }
+
+  size = (size_t)tell_result;
+  if ((long)size != tell_result || size == (size_t)-1 || (size_t)(int)size != size)
+  {
+    fclose(pf);
+    fprintf(stderr, "Error: file too large %s\n", safe_filename);
+    return 0;
+  }
+
+  if(fseek(pf, 0, SEEK_SET) != 0)
+  {
+    fclose(pf);
+    fprintf(stderr, "Error: rewind failed %s\n", safe_filename);
+    return 0;
+  }
 
   buf = (char*)calloc(size+1, 1);
   if(!buf)
   {
     fclose(pf);
-    fprintf(stderr, "Error: malloc oom %s\n", filename);
+    fprintf(stderr, "Error: malloc oom %s\n", safe_filename);
     return 0;
   }
 
@@ -196,7 +240,7 @@
   {
     free(buf);
     fclose(pf);
-    fprintf(stderr, "Error: read failed %s\n", filename);
+    fprintf(stderr, "Error: read failed %s\n", safe_filename);
     return 0;
   }
 

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,24 +102,49 @@
     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="))
+    while ((tmp = strstr(tmp, "DUKSID=")))
     {
       sesid= tmp;
       tmp++;
     }
-    CosaPhpExtLog("%s: sesid %s\n", __PRETTY_FUNCTION__, sesid);
     if(sesid)
     {
+      CosaPhpExtLog("%s: sesid %s\n", __PRETTY_FUNCTION__, 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);
+    }
+    else
+    {
+      CosaPhpExtLog("%s: sesid <not found>\n", __PRETTY_FUNCTION__);
+    }
+
+    if(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 +153,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;
   }
 
@@ -304,7 +330,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/parser_test.cpp

@@ -31,6 +31,7 @@
 #include <unistd.h>
 
 extern "C" {
+  int read_file(const char *filename, char** bufout, size_t* lenout);
   duk_ret_t ccsp_post_module_open(duk_context *ctx);
   duk_ret_t ccsp_session_module_open(duk_context *ctx);
 }
@@ -72,6 +73,9 @@ class EnvVarGuard
       unsetenv(name_);
   }
 
+  EnvVarGuard(const EnvVarGuard&) = delete;
+  EnvVarGuard& operator=(const EnvVarGuard&) = delete;
+
   void set(const char* value)
   {
     if (value)
@@ -214,6 +218,21 @@ TEST(general, parser) {
   }
 }
 
+TEST(general, read_file_directory_input_returns_failure)
+{
+  char temp_dir_template[] = "/tmp/jst_read_file_directory_input_XXXXXX";
+  char* buffer = reinterpret_cast<char*>(0x1);
+  size_t length = 123;
+  char* temp_dir = mkdtemp(temp_dir_template);
+
+  ASSERT_NE(temp_dir, nullptr);
+
+  EXPECT_EQ(read_file(temp_dir, &buffer, &length), 0);
+  EXPECT_EQ(buffer, nullptr);
+  EXPECT_EQ(length, 0u);
+  EXPECT_EQ(rmdir(temp_dir), 0);
+}
+
 TEST(general, session_create_multiple_calls_succeed)
 {
   duk_context* ctx = duk_create_heap_default();
@@ -341,6 +360,93 @@ TEST(general, session_create_destroy_cycle_and_id_format)
   duk_destroy_heap(ctx);
 }
 
+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);
+  ASSERT_EQ(duk_pcall(ctx, 0), DUK_EXEC_SUCCESS);
+  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);
+  ASSERT_EQ(duk_pcall(ctx, 0), DUK_EXEC_SUCCESS);
+  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);