Add missing string replacement sanitizers to log-injection and string-break by owen-mc · Pull Request #11910 · github/codeql

owen-mc · 2023-01-17T14:24:22Z

Add strings.Replacer.Replace and strings.Replacer.WriteString as sanitizers for log-injection and string-break.

This is a resurrection of github/codeql-go#731.

go/ql/lib/semmle/go/security/LogInjectionCustomizations.qll

go/ql/lib/semmle/go/security/StringBreakCustomizations.qll

owen-mc · 2023-01-17T15:44:20Z

Performance evaluation showed no changes

mbg

Looks good to me aside from the one question I added, but might also be good for @smowton to sanity check in case I have missed something.

mbg · 2023-01-17T16:24:27Z

go/ql/lib/semmle/go/security/LogInjectionCustomizations.qll

+  class StringsNewReplacerCall extends DataFlow::CallNode {
+    StringsNewReplacerCall() { this.getTarget().hasQualifiedName("strings", "NewReplacer") }
+
+    /**
+     * Gets an argument to this call corresponding to a string that will be
+     * replaced.
+     */
+    DataFlow::Node getAReplacedArgument() {
+      exists(int n | n % 2 = 0 and result = this.getArgument(n))
+    }
+  }
+
+  /**
+   * A configuration for tracking flow from a call to `strings.NewReplacer` to
+   * the receiver of a call to `strings.Replacer.Replace` or
+   * `strings.Replacer.WriteString`.
+   */
+  class StringsNewReplacerConfiguration extends DataFlow2::Configuration {
+    StringsNewReplacerConfiguration() { this = "StringsNewReplacerConfiguration" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof StringsNewReplacerCall }
+
+    override predicate isSink(DataFlow::Node sink) {
+      exists(DataFlow::MethodCallNode call |
+        sink = call.getReceiver() and
+        call.getTarget().hasQualifiedName("strings", "Replacer", ["Replace", "WriteString"])
+      )
+    }
+  }


This part seems to be identical between LogInjectionCustomizations and StringBreakCustomizations. Would it make sense / be possible to de-duplicate this in a shared location?

Good idea. It feels like something that should be usable in many different places. I've moved it to StringOps.qll, which should hopefully make that simple. I've started a performance evaluation as this is a fairly major change.

The performance analysis was neutral.

smowton

Looks good -- no comments on top of @mbg's query

owen-mc · 2023-01-19T13:54:49Z

I've updated this to use its own copy of the dataflow library. The failing QLDoc test is because of an undocumented predicate in the dataflow library, which obviously I'm not going to fix in this PR, so I believe it should be ignored.

smowton

Made an import of an internal library private; otherwise LGTM

…ion (CWE-117) Replace custom byte-level loop with package-level strings.NewReplacer variable (logSanitizer). CodeQL explicitly recognizes strings.Replacer.Replace as a sanitizer for go/log-injection since github/codeql#11910. Call logSanitizer.Replace() directly at each log site. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

…176) * Add lifecycle management for sandbox processes Introduces new lifecycle features to the sandbox API, including the ability to force stop processes and retrieve the current sandbox status. The `/stop` endpoint allows for immediate or scheduled removal of the keepAlive flag from running processes, enabling auto-hibernation. The `/status` endpoint provides information on the current state of the sandbox and active keepAlive processes. Additionally, integrates the lifecycle management with the MCP tools, enhancing the overall process control and monitoring capabilities. The scale-to-zero functionality is also improved with crash recovery mechanisms. Updates include: - New `LifecycleHandler` for managing lifecycle operations. - API documentation updates for new endpoints. - Integration of keepAlive functionality in process management. - Comprehensive tests for lifecycle features and MCP integration. * Enhance scale-to-zero functionality with improved error handling and logging * Remove lifecycle management endpoints and related functionality This commit removes the `/stop` and `/status` endpoints from the sandbox API, along with the associated `LifecycleHandler` and related data structures. The lifecycle management features, including the ability to force stop processes and retrieve the current sandbox status, have been deprecated. Updates include: - Deletion of lifecycle-related API routes and handlers. - Removal of lifecycle management types and structures from the codebase. - Adjustments to documentation to reflect the removal of these features. This change simplifies the API and focuses on core process management functionalities. * Fix PR * Fix bug introduce with AI check * Implement keepAlive timeout handling for restarted processes This commit introduces functionality to manage timeouts for processes with the keepAlive flag enabled. If a process is restarted and keepAlive is active with a specified timeout, a goroutine is initiated to monitor the timeout and kill the process if it exceeds the limit. For processes with an infinite timeout, the goroutine simply waits for the process to complete. This enhancement improves process management and ensures better resource handling. * Fix some recommendation from AI * Enhance logging in ProcessManager to include process name in scale-to-zero warnings and timeout messages. Clear KeepAlive state before killing processes to prevent double ScaleEnable calls. Refactor related log messages for consistency. * Refactor logging in ProcessManager to use structured log entries for KeepAlive events. Enhance clarity by including process details in log messages for scale-to-zero operations and timeout handling. This improves consistency and debuggability of process management logs. * Sanitize user-provided values in log entries to prevent log injection (CWE-117) Add sanitizeLogValue() helper that escapes newlines and control characters. Replace structured logging (logrus.WithFields) with simple logrus.Infof/Warnf calls that use sanitized values - clearer and more readable. Addresses all CodeQL 'Log entries created from user input' warnings. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * Improve sanitizeLogValue to strip all control characters (CWE-117) Replace strings.NewReplacer with byte-level filtering that strips all control characters (< 0x20) including newlines. This provides more thorough sanitization against log injection. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * Use strings.NewReplacer as CodeQL-recognized sanitizer for log injection (CWE-117) Replace custom byte-level loop with package-level strings.NewReplacer variable (logSanitizer). CodeQL explicitly recognizes strings.Replacer.Replace as a sanitizer for go/log-injection since github/codeql#11910. Call logSanitizer.Replace() directly at each log site. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * Use %q format directive for user-provided values in log entries (CWE-117) CodeQL's SafeFormatArgumentSanitizer explicitly recognizes %q as safe because it escapes newline characters. This is the simplest and most idiomatic fix - no helper functions or variables needed. Removes the logSanitizer variable entirely. User-provided values (name, command) are now logged with %q which produces Go-syntax quoted strings, making any control characters visible in the output. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * Revert sanitize/log changes - restore original structured logging Reverts commits 6902a62, 16773fb, 4eeb6f7, 3a5b776 which added log sanitization for CWE-117. Keeps all other agent recommendations (structured logging with logrus.WithFields, race condition fix, etc). Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * Fix StopProcess keepAlive leak and KeepAlive data race - StopProcess (SIGTERM path) now clears KeepAlive and calls ScaleEnable, preventing the scale-to-zero counter from leaking when a keepAlive process is gracefully stopped. - All reads/writes of process.KeepAlive are now synchronized via pm.mu: KillProcess and StopProcess write under Lock(), completion goroutines read under RLock(). This eliminates the data race between the kill/stop path and the completion goroutine. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * Fix data race on oldProcess.KeepAlive read in restartProcess Protect the read of oldProcess.KeepAlive and oldProcess.Timeout in restartProcess with pm.mu.RLock(), matching the synchronization used by KillProcess/StopProcess which write under pm.mu.Lock(). Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: cploujoux <ch.ploujoux@gmail.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

owen-mc requested a review from a team as a code owner January 17, 2023 14:24

github-actions bot added documentation Go labels Jan 17, 2023

owen-mc mentioned this pull request Jan 17, 2023

False positive in go/log-injection #9016

Closed

github-advanced-security bot found potential problems Jan 17, 2023

View reviewed changes

go/ql/lib/semmle/go/security/LogInjectionCustomizations.qll Fixed Show fixed Hide fixed

go/ql/lib/semmle/go/security/StringBreakCustomizations.qll Fixed Show fixed Hide fixed

owen-mc force-pushed the go/log-injection-sanitizer-newreplacer-replace branch from 6282385 to 9d2d4f4 Compare January 17, 2023 15:43

mbg previously approved these changes Jan 17, 2023

View reviewed changes

smowton previously approved these changes Jan 17, 2023

View reviewed changes

owen-mc added 4 commits January 18, 2023 15:12

Move ReplaceAll sanitizer to shared code

2b1a789

Add use of strings.Replacer to replace sanitizer

015ef4c

Add string replacement sanitizer to log injection

30f0dd8

Add change note

3fda9f6

owen-mc dismissed stale reviews from smowton and mbg via 3fda9f6 January 18, 2023 15:42

owen-mc force-pushed the go/log-injection-sanitizer-newreplacer-replace branch from 9d2d4f4 to 3fda9f6 Compare January 18, 2023 15:42

Make new data flow copy for StringOps.StringsNewReplacer

13d1c88

Make import private

9a5e1f5

smowton approved these changes Jan 19, 2023

View reviewed changes

mbg merged commit 14cc27e into github:main Jan 19, 2023

owen-mc deleted the go/log-injection-sanitizer-newreplacer-replace branch January 19, 2023 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing string replacement sanitizers to log-injection and string-break#11910

Add missing string replacement sanitizers to log-injection and string-break#11910
mbg merged 6 commits intogithub:mainfrom
owen-mc:go/log-injection-sanitizer-newreplacer-replace

owen-mc commented Jan 17, 2023

Uh oh!

Uh oh!

Uh oh!

owen-mc commented Jan 17, 2023

Uh oh!

mbg left a comment

Uh oh!

mbg Jan 17, 2023

Uh oh!

owen-mc Jan 18, 2023

Uh oh!

owen-mc Jan 18, 2023

Uh oh!

smowton left a comment

Uh oh!

owen-mc commented Jan 19, 2023

Uh oh!

smowton left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

owen-mc commented Jan 17, 2023

Uh oh!

Uh oh!

Uh oh!

owen-mc commented Jan 17, 2023

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

mbg Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

owen-mc Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

owen-mc Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

owen-mc commented Jan 19, 2023

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants