Merge pull request #307 from 0xreacher/patch-1

Fixed Chrome Tab Leak, Wrong Error Fields & Task Mutation

Author: edoardottt

pkg/scan/chrome.go

@@ -1,8 +1,5 @@
-/*
-pphack - The Most Advanced Client-Side Prototype Pollution Scanner
-
-This repository is under MIT License https://github.com/edoardottt/pphack/blob/main/LICENSE
-*/
+// pphack - The Most Advanced Client-Side Prototype Pollution Scanner
+// This repository is under MIT License https://github.com/edoardottt/pphack/blob/main/LICENSE
 
 package scan
 
@@ -20,7 +17,9 @@ import (
 )
 
 // GetChromeOptions takes as input the runner settings and returns
-// the chrome options.
+// the chrome options used to configure the headless browser instance.
+// It always disables certificate errors and sets a custom user agent.
+// If a proxy is configured in the runner options, it is appended as well.
 func GetChromeOptions(r *Runner) []func(*chromedp.ExecAllocator) {
 	copts := append(chromedp.DefaultExecAllocatorOptions[:],
 		chromedp.Flag("ignore-certificate-errors", true),
@@ -37,97 +36,153 @@ func GetChromeOptions(r *Runner) []func(*chromedp.ExecAllocator) {
 // GetChromeBrowser takes as input the chrome options and returns
 // the contexts with the associated cancel functions to use the
 // headless chrome browser it creates.
-func GetChromeBrowser(copts []func(*chromedp.ExecAllocator)) (context.CancelFunc,
-	context.Context, context.CancelFunc) {
+// Returns ecancel (exec allocator cancel), pctx (parent browser context),
+// and pcancel (parent context cancel).
+// Callers must invoke pcancel before ecancel to ensure correct cleanup order.
+// ecancel is also called internally on fatal browser startup failure
+// to avoid leaking the exec allocator before the process exits.
+func GetChromeBrowser(copts []func(*chromedp.ExecAllocator)) (context.CancelFunc, context.Context, context.CancelFunc) {
 	ectx, ecancel := chromedp.NewExecAllocator(context.Background(), copts...)
 	pctx, pcancel := chromedp.NewContext(ectx)
 
+	// Run an empty chromedp task to verify the browser starts successfully.
+	// If it fails, ecancel is called before Fatal to avoid leaking the allocator.
 	if err := chromedp.Run(pctx); err != nil {
+		ecancel()
 		gologger.Fatal().Msgf("error starting browser: %s", err.Error())
 	}
 
 	return ecancel, pctx, pcancel
 }
 
-// Scan is the actual function that takes as input a browser context, other info
-// and performs the scan.
-func Scan(pctx context.Context, r *Runner, headers map[string]interface{},
-	js, value, targetURL string) (output.ResultData, error) {
+// buildHeaders is a helper that converts a headers map into a chromedp.Tasks
+// slice containing the SetExtraHTTPHeaders action.
+// Returns nil if headers is nil, making it safe to append directly onto any
+// existing chromedp.Tasks without an extra nil check at the call site.
+func buildHeaders(headers map[string]interface{}) chromedp.Tasks {
+	if headers == nil {
+		return nil
+	}
+
+	return chromedp.Tasks{network.SetExtraHTTPHeaders(network.Headers(headers))}
+}
+
+// Scan is the core function that performs the prototype pollution scan.
+// It takes a parent browser context (pctx), runner config (r), optional HTTP
+// headers, the JavaScript payload (js), the original input value, and the
+// fully constructed target URL.
+//
+// Flow:
+//  1. Creates a timeout-scoped context and a dedicated Chrome tab context.
+//  2. Navigates to targetURL and evaluates the JS pollution payload.
+//  3. If exploit mode is enabled and the payload returned a non-empty result,
+//     it runs fingerprinting to identify the affected library/sink.
+//  4. Attempts exploitation using the fingerprint results.
+//  5. Populates and returns a ResultData struct with all findings and errors.
+func Scan(
+	pctx context.Context,
+	r *Runner,
+	headers map[string]interface{},
+	js, value, targetURL string,
+) (output.ResultData, error) {
 	var (
-		resScan                string
-		resDetection           []string
-		chromedpTasksScan      chromedp.Tasks
-		chromedpTasksDetection chromedp.Tasks
+		resScan      string
+		resDetection []string
 	)
 
+	// Initialize result with the original input value and the constructed scan URL.
 	resultData := output.ResultData{
 		TargetURL: value,
 		ScanURL:   targetURL,
 	}
 
-	if headers != nil {
-		chromedpTasksScan = append(chromedpTasksScan, network.SetExtraHTTPHeaders(network.Headers(headers)))
-	}
-
-	chromedpTasksScan = append(
-		chromedpTasksScan, chromedp.Navigate(targetURL),
+	// Wrap the parent context with a per-scan timeout so hung pages
+	// don't block the scanner indefinitely.
+	ctx, ctxCancel := context.WithTimeout(pctx, time.Second*time.Duration(r.Options.Timeout))
+	defer ctxCancel()
+
+	// Open a new Chrome tab scoped to the timeout context.
+	// tabCancel explicitly closes the tab when Scan returns,
+	// preventing tab accumulation across concurrent scans.
+	// Previously this cancel was silently dropped with _, causing a tab leak.
+	tabCtx, tabCancel := chromedp.NewContext(ctx)
+	defer tabCancel()
+
+	// Build the scan task list: optionally inject custom HTTP headers,
+	// navigate to the target, then evaluate the prototype pollution JS payload.
+	scanTasks := buildHeaders(headers)
+	scanTasks = append(
+		scanTasks,
+		chromedp.Navigate(targetURL),
 		chromedp.EvaluateAsDevTools(js, &resScan),
 	)
 
-	ctx, cancel := context.WithTimeout(pctx, time.Second*time.Duration(r.Options.Timeout))
-	ctx, _ = chromedp.NewContext(ctx)
-
-	defer cancel()
-
-	errScan := chromedp.Run(ctx, chromedpTasksScan)
+	// Execute the scan tasks inside the dedicated tab context.
+	errScan := chromedp.Run(tabCtx, scanTasks)
 	if errScan != nil {
 		resultData.ScanError = errScan.Error()
 	}
 
+	// Trim and store the JS evaluation result.
+	// This value is reused in the exploit gate below to avoid a redundant TrimSpace call.
 	resultData.JSEvaluation = strings.TrimSpace(resScan)
 
-	// if I have to detect the exploit, no errors and it's vulnerable.
-	if r.Options.Exploit && errScan == nil {
-		if resTrimmed := strings.TrimSpace(resScan); resTrimmed != "" {
-			if r.Options.Verbose {
-				gologger.Info().Label("VULN").Msg(fmt.Sprintf("Target is Vulnerable %s", targetURL))
-			}
-
-			chromedpTasksScan = append(chromedpTasksScan, chromedp.EvaluateAsDevTools(exploit.Fingerprint, &resDetection))
-
-			errDetection := chromedp.Run(ctx, chromedpTasksScan)
-			if errDetection != nil && r.Options.Verbose {
-				gologger.Error().Msg(errDetection.Error())
-			}
+	// Early return guard: skip exploit phase entirely if:
+	// - exploit mode is off, OR
+	// - the scan itself errored (page unreachable, timeout, etc.), OR
+	// - the JS payload returned empty (no pollution detected).
+	if !r.Options.Exploit || errScan != nil || resultData.JSEvaluation == "" {
+		return resultData, nil
+	}
 
-			resultData.Fingerprint = resDetection
-			resultData.References = exploit.GetReferences(resDetection)
+	if r.Options.Verbose {
+		gologger.Info().Label("VULN").Msg(fmt.Sprintf("Target is Vulnerable %s", targetURL))
+	}
 
-			if errDetection != nil {
-				resultData.FingerprintError = errDetection.Error()
-			}
+	// Run fingerprinting as a separate, isolated task list.
+	// Previously the fingerprint eval was appended onto scanTasks, which caused
+	// the full task list (Navigate + JS eval + fingerprint) to re-run from scratch,
+	// re-navigating the page unnecessarily and potentially corrupting scan state.
+	fingerprintTasks := chromedp.Tasks{
+		chromedp.EvaluateAsDevTools(exploit.Fingerprint, &resDetection),
+	}
 
-			if headers != nil {
-				chromedpTasksDetection = append(chromedpTasksDetection, network.SetExtraHTTPHeaders(network.Headers(headers)))
-			}
+	errDetection := chromedp.Run(tabCtx, fingerprintTasks)
+	if errDetection != nil {
+		// Log detection errors unconditionally - errors are not verbosity-dependent.
+		gologger.Error().Msg(errDetection.Error())
+		resultData.FingerprintError = errDetection.Error()
+	}
 
-			if r.Options.Verbose {
-				gologger.Info().Msg(fmt.Sprintf("Trying to exploit %s", value))
-			}
+	// Store fingerprint results and cross-reference known exploit references.
+	resultData.Fingerprint = resDetection
+	resultData.References = exploit.GetReferences(resDetection)
 
-			result, errExploit := exploit.CheckExploit(pctx, chromedpTasksDetection, resDetection, targetURL,
-				r.Options.Verbose, r.Options.Timeout)
+	if r.Options.Verbose {
+		gologger.Info().Msg(fmt.Sprintf("Trying to exploit %s", value))
+	}
 
-			resultData.ExploitURLs = result
+	// Build exploit-phase headers separately using buildHeaders.
+	// Previously this was a duplicated inline block; now it uses the shared helper
+	// for consistency with the scan phase header handling.
+	exploitTasks := buildHeaders(headers)
+
+	result, errExploit := exploit.CheckExploit(
+		pctx,
+		exploitTasks,
+		resDetection,
+		targetURL,
+		r.Options.Verbose,
+		r.Options.Timeout,
+	)
 
-			if errExploit != nil {
-				resultData.ExploitError = errDetection.Error()
-			}
+	resultData.ExploitURLs = result
 
-			if errExploit != nil && !r.Options.Verbose {
-				gologger.Error().Msg(errExploit.Error())
-			}
-		}
+	if errExploit != nil {
+		// Previously this field was incorrectly set to errDetection.Error(),
+		// masking the actual exploit error. Now correctly uses errExploit.
+		resultData.ExploitError = errExploit.Error()
+		gologger.Error().Msg(errExploit.Error())
 	}
 
 	return resultData, nil