Fix: Remediate DOM Clobbering vulnerability in IAST shim (#8)

This commit fixes a critical DOM Clobbering vulnerability that allowed a target application to disable the IAST analyzer by overwriting its global callback functions.

The following changes have been implemented:

1.  **Randomized Callback Names:** The Go backend now generates unique, session-specific names for the `sink_event`, `execution_proof`, and `shim_error` callbacks by appending a UUID. This prevents attackers from predicting the function names.

2.  **Dynamic Probe Generation:** XSS probes are now generated dynamically to use the session-specific `execution_proof` callback name, ensuring payloads trigger the correct, non-clobberable function.

3.  **Defense-in-Depth JS Closure:** The JavaScript shim has been hardened to immediately capture the exposed callback functions into a local closure upon initialization. All internal reporting now uses these captured references, making the shim immune to DOM Clobbering that occurs after it has loaded.

4.  **Updated Tests:** All relevant unit tests have been updated to accommodate the dynamic nature of the callback names, ensuring the test suite passes and validates the new behavior.

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

Author: google-labs-jules[bot]

internal/analysis/active/taint/probes.go

@@ -65,8 +65,9 @@ var ValidTaintFlows = map[TaintFlowPath]bool{
 // Pollution, and OAST-based checks. This list serves as the default configuration
 // for the taint analyzer.
 func DefaultProbes() []ProbeDefinition {
-	// Define the execution proof function call.
-	executionProofCall := `window.` + JSCallbackExecutionProof + `('{{.Canary}}')`
+	// VULN-FIX: Use a template placeholder for the callback name in the execution proof.
+	// This will be replaced by the session-specific randomized name in the analyzer.
+	executionProofCall := `window.{{.ProofCallbackName}}('{{.Canary}}')`
 
 	// OAST Integration: Define the OAST callback formats.
 	// {{.OASTServer}} is replaced by the Analyzer if an OAST provider is configured.

internal/analysis/active/taint/taint_analyzer.go

@@ -57,6 +57,11 @@ type Analyzer struct {
 	producersWG      sync.WaitGroup
 	backgroundCtx    context.Context
 	backgroundCancel context.CancelFunc
+
+	// VULN-FIX: Store session-specific, randomized callback names to prevent DOM Clobbering.
+	jsCallbackSinkEventName      string
+	jsCallbackExecutionProofName string
+	jsCallbackShimErrorName      string
 }
 
 // NewAnalyzer creates and initializes a new taint Analyzer instance. It reads
@@ -89,6 +94,13 @@ func NewAnalyzer(config Config, reporter ResultsReporter, oastProvider OASTProvi
 		taskLogger.Info("No OAST provider configured; out-of-band tests will be skipped.")
 	}
 
+	// VULN-FIX: Generate unique callback names for this analysis session to prevent DOM Clobbering.
+	// This makes it impossible for a target page to predict and overwrite the callback functions.
+	shortID := uuid.New().String()[:8]
+	sinkCallbackName := fmt.Sprintf("%s_%s", JSCallbackSinkEvent, shortID)
+	proofCallbackName := fmt.Sprintf("%s_%s", JSCallbackExecutionProof, shortID)
+	errorCallbackName := fmt.Sprintf("%s_%s", JSCallbackShimError, shortID)
+
 	return &Analyzer{
 		config:          config,
 		reporter:        reporter,
@@ -99,6 +111,11 @@ func NewAnalyzer(config Config, reporter ResultsReporter, oastProvider OASTProvi
 		eventsChan:      make(chan Event, config.EventChannelBuffer),
 		shimTemplate:    templateContent, // <-- Store the raw string
 		validTaintFlows: localValidTaintFlows,
+
+		// VULN-FIX: Store the generated unique names.
+		jsCallbackSinkEventName:      sinkCallbackName,
+		jsCallbackExecutionProofName: proofCallbackName,
+		jsCallbackShimErrorName:      errorCallbackName,
 	}, nil
 }
 
@@ -115,7 +132,7 @@ func (a *Analyzer) UpdateTaintFlowRuleForTesting(flow TaintFlowPath, isValid boo
 // JavaScript instrumentation shim from a template string and a JSON configuration
 // for sinks. This allows the session manager to prepare the shim before the
 // analyzer is fully instantiated.
-func BuildTaintShim(templateContent string, configJSON string) (string, error) {
+func BuildTaintShim(templateContent string, configJSON string, sinkCallback, proofCallback, errorCallback string) (string, error) {
 	// 1. Parse the template content passed as an argument.
 	tmpl, err := template.New("shim").Parse(templateContent)
 	if err != nil {
@@ -130,9 +147,9 @@ func BuildTaintShim(templateContent string, configJSON string) (string, error) {
 		ErrorCallbackName string
 	}{
 		SinksJSON:         configJSON,
-		SinkCallbackName:  JSCallbackSinkEvent,
-		ProofCallbackName: JSCallbackExecutionProof,
-		ErrorCallbackName: JSCallbackShimError,
+		SinkCallbackName:  sinkCallback,
+		ProofCallbackName: proofCallback,
+		ErrorCallbackName: errorCallback,
 	}
 
 	// 3. Execute the template into a buffer.
@@ -268,13 +285,14 @@ func (a *Analyzer) startBackgroundWorkers() {
 // instrument hooks into the client side by exposing Go functions to JavaScript
 // and injecting the instrumentation shim.
 func (a *Analyzer) instrument(ctx context.Context, session SessionContext) error {
-	if err := session.ExposeFunction(ctx, JSCallbackSinkEvent, a.handleSinkEvent); err != nil {
+	// VULN-FIX: Use the randomized, session-specific names when exposing functions.
+	if err := session.ExposeFunction(ctx, a.jsCallbackSinkEventName, a.handleSinkEvent); err != nil {
 		return fmt.Errorf("failed to expose sink event callback: %w", err)
 	}
-	if err := session.ExposeFunction(ctx, JSCallbackExecutionProof, a.handleExecutionProof); err != nil {
+	if err := session.ExposeFunction(ctx, a.jsCallbackExecutionProofName, a.handleExecutionProof); err != nil {
 		return fmt.Errorf("failed to expose execution proof callback: %w", err)
 	}
-	if err := session.ExposeFunction(ctx, JSCallbackShimError, a.handleShimError); err != nil {
+	if err := session.ExposeFunction(ctx, a.jsCallbackShimErrorName, a.handleShimError); err != nil {
 		return fmt.Errorf("failed to expose shim error callback: %w", err)
 	}
 
@@ -300,7 +318,8 @@ func (a *Analyzer) generateShim() (string, error) {
 	}
 
 	// 2. Call the exported BuildTaintShim function with the pre-loaded template string.
-	return BuildTaintShim(a.shimTemplate, string(sinksJSON))
+	// VULN-FIX: Pass the session-specific randomized callback names to be injected into the shim template.
+	return BuildTaintShim(a.shimTemplate, string(sinksJSON), a.jsCallbackSinkEventName, a.jsCallbackExecutionProofName, a.jsCallbackShimErrorName)
 }
 
 // enqueueEvent provides a safe, non blocking mechanism for sending an event to the correlation engine.
@@ -448,7 +467,13 @@ func (a *Analyzer) preparePayload(probeDef ProbeDefinition, canary string) strin
 		return ""
 	}
 
-	replacements := []string{"{{.Canary}}", canary}
+	// VULN-FIX: Add the dynamic, session-specific proof callback name to the replacements.
+	// This ensures that XSS probes are generated with the correct, non-clobberable callback.
+	replacements := []string{
+		"{{.Canary}}", canary,
+		"{{.ProofCallbackName}}", a.jsCallbackExecutionProofName,
+	}
+
 	if requiresOAST {
 		// We can now safely access oastProvider as oastConfigured is true.
 		oastURL := a.oastProvider.GetServerURL()

internal/analysis/active/taint/taint_analyzer_test.go

@@ -123,9 +123,11 @@ func TestGenerateShim(t *testing.T) {
 	require.NoError(t, err)
 	require.NotEmpty(t, shim)
 
-	assert.Contains(t, shim, fmt.Sprintf(`SinkCallbackName: "%s"`, JSCallbackSinkEvent))
-	assert.Contains(t, shim, fmt.Sprintf(`ProofCallbackName: "%s"`, JSCallbackExecutionProof))
-	assert.Contains(t, shim, fmt.Sprintf(`ErrorCallbackName: "%s"`, JSCallbackShimError))
+	// VULN-FIX: Assert that the shim contains the session-specific randomized names, not the static constants.
+	assert.Contains(t, shim, fmt.Sprintf(`SinkCallbackName: "%s"`, analyzer.jsCallbackSinkEventName))
+	assert.Contains(t, shim, fmt.Sprintf(`ProofCallbackName: "%s"`, analyzer.jsCallbackExecutionProofName))
+	assert.Contains(t, shim, fmt.Sprintf(`ErrorCallbackName: "%s"`, analyzer.jsCallbackShimErrorName))
+	assert.NotContains(t, shim, fmt.Sprintf(`SinkCallbackName: "%s"`, JSCallbackSinkEvent))
 
 	expectedSinksJSON := `[{"Name":"eval","Type":"EVAL","Setter":false,"ArgIndex":0},{"Name":"Element.prototype.innerHTML","Type":"INNER_HTML","Setter":true,"ArgIndex":0,"ConditionID":"COND_TEST"}]`
 	assert.Contains(t, shim, "Sinks: "+expectedSinksJSON+",")
@@ -136,9 +138,17 @@ func TestInstrument_Success(t *testing.T) {
 	mockSession := mocks.NewMockSessionContext()
 	ctx := context.Background()
 
-	mockSession.On("ExposeFunction", ctx, JSCallbackSinkEvent, mock.AnythingOfType("func(taint.SinkEvent)")).Return(nil).Once()
-	mockSession.On("ExposeFunction", ctx, JSCallbackExecutionProof, mock.AnythingOfType("func(taint.ExecutionProofEvent)")).Return(nil).Once()
-	mockSession.On("ExposeFunction", ctx, JSCallbackShimError, mock.AnythingOfType("func(taint.ShimErrorEvent)")).Return(nil).Once()
+	// VULN-FIX: The callback names are now randomized. The mock should expect any string that
+	// matches the base prefix, rather than a hardcoded static value. This makes the test robust.
+	mockSession.On("ExposeFunction", ctx, mock.MatchedBy(func(name string) bool {
+		return strings.HasPrefix(name, JSCallbackSinkEvent)
+	}), mock.AnythingOfType("func(taint.SinkEvent)")).Return(nil).Once()
+	mockSession.On("ExposeFunction", ctx, mock.MatchedBy(func(name string) bool {
+		return strings.HasPrefix(name, JSCallbackExecutionProof)
+	}), mock.AnythingOfType("func(taint.ExecutionProofEvent)")).Return(nil).Once()
+	mockSession.On("ExposeFunction", ctx, mock.MatchedBy(func(name string) bool {
+		return strings.HasPrefix(name, JSCallbackShimError)
+	}), mock.AnythingOfType("func(taint.ShimErrorEvent)")).Return(nil).Once()
 	mockSession.On("InjectScriptPersistently", ctx, mock.AnythingOfType("string")).Return(nil).Once()
 
 	err := analyzer.instrument(ctx, mockSession)
@@ -153,7 +163,10 @@ func TestInstrument_Failure_ExposeFunction(t *testing.T) {
 	ctx := context.Background()
 
 	expectedError := errors.New("browser connection lost")
-	mockSession.On("ExposeFunction", ctx, JSCallbackSinkEvent, mock.Anything).Return(expectedError).Once()
+	// VULN-FIX: Match against the randomized prefix, not the static constant.
+	mockSession.On("ExposeFunction", ctx, mock.MatchedBy(func(name string) bool {
+		return strings.HasPrefix(name, JSCallbackSinkEvent)
+	}), mock.Anything).Return(expectedError).Once()
 
 	err := analyzer.instrument(ctx, mockSession)
 
@@ -734,7 +747,8 @@ func TestAnalyze_HappyPath(t *testing.T) {
 		}
 		analyzer.probesMutex.RUnlock()
 		require.NotEmpty(t, activeCanary)
-		simulateCallback(t, mockSession, JSCallbackSinkEvent, SinkEvent{Type: schemas.SinkFetchURL, Value: "http://oast.example.com/" + activeCanary})
+		// VULN-FIX: Use the dynamic callback name from the analyzer instance.
+		simulateCallback(t, mockSession, analyzer.jsCallbackSinkEventName, SinkEvent{Type: schemas.SinkFetchURL, Value: "http://oast.example.com/" + activeCanary})
 		reporter.On("Report", mock.MatchedBy(func(f CorrelatedFinding) bool {
 			return f.Canary == activeCanary && f.Sink == schemas.SinkFetchURL
 		})).Once()

internal/analysis/active/taint/taint_shim.js

@@ -31,6 +31,24 @@
         log: (...args) => scope.console.log(`[Scalpel Taint Shim - ${CONTEXT_NAME}]`, ...args)
     };
 
+    // --- VULN-FIX START: Capture Callbacks in Closure ---
+    // Immediately capture the callback functions from the global scope into a local,
+    // closure-scoped variable. This prevents DOM Clobbering that occurs after the
+    // shim has initialized from affecting the shim's own reporting mechanisms.
+    const SinkCallback = scope[CONFIG.SinkCallbackName];
+    const ProofCallback = scope[CONFIG.ProofCallbackName];
+    const ErrorCallback = scope[CONFIG.ErrorCallbackName];
+
+    // Validate that the callbacks were exposed correctly. If not, the analysis is blind.
+    // Use the raw console.error as our own ErrorCallback might be the one that's broken.
+    if (typeof SinkCallback !== 'function' || typeof ProofCallback !== 'function' || typeof ErrorCallback !== 'function') {
+        scope.console.error("[Scalpel] Critical Error: Backend callbacks not exposed correctly or were clobbered before shim initialization. Analysis may be ineffective.");
+        // Abort initialization as the shim cannot function without its callbacks.
+        return;
+    }
+    // --- VULN-FIX END ---
+
+
     // Predefined condition handlers for CSP compatibility.
     const ConditionHandlers = {
         'IS_STRING_ARG0': (args) => typeof args[0] === 'string',
@@ -45,7 +63,8 @@
      * ROBUSTNESS: Reports internal instrumentation errors back to the Go backend.
      */
     function reportShimError(error, location, stack = null) {
-        const callback = scope[CONFIG.ErrorCallbackName];
+        // VULN-FIX: Use the closure-scoped `ErrorCallback` instead of re-resolving from the global scope.
+        const callback = ErrorCallback;
         if (typeof callback === 'function') {
             // Execute asynchronously
             setTimeout(() => {
@@ -61,6 +80,7 @@
                 }
             }, 0);
         } else {
+            // This case should theoretically not be reached due to the initial check, but is kept for robustness.
             logger.error(`Shim Error at ${location}:`, error);
         }
     }
@@ -196,7 +216,8 @@
      * Reports a detected sink event to the Go backend.
      */
     function reportSink(type, value, detail) {
-        const callback = scope[CONFIG.SinkCallbackName];
+        // VULN-FIX: Use the closure-scoped `SinkCallback` instead of re-resolving from the global scope.
+        const callback = SinkCallback;
         if (typeof callback === 'function') {
             const stack = getStackTrace();
             // FIX: Capture page context for FP reduction.
@@ -243,13 +264,17 @@
      * Overrides the execution proof callback to capture stack trace.
      */
     function initializeExecutionProofCallback() {
-        const originalCallback = scope[CONFIG.ProofCallbackName];
+        // VULN-FIX: Use the closure-scoped `ProofCallback` as the original function to wrap.
+        const originalCallback = ProofCallback;
         if (typeof originalCallback !== 'function') {
+            // This case should not be reached due to the initial check, but is kept for robustness.
             logger.error("Backend execution proof callback not exposed correctly.");
             return;
         }
 
         // Replace the exposed Go function (which is a direct binding) with a JS wrapper.
+        // This is necessary because the XSS payload can only call the global function directly.
+        // Our wrapper adds the stack trace before calling the real (captured) callback.
         scope[CONFIG.ProofCallbackName] = function(canary) {
             const stack = getStackTrace();
             // FIX: Capture page context for FP reduction.