Fix the reported security bug. (#159)

See GHSA-62q6-v997-f7v9 for more details on the security bug. Fix: - Use the JS_FunctionCall() API to call the FindProxyForURL() function instead of JS_EvaluateScript() API. - This also does away with the need to str_replace function. Making code simpler. - I've verified the fix with the test cases provided in the security bug.
manugarg · May 25, 2023 · 0bf0636 · 0bf0636
1 parent e7c99e3
commit 0bf0636
Show file tree

Hide file tree

Showing 5 changed files with 26 additions and 150 deletions.
diff --git a/src/Makefile b/src/Makefile
@@ -87,11 +87,7 @@ jsapi_buildstamp: spidermonkey/js/src
 spidermonkey/libjs.a: spidermonkey/js/src
 	cd spidermonkey && SMCFLAGS="$(SHFLAGS) $(SMCFLAGS)" $(MAKE) jslib
 
-pac_utils_test: pac_utils_test.c pac_utils.h
-	$(CC) $(MAINT_CFLAGS) $(CFLAGS) $(SHFLAGS) pac_utils_test.c -o pac_utils_test -lm -L. -I.
-
-pacparser.o: pacparser.c pac_utils.h pacparser.h pac_utils_test jsapi_buildstamp
-	./pac_utils_test
+pacparser.o: pacparser.c pac_utils.h pacparser.h jsapi_buildstamp
 	$(CC) $(MAINT_CFLAGS) $(CFLAGS) $(SHFLAGS) -c pacparser.c -o pacparser.o
 	touch pymod/pacparser_o_buildstamp
 

diff --git a/src/pac_utils.h b/src/pac_utils.h
@@ -26,9 +26,6 @@
 // Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
 // USA
 
-#include <string.h>
-#include <stdlib.h>
-
 static const char *pacUtils =
 "function dnsDomainIs(host, domain) {\n"
 "    return (host.length >= domain.length &&\n"
@@ -335,57 +332,3 @@ static const char *pacUtils =
 "        return FindProxyForURL(url, host);\n"
 "    }\n"
 "}\n";
-
-
-// You must free the result if result is non-NULL.
-char *str_replace(const char *orig, const char *rep, const char *with) {
-    if (orig == NULL || rep == NULL || with == NULL) {
-        return NULL;
-    }
-
-    size_t len_orig  = strnlen(orig, 1024);
-    size_t len_rep  = strnlen(rep, 1024);
-    size_t len_with = strnlen(with, 1024);
-
-    if (len_orig == 0 || len_rep == 0) {
-        char *result = malloc(len_orig + 1);
-        strcpy(result, orig);
-        return result;
-    }
-
-    // Count replacements needed
-    int count;           // number of replacements
-    char const *start = orig;
-    // Cursor moves through the string, looking for rep.
-    char const *cursor;
-    for (count = 0;; ++count) {
-        cursor = strstr(start, rep);
-        if (cursor == NULL) {
-            break;
-        }
-        start = cursor + len_rep;
-    }
-
-    char *tmp;
-    char *result;
-    tmp = result = malloc(len_orig + (len_with - len_rep) * count + 1);
-
-    // first time through the loop, all the variable are set correctly
-    // from here on,
-    //    tmp points to the end of the result string
-    //    ins points to the next occurrence of rep in orig
-    //    orig points to the remainder of orig after "end of rep"
-    while (count--) {
-        const char *ins = strstr(orig, rep);
-        int len_front = (int)(ins - orig); // How far have we moved
-        // Into the tmp, copy everything until we reach the rep.
-        // and move tmp forward.
-        tmp = strncpy(tmp, orig, len_front) + len_front;
-        tmp = strcpy(tmp, with) + len_with;
-        orig += len_front + len_rep; // move to next "end of rep"
-    }
-
-    // Copy the remaining string.
-    strcpy(tmp, orig);
-    return result;
-}
diff --git a/src/pac_utils_test.c b/src/pac_utils_test.c
diff --git a/src/pacparser.c b/src/pacparser.c
@@ -238,7 +238,7 @@ my_ip(JSContext *cx, JSObject *UNUSED(o), uintN UNUSED(u), jsval *argv, jsval *r
 // myIpAddressEx in JS context; not available in core JavaScript.
 // returns 127.0.0.1 if not able to determine local ip.
 static JSBool                                  // JS_TRUE or JS_FALSE
-my_ip_ex(JSContext *cx, JSObject *UNUSED(o), uintN UNUSED(u), jsval *argv, jsval *rval)
+my_ip_ex(JSContext *cx, JSObject *UNUSED(o), uintN UNUSED(u), jsval *UNUSED(argv), jsval *rval)
 {
   char ipaddr[INET6_ADDRSTRLEN * MAX_IP_RESULTS + MAX_IP_RESULTS];
   char* out;
@@ -451,32 +451,14 @@ pacparser_find_proxy(const char *url, const char *host)
     return NULL;
   }
 
-  // URL-encode "'" as we use single quotes to stick the URL into a temporary script.
-  char *sanitized_url = str_replace(url, "'", "%27");
-  // Hostname shouldn't have single quotes in them
-  if (strchr(host, '\'')) {
-    print_error("%s %s\n", error_prefix,
-      "Invalid hostname: hostname can't have single quotes.");
-    free(sanitized_url);
-    return NULL;
-  }
-
-  script = (char*) malloc(32 + strlen(sanitized_url) + strlen(host));
-  script[0] = '\0';
-  strcat(script, "findProxyForURL('");
-  strcat(script, sanitized_url);
-  strcat(script, "', '");
-  strcat(script, host);
-  strcat(script, "')");
-  if (_debug()) print_error("DEBUG: Executing JavaScript: %s\n", script);
-  if (!JS_EvaluateScript(cx, global, script, strlen(script), NULL, 1, &rval)) {
+  jsval args[2];
+  args[0] = STRING_TO_JSVAL(JS_NewStringCopyZ(cx, url));
+  args[1] = STRING_TO_JSVAL(JS_NewStringCopyZ(cx, host));
+
+  if (!JS_CallFunctionName(cx, global, "findProxyForURL", 2, args, &rval)) {
     print_error("%s %s\n", error_prefix, "Problem in executing findProxyForURL.");
-    free(sanitized_url);
-    free(script);
     return NULL;
   }
-  free(sanitized_url);
-  free(script);
   return JS_GetStringBytes(JS_ValueToString(cx, rval));
 }
 

diff --git a/tests/proxy.pac b/tests/proxy.pac
@@ -3,41 +3,36 @@
 // Go via proxy for all other hosts.
 
 function FindProxyForURL(url, host) {
-
-  if ((isPlainHostName(host) ||
-      dnsDomainIs(host, ".manugarg.com")) &&
-      !localHostOrDomainIs(host, "www.manugarg.com"))
-    return "plainhost/.manugarg.com";
+  if (
+    (isPlainHostName(host) || dnsDomainIs(host, '.manugarg.com')) &&
+    !localHostOrDomainIs(host, 'www.manugarg.com')
+  )
+    return 'plainhost/.manugarg.com';
 
   // Test single quote handling in URL.
-  if (/.*%27.*/.test(url)) {
-    return "URLHasQuotes";
+  if (/'/.test(url)) {
+    return 'URLHasQuotes';
   }
 
   // Return externaldomain if host matches .*\.externaldomain\.com
-  if (/.*\.externaldomain\.com/.test(host))
-    return "externaldomain";
+  if (/.*\.externaldomain\.com/.test(host)) return 'externaldomain';
 
   // Test if DNS resolving is working as intended
-  if (dnsDomainIs(host, ".google.com") &&
-      isResolvable(host))
-    return "isResolvable";
+  if (dnsDomainIs(host, '.google.com') && isResolvable(host))
+    return 'isResolvable';
 
   // Test if DNS resolving is working as intended
-  if (dnsDomainIs(host, ".notresolvabledomainXXX.com") &&
-      !isResolvable(host))
-    return "isNotResolvable";
+  if (dnsDomainIs(host, '.notresolvabledomainXXX.com') && !isResolvable(host))
+    return 'isNotResolvable';
 
-  if (/^https:\/\/.*$/.test(url))
-    return "secureUrl";
+  if (/^https:\/\/.*$/.test(url)) return 'secureUrl';
 
-  if (isInNet(myIpAddress(), '10.10.0.0', '255.255.0.0'))
-    return '10.10.0.0';
+  if (isInNet(myIpAddress(), '10.10.0.0', '255.255.0.0')) return '10.10.0.0';
 
-  if ((typeof(myIpAddressEx) == "function") &&
-      isInNetEx(myIpAddressEx(), '3ffe:8311:ffff/48'))
+  if (
+    typeof myIpAddressEx == 'function' &&
+    isInNetEx(myIpAddressEx(), '3ffe:8311:ffff/48')
+  )
     return '3ffe:8311:ffff';
-
-  else
-    return "END-OF-SCRIPT";
+  else return 'END-OF-SCRIPT';
 }