Merge pull request #17815 from michaelnebel/csharp/logforgingreplacel…

…ineending C#: Add sanitizer to `cs/log-forging`.
github · Oct 22, 2024 · 16fe7a2 · 16fe7a2
2 parents 2312f9d + 1217c55
commit 16fe7a2
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 7 deletions.
diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/System.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/System.qll
@@ -354,6 +354,14 @@ class SystemStringClass extends StringType {
     result.getReturnType() instanceof StringType
   }
 
+  /** Gets the `ReplaceLineEndings(string) method. */
+  Method getReplaceLineEndingsMethod() {
+    result.getDeclaringType() = this and
+    result.hasName("ReplaceLineEndings") and
+    result.getNumberOfParameters() = 1 and
+    result.getReturnType() instanceof StringType
+  }
+
   /** Gets a `Format(...)` method. */
   Method getFormatMethod() {
     result.getDeclaringType() = this and

diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/LogForgingQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/LogForgingQuery.qll
@@ -70,7 +70,9 @@ private class ExternalLoggingExprSink extends Sink {
 private class StringReplaceSanitizer extends Sanitizer {
   StringReplaceSanitizer() {
     exists(Method m |
-      exists(SystemStringClass s | m = s.getReplaceMethod() or m = s.getRemoveMethod())
+      exists(SystemStringClass s |
+        m = s.getReplaceMethod() or m = s.getRemoveMethod() or m = s.getReplaceLineEndingsMethod()
+      )
       or
       m = any(SystemTextRegularExpressionsRegexClass r).getAReplaceMethod()
     |

diff --git a/csharp/ql/src/change-notes/2024-10-21-log-forging-replacelineendings.md b/csharp/ql/src/change-notes/2024-10-21-log-forging-replacelineendings.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* C#: The method `string.ReplaceLineEndings(string)` is now considered a sanitizer for the `cs/log-forging` query. 
diff --git a/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.cs b/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.cs
@@ -23,6 +23,8 @@ public void ProcessRequest(HttpContext ctx)
         logger.Warn(username.Replace(Environment.NewLine, "") + " logged in");
         // GOOD: New-lines removed
         logger.Warn(username.Replace(Environment.NewLine, "", StringComparison.InvariantCultureIgnoreCase) + " logged in");
+        // GOOD: New-lines replaced
+        logger.Warn(username.ReplaceLineEndings("") + " logged in");
         // GOOD: Html encoded
         logger.Warn(WebUtility.HtmlEncode(username) + " logged in");
         // BAD: Logged as-is to TraceSource

diff --git a/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.expected b/csharp/ql/test/query-tests/Security Features/CWE-117/LogForging.expected
@@ -1,12 +1,12 @@
 #select
 | LogForging.cs:21:21:21:43 | ... + ... | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:21:21:21:43 | ... + ... | This log entry depends on a $@. | LogForging.cs:18:27:18:49 | access to property QueryString | user-provided value |
-| LogForging.cs:29:50:29:72 | ... + ... | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:29:50:29:72 | ... + ... | This log entry depends on a $@. | LogForging.cs:18:27:18:49 | access to property QueryString | user-provided value |
-| LogForging.cs:33:26:33:33 | access to local variable username | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:33:26:33:33 | access to local variable username | This log entry depends on a $@. | LogForging.cs:18:27:18:49 | access to property QueryString | user-provided value |
+| LogForging.cs:31:50:31:72 | ... + ... | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:31:50:31:72 | ... + ... | This log entry depends on a $@. | LogForging.cs:18:27:18:49 | access to property QueryString | user-provided value |
+| LogForging.cs:35:26:35:33 | access to local variable username | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:35:26:35:33 | access to local variable username | This log entry depends on a $@. | LogForging.cs:18:27:18:49 | access to property QueryString | user-provided value |
 | LogForgingAsp.cs:12:21:12:43 | ... + ... | LogForgingAsp.cs:8:32:8:39 | username : String | LogForgingAsp.cs:12:21:12:43 | ... + ... | This log entry depends on a $@. | LogForgingAsp.cs:8:32:8:39 | username | user-provided value |
 edges
 | LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:21:21:21:43 | ... + ... | provenance |  |
-| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:29:50:29:72 | ... + ... | provenance |  |
-| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:33:26:33:33 | access to local variable username | provenance |  |
+| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:31:50:31:72 | ... + ... | provenance |  |
+| LogForging.cs:18:16:18:23 | access to local variable username : String | LogForging.cs:35:26:35:33 | access to local variable username | provenance |  |
 | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:18:16:18:23 | access to local variable username : String | provenance |  |
 | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:18:27:18:61 | access to indexer : String | provenance | MaD:1 |
 | LogForging.cs:18:27:18:61 | access to indexer : String | LogForging.cs:18:16:18:23 | access to local variable username : String | provenance |  |
@@ -18,8 +18,8 @@ nodes
 | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
 | LogForging.cs:18:27:18:61 | access to indexer : String | semmle.label | access to indexer : String |
 | LogForging.cs:21:21:21:43 | ... + ... | semmle.label | ... + ... |
-| LogForging.cs:29:50:29:72 | ... + ... | semmle.label | ... + ... |
-| LogForging.cs:33:26:33:33 | access to local variable username | semmle.label | access to local variable username |
+| LogForging.cs:31:50:31:72 | ... + ... | semmle.label | ... + ... |
+| LogForging.cs:35:26:35:33 | access to local variable username | semmle.label | access to local variable username |
 | LogForgingAsp.cs:8:32:8:39 | username : String | semmle.label | username : String |
 | LogForgingAsp.cs:12:21:12:43 | ... + ... | semmle.label | ... + ... |
 subpaths