From 0b8e83dc872a8e843fabb60fb70e7a7e6cc1e6eb Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 21 Oct 2024 11:55:09 +0200 Subject: [PATCH 1/4] C#: Add log forging false positive example using ReplaceLineEndings. --- .../Security Features/CWE-117/LogForging.cs | 2 ++ .../CWE-117/LogForging.expected | 20 +++++++++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) 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 index 9b8297754635..b5dbfa0f268f 100644 --- 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 (False positive) + 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 index 4d1c27c48437..2f53733bf760 100644 --- 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,25 +1,33 @@ #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:27:21:27:66 | ... + ... | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:27:21:27:66 | ... + ... | 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:27:21:27:28 | access to local variable username : String | 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 | | +| LogForging.cs:27:21:27:28 | access to local variable username : String | LogForging.cs:27:21:27:51 | call to method ReplaceLineEndings : String | provenance | MaD:2 | +| LogForging.cs:27:21:27:51 | call to method ReplaceLineEndings : String | LogForging.cs:27:21:27:66 | ... + ... | provenance | | | LogForgingAsp.cs:8:32:8:39 | username : String | LogForgingAsp.cs:12:21:12:43 | ... + ... | provenance | | models | 1 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated | +| 2 | Summary: System; String; false; ReplaceLineEndings; (System.String); ; Argument[this]; ReturnValue; taint; df-generated | nodes | LogForging.cs:18:16:18:23 | access to local variable username : String | semmle.label | access to local variable username : String | | 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:27:21:27:28 | access to local variable username : String | semmle.label | access to local variable username : String | +| LogForging.cs:27:21:27:51 | call to method ReplaceLineEndings : String | semmle.label | call to method ReplaceLineEndings : String | +| LogForging.cs:27:21:27:66 | ... + ... | semmle.label | ... + ... | +| 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 From b2b1a3ea650fd0a3cbbf876b1e229b5644113e70 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 21 Oct 2024 12:03:59 +0200 Subject: [PATCH 2/4] C#: Consider string.ReplaceLineEndings(string) as a sanitizer for log forging. --- csharp/ql/lib/semmle/code/csharp/frameworks/System.qll | 8 ++++++++ .../code/csharp/security/dataflow/LogForgingQuery.qll | 4 +++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/System.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/System.qll index fcb46d5c9111..ac16df764162 100644 --- 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 index ebb481d2525e..422b5dc717a5 100644 --- 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() | From 191658f637b57336e025cb0a3cd532ddffcee0e3 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 21 Oct 2024 12:04:31 +0200 Subject: [PATCH 3/4] C#: Update expected test output. --- .../query-tests/Security Features/CWE-117/LogForging.cs | 2 +- .../Security Features/CWE-117/LogForging.expected | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-) 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 index b5dbfa0f268f..691d98f986e0 100644 --- 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,7 +23,7 @@ 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 (False positive) + // GOOD: New-lines replaced logger.Warn(username.ReplaceLineEndings("") + " logged in"); // GOOD: Html encoded logger.Warn(WebUtility.HtmlEncode(username) + " logged in"); 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 index 2f53733bf760..f817ebd27b03 100644 --- 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,31 +1,23 @@ #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:27:21:27:66 | ... + ... | LogForging.cs:18:27:18:49 | access to property QueryString : NameValueCollection | LogForging.cs:27:21:27:66 | ... + ... | 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:27:21:27:28 | access to local variable username : String | 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 | | -| LogForging.cs:27:21:27:28 | access to local variable username : String | LogForging.cs:27:21:27:51 | call to method ReplaceLineEndings : String | provenance | MaD:2 | -| LogForging.cs:27:21:27:51 | call to method ReplaceLineEndings : String | LogForging.cs:27:21:27:66 | ... + ... | provenance | | | LogForgingAsp.cs:8:32:8:39 | username : String | LogForgingAsp.cs:12:21:12:43 | ... + ... | provenance | | models | 1 | Summary: System.Collections.Specialized; NameValueCollection; false; get_Item; (System.String); ; Argument[this]; ReturnValue; taint; df-generated | -| 2 | Summary: System; String; false; ReplaceLineEndings; (System.String); ; Argument[this]; ReturnValue; taint; df-generated | nodes | LogForging.cs:18:16:18:23 | access to local variable username : String | semmle.label | access to local variable username : String | | 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:27:21:27:28 | access to local variable username : String | semmle.label | access to local variable username : String | -| LogForging.cs:27:21:27:51 | call to method ReplaceLineEndings : String | semmle.label | call to method ReplaceLineEndings : String | -| LogForging.cs:27:21:27:66 | ... + ... | semmle.label | ... + ... | | 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 | From 1217c55c36cfe36cbc6ebfb2b0741c5a3c219035 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 21 Oct 2024 12:08:03 +0200 Subject: [PATCH 4/4] C#: Add change note. --- .../change-notes/2024-10-21-log-forging-replacelineendings.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/src/change-notes/2024-10-21-log-forging-replacelineendings.md 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 new file mode 100644 index 000000000000..2e309ea185cb --- /dev/null +++ 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. \ No newline at end of file