fix: Disallow javascript: navigation in marvin by default (#6)

* chore: switch local dev to tsx

* refactor: standardize LOCAL_DEV flag

* fix: disallow javascript: urls with precheck

vendor/xssbot/context/marvin/api.ts

@@ -80,13 +80,13 @@ router.post("/visit", async (ctx) => {
     const validator = getVisitRequestValidator();
     if (!validator(req)) {
         ctx.status = 400;
-        ctx.body = process.env.NODE_ENV === "development" ? validator.errors : "invalid request";
+        ctx.body = config.IS_LOCAL_DEV ? validator.errors : "invalid request";
         return;
     }
     if (!config.PER_REQ_LIMITS && req.resourceLimits) {
         ctx.status = 400;
         ctx.body =
-            process.env.NODE_ENV === "development"
+            config.IS_LOCAL_DEV
                 ? "found timeout field when PER_REQ_LIMITS was not set"
                 : "invalid request";
         return;

vendor/xssbot/context/marvin/browser/visitor.ts

@@ -1,7 +1,7 @@
 import puppeteer from "puppeteer";
 import { VisitResourceLimits } from "../types";
 import * as config from "../config";
-import checkRequest from "../security/browser";
+import { checkBrowserRequest, precheckBrowserNavigation } from "../security/browser";
 import applyAuth from "./auth";
 import logger from "./logger";
 
@@ -19,7 +19,7 @@ async function safeClosePage(page: puppeteer.Page) {
 
 async function browserRequestValidator(request: puppeteer.HTTPRequest) {
     logger.info(`Intercepted ${request.method()} ${request.url()}`);
-    const [status, errMsg] = await checkRequest(request);
+    const [status, errMsg] = await checkBrowserRequest(request);
     if (!status) {
         logger.info(
             {
@@ -36,7 +36,7 @@ async function browserRequestValidator(request: puppeteer.HTTPRequest) {
 function pageConsoleLogger(msg: puppeteer.ConsoleMessage) {
     logger.info({
         type: msg.type(),
-        trace: msg.stackTrace().join("\n"),
+        trace: msg.stackTrace().map(trace => `${trace.url || "unknown"} (${trace.lineNumber}:${trace.columnNumber})`).join("\n"),
         console: msg.text()
     }, 'console');
 }
@@ -73,7 +73,7 @@ export default class BotVisitor {
             await page.setRequestInterception(true);
 
             page.on("request", browserRequestValidator);
-            if(process.env.NODE_ENVIRONMENT === "development") {
+            if(config.IS_LOCAL_DEV) {
                 page.on("console", pageConsoleLogger);
             }
             setTimeout(async () => safeClosePage(page), this.resourceLimits.timeouts.total);
@@ -83,6 +83,14 @@ export default class BotVisitor {
     }
 
     public async visit(url: string) {
+        logger.info({ url }, "received navigation intent");
+
+        const [shouldNavigate, message] = await precheckBrowserNavigation({ url });
+        if(!shouldNavigate) {
+            logger.info(`navigation intent precheck failed with: ${message}. refusing navigation`);
+            return;
+        }
+
         this.safeVisit(async (page) => {
             await applyAuth(page);
             await page.goto(url, {

vendor/xssbot/context/marvin/config.ts

@@ -5,6 +5,8 @@ export const PORT = +(process.env.PORT ?? 8000);
 export const BROWSER_EXECUTABLE = process.env.BROWSER_EXECUTABLE ?? "/usr/bin/google-chrome-stable";
 /** The redis host for managing tasks, defaults to localhost */
 export const REDIS_HOST = process.env.REDIS_HOST ?? "localhost";
+/** Flag for enabling local dev only behavior */
+export const IS_LOCAL_DEV = process.env.NODE_ENVIRONMENT === "development";
 
 // Ops
 /** Healthcheck url, defaults to /healthz. Disable if set to 'DISABLE' */

vendor/xssbot/context/marvin/security/browser.ts

@@ -9,24 +9,24 @@ export class RequestSecError extends Error {}
 type RequestChecker = (req: puppeteer.HTTPRequest) => Promise<void>;
 
 // SEC: Confusion vuln here if node's URL != chrome's URL
-async function validateURL(req: puppeteer.HTTPRequest) {
+async function validateURL(urlString: string) {
     if (config.ALLOW_ALL_PROTOCOLS) {
         return;
     }
-    const url = new URL(req.url());
+    const url = new URL(urlString);
 
     if (!["http:", "https:"].includes(url.protocol)) {
         throw new RequestSecError("Invalid protocol");
     }
 }
 
 // SEC: Theres a bypass for a rebind here but :shrug:
-async function noInternalAccess(req: puppeteer.HTTPRequest) {
+async function noInternalAccess(urlString: string) {
     if (config.ALLOW_INTERNAL_ADDRESSES) {
         return;
     }
 
-    const url = new URL(req.url());
+    const url = new URL(urlString);
     const ipAddr = await dns.promises.lookup(url.hostname);
     if (!ipAddr || !ipaddress.isValid(ipAddr.address)) {
         throw new RequestSecError("Invalid IP");
@@ -38,8 +38,15 @@ async function noInternalAccess(req: puppeteer.HTTPRequest) {
     }
 }
 
-export default async function checkRequest(request: puppeteer.HTTPRequest): Promise<[boolean, string?]> {
-    const checks: RequestChecker[] = [validateURL, noInternalAccess];
+/**
+ * Validates a browser based request. This is intended to be called in a puppeteer navigation context
+ * for example within a "request" event handler.
+ */
+export async function checkBrowserRequest(request: puppeteer.HTTPRequest): Promise<[boolean, string?]> {
+    const checks: RequestChecker[] = [
+        r => validateURL(r.url()),
+        r => noInternalAccess(r.url())
+    ];
     try {
         await Promise.all(checks.map(async (fn) => fn(request)));
         return [true];
@@ -48,3 +55,18 @@ export default async function checkRequest(request: puppeteer.HTTPRequest): Prom
         return [false, (<RequestSecError>e).message];
     }
 }
+
+/**
+ * Validatese a intended browser navigation before a page visit is issued. This should be used to
+ * guard against chases where the actual visitation is potentially dangerous, or the intended request
+ * cannot be safely checked by checkBrowserRequest. An example of this is deny access to javascript: style
+ * urls, which do not trigger request event handlers.
+ */
+export async function precheckBrowserNavigation(navigationIntent: { url: string }): Promise<[boolean, string?]> {
+    try {
+        await validateURL(navigationIntent.url);
+    } catch(e) {
+        return [false, (<RequestSecError>e).message];
+    }
+    return [true];
+}

vendor/xssbot/context/package.json

@@ -2,13 +2,13 @@
     "name": "xssbot",
     "version": "1.0.0",
     "description": "XssBot for DUCTF",
-    "main": "src/index.ts",
+    "main": "main.ts",
     "author": "todo",
     "license": "MIT",
     "scripts": {
-        "lint:check": "prettier -c ./xssbot && eslint -c ./xssbot/*",
-        "lint": "prettier --write ./xssbot && eslint --fix ./xssbot/*",
-        "dev": "nodemon xssbot/main.ts"
+        "lint:check": "prettier -c ./marvin && eslint -c ./marvin/*",
+        "lint": "prettier --write ./marvin && eslint --fix ./marvin/*",
+        "dev": "NODE_ENVIRONMENT=development tsx watch marvin/main.ts"
     },
     "devDependencies": {
         "@types/ajv": "^1.0.0",
@@ -26,8 +26,8 @@
         "eslint-config-airbnb-typescript": "^14.0.0",
         "eslint-config-prettier": "^8.3.0",
         "eslint-plugin-import": "^2.24.2",
-        "nodemon": "^2.0.12",
         "prettier": "^2.3.2",
+        "tsx": "^4.16.2",
         "typescript": "^4.4.2"
     },
     "dependencies": {