Fix DOM Clobbering Vulnerability in Prototype Pollution Analyzer (#9)

Refactored the JavaScript shim and Go analyzer for the prototype pollution scanner to mitigate a DOM Clobbering vulnerability.

Key changes:
- The JavaScript callback function name is now randomized for each session.
- The shim immediately captures the callback function in a local closure, preventing global scope pollution.
- The Go analyzer was updated to generate and inject the randomized callback name.
- Tests were adapted to use flexible matchers to accommodate the dynamic function names.

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>

Author: google-labs-jules[bot]

internal/analysis/active/protopollution/analyze/proto_analyzer.go

@@ -18,10 +18,9 @@ import (
 )
 
 const (
-	ModuleName          = "PrototypePollutionAnalyzer"
-	jsCallbackName      = "__scalpel_protopollution_proof"
-	placeholderCanary   = "{{SCALPEL_CANARY}}"
-	placeholderCallback = "{{SCALPEL_CALLBACK}}"
+	ModuleName            = "PrototypePollutionAnalyzer"
+	placeholderCanary     = "{{SCALPEL_CANARY}}"
+	placeholderCallbackName = "{{SCALPEL_CALLBACK_NAME}}"
 )
 
 //go:embed shim.js
@@ -49,12 +48,13 @@ type PollutionProofEvent struct {
 // analysisContext holds all the state required for a single, concurrent analysis
 // of a target URL. It is created for the duration of one `Analyze` call and then discarded.
 type analysisContext struct {
-	ctx       context.Context
-	session   schemas.SessionContext
-	taskID    string
-	targetURL string
-	canary    string
-	logger    *zap.Logger
+	ctx          context.Context
+	session      schemas.SessionContext
+	taskID       string
+	targetURL    string
+	canary       string
+	callbackName string
+	logger       *zap.Logger
 }
 
 // NewAnalyzer creates a new, reusable instance of the prototype pollution analyzer.
@@ -92,12 +92,13 @@ func (a *Analyzer) Analyze(ctx context.Context, taskID, targetURL string) error
 
 	// 2. Initialize an isolated context for this specific task.
 	aCtx := &analysisContext{
-		ctx:       ctx,
-		session:   session,
-		taskID:    taskID,
-		targetURL: targetURL,
-		canary:    "sclp_" + uuid.New().String()[:8], // Generate a unique canary for this run.
-		logger:    a.logger.With(zap.String("taskID", taskID), zap.String("target", targetURL)),
+		ctx:          ctx,
+		session:      session,
+		taskID:       taskID,
+		targetURL:    targetURL,
+		canary:       "sclp_" + uuid.New().String()[:8], // Generate a unique canary for this run.
+		callbackName: "sclp_cb_" + uuid.New().String()[:12], // Generate a unique, randomized callback name.
+		logger:       a.logger.With(zap.String("taskID", taskID), zap.String("target", targetURL)),
 	}
 
 	// 3. Instrument the browser session with our shim and callbacks.
@@ -135,11 +136,11 @@ func (a *Analyzer) Analyze(ctx context.Context, taskID, targetURL string) error
 func (a *Analyzer) instrumentSession(ctx context.Context, session schemas.SessionContext, aCtx *analysisContext) error {
 	// The handler is now a method on analysisContext, which closes over the necessary state.
 	// This avoids complex parameter passing for the callback.
-	if err := session.ExposeFunction(ctx, jsCallbackName, aCtx.handlePollutionProof); err != nil {
+	if err := session.ExposeFunction(ctx, aCtx.callbackName, aCtx.handlePollutionProof); err != nil {
 		return fmt.Errorf("failed to expose proof function: %w", err)
 	}
 
-	shimScript := a.generateShim(aCtx.canary)
+	shimScript := a.generateShim(aCtx)
 
 	if err := session.InjectScriptPersistently(ctx, shimScript); err != nil {
 		return fmt.Errorf("failed to inject pp shim: %w", err)
@@ -192,10 +193,10 @@ func (aCtx *analysisContext) handlePollutionProof(event PollutionProofEvent) {
 }
 
 // generateShim prepares the JavaScript payload using fast string replacement.
-func (a *Analyzer) generateShim(canary string) string {
+func (a *Analyzer) generateShim(aCtx *analysisContext) string {
 	shim := protoPollutionShim
-	shim = strings.ReplaceAll(shim, placeholderCanary, canary)
-	shim = strings.ReplaceAll(shim, placeholderCallback, jsCallbackName)
+	shim = strings.ReplaceAll(shim, placeholderCanary, aCtx.canary)
+	shim = strings.ReplaceAll(shim, placeholderCallbackName, aCtx.callbackName)
 	return shim
 }
 

internal/analysis/active/protopollution/analyze/proto_analyzer_test.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"errors"
 	"regexp"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -58,14 +59,26 @@ func TestAnalyze_FindingFound(t *testing.T) {
 	// --- Mocks ---
 	mockBrowserManager.On("NewAnalysisContext", ctx, cfg, schemas.Persona{}, "", "", (chan<- schemas.Finding)(nil)).Return(mockSession, nil).Once()
 
-	mockSession.On("ExposeFunction", ctx, jsCallbackName, mock.AnythingOfType("func(proto.PollutionProofEvent)")).Return(nil).Once()
+	// The callback name is now randomized, so we need to capture it.
+	var capturedCallbackName string
+	mockSession.On("ExposeFunction", ctx, mock.MatchedBy(func(name string) bool {
+		return strings.HasPrefix(name, "sclp_cb_")
+	}), mock.AnythingOfType("func(proto.PollutionProofEvent)")).Return(nil).Once().Run(func(args mock.Arguments) {
+		capturedCallbackName = args.String(1)
+	})
 
 	mockSession.On("InjectScriptPersistently", ctx, mock.AnythingOfType("string")).Return(nil).Once().Run(func(args mock.Arguments) {
 		script := args.String(1)
-		re := regexp.MustCompile(`let pollutionCanary = '([^']+)';`)
-		matches := re.FindStringSubmatch(script)
-		require.Len(t, matches, 2, "Could not find canary in injected script.")
-		capturedCanary = matches[1]
+		canaryRe := regexp.MustCompile(`let pollutionCanary = '([^']+)';`)
+		canaryMatches := canaryRe.FindStringSubmatch(script)
+		require.Len(t, canaryMatches, 2, "Could not find canary in injected script.")
+		capturedCanary = canaryMatches[1]
+
+		callbackRe := regexp.MustCompile(`let detectionCallbackName = '([^']+)';`)
+		callbackMatches := callbackRe.FindStringSubmatch(script)
+		require.Len(t, callbackMatches, 2, "Could not find callback name in injected script.")
+		// We assert that the callback name in the script matches the one passed to ExposeFunction.
+		assert.Equal(t, capturedCallbackName, callbackMatches[1])
 	})
 
 	mockSession.On("AddFinding", ctx, mock.AnythingOfType("schemas.Finding")).Return(nil).Once().Run(func(args mock.Arguments) {
@@ -78,9 +91,10 @@ func TestAnalyze_FindingFound(t *testing.T) {
 		go func() {
 			time.Sleep(20 * time.Millisecond)
 			require.NotEmpty(t, capturedCanary, "Canary was not captured before navigation simulation")
+			require.NotEmpty(t, capturedCallbackName, "Callback name was not captured before navigation simulation")
 
 			// Use the mock's helper to get the function and call it.
-			fn, ok := mockSession.GetExposedFunction(jsCallbackName)
+			fn, ok := mockSession.GetExposedFunction(capturedCallbackName)
 			require.True(t, ok, "Callback function was not set via ExposeFunction")
 			callback, ok := fn.(func(PollutionProofEvent))
 			require.True(t, ok, "Exposed function has the wrong signature")
@@ -141,7 +155,7 @@ func TestAnalyze_NoFinding(t *testing.T) {
 	ctx := context.Background()
 
 	mockBrowserManager.On("NewAnalysisContext", ctx, cfg, schemas.Persona{}, "", "", (chan<- schemas.Finding)(nil)).Return(mockSession, nil)
-	mockSession.On("ExposeFunction", ctx, jsCallbackName, mock.AnythingOfType("func(proto.PollutionProofEvent)")).Return(nil).Once()
+	mockSession.On("ExposeFunction", ctx, mock.AnythingOfType("string"), mock.AnythingOfType("func(proto.PollutionProofEvent)")).Return(nil).Once()
 	mockSession.On("InjectScriptPersistently", ctx, mock.AnythingOfType("string")).Return(nil)
 	mockSession.On("Navigate", ctx, "http://clean.example.com").Return(nil)
 	mockSession.On("Close", mock.Anything).Return(nil)
@@ -183,7 +197,7 @@ func TestAnalyze_ContextCancellation(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 
 	mockBrowserManager.On("NewAnalysisContext", mock.Anything, cfg, schemas.Persona{}, "", "", (chan<- schemas.Finding)(nil)).Return(mockSession, nil)
-	mockSession.On("ExposeFunction", mock.Anything, jsCallbackName, mock.AnythingOfType("func(proto.PollutionProofEvent)")).Return(nil)
+	mockSession.On("ExposeFunction", mock.Anything, mock.AnythingOfType("string"), mock.AnythingOfType("func(proto.PollutionProofEvent)")).Return(nil)
 	mockSession.On("InjectScriptPersistently", mock.Anything, mock.AnythingOfType("string")).Return(nil)
 	mockSession.On("Navigate", mock.Anything, "http://slow.example.com").Return(nil).Run(func(args mock.Arguments) {
 		go func() {
@@ -216,7 +230,7 @@ func TestAnalyze_InstrumentationError(t *testing.T) {
 	expectedErr := errors.New("browser disconnected")
 
 	mockBrowserManager.On("NewAnalysisContext", ctx, cfg, schemas.Persona{}, "", "", (chan<- schemas.Finding)(nil)).Return(mockSession, nil)
-	mockSession.On("ExposeFunction", ctx, jsCallbackName, mock.Anything).Return(expectedErr)
+	mockSession.On("ExposeFunction", ctx, mock.AnythingOfType("string"), mock.Anything).Return(expectedErr)
 	mockSession.On("Close", ctx).Return(nil)
 
 	err := analyzer.Analyze(ctx, "task-instrument-fail", "http://example.com")
@@ -237,14 +251,18 @@ func TestAnalyze_MismatchedCanary(t *testing.T) {
 	analyzer := NewAnalyzer(logger, mockBrowserManager, cfg)
 	ctx := context.Background()
 
+	var capturedCallbackName string
 	mockBrowserManager.On("NewAnalysisContext", ctx, cfg, schemas.Persona{}, "", "", (chan<- schemas.Finding)(nil)).Return(mockSession, nil)
-	mockSession.On("ExposeFunction", ctx, jsCallbackName, mock.AnythingOfType("func(proto.PollutionProofEvent)")).Return(nil)
+	mockSession.On("ExposeFunction", ctx, mock.AnythingOfType("string"), mock.AnythingOfType("func(proto.PollutionProofEvent)")).Return(nil).Run(func(args mock.Arguments) {
+		capturedCallbackName = args.String(1)
+	})
 	mockSession.On("InjectScriptPersistently", ctx, mock.AnythingOfType("string")).Return(nil)
 	mockSession.On("Navigate", ctx, "http://example.com").Return(nil).Run(func(args mock.Arguments) {
 		go func() {
 			time.Sleep(10 * time.Millisecond)
+			require.NotEmpty(t, capturedCallbackName)
 			// Use the mock's helper to get the function and call it.
-			fn, ok := mockSession.GetExposedFunction(jsCallbackName)
+			fn, ok := mockSession.GetExposedFunction(capturedCallbackName)
 			require.True(t, ok, "Callback function was not set via ExposeFunction")
 			callback, ok := fn.(func(PollutionProofEvent))
 			require.True(t, ok, "Exposed function has the wrong signature")

internal/analysis/active/protopollution/analyze/shim.js

@@ -6,7 +6,12 @@
 
     // These placeholders are replaced by the Go analyzer before injection.
     let pollutionCanary = '{{SCALPEL_CANARY}}';
-    let detectionCallbackName = '__scalpel_protopollution_proof';
+    let detectionCallbackName = '{{SCALPEL_CALLBACK_NAME}}';
+
+    // Immediately capture the callback function into a local variable. This is the defense-in-depth
+    // measure against DOM Clobbering. Even if an element overwrites the global `window[detectionCallbackName]`,
+    // our shim will still hold a reference to the original function.
+    const callback = scope[detectionCallbackName];
 
     let domObserver = null;
 
@@ -38,12 +43,13 @@
      * @param {string} vector - A more detailed description or the actual malicious payload.
      */
     function notifyBackend(source, vector) {
-        if (typeof scope[detectionCallbackName] === 'function') {
+        // Use the locally captured callback function, not the global one.
+        if (typeof callback === 'function') {
             const stack = new Error().stack;
             // Use setTimeout to avoid blocking the main thread.
             setTimeout(() => {
                 try {
-                    scope[detectionCallbackName]({
+                    callback({
                         source: source,
                         canary: pollutionCanary,
                         vector: vector || "N/A",