From dad94207aa9d72d30e6da242e6a8d7f0b507051c Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 8 Jan 2025 14:06:29 +0100 Subject: [PATCH 01/10] C#: Add some implicit toString data flow test cases. --- .../implicittostring/implicitToString.cs | 43 +++++++++++++++++++ .../implicitToString.expected | 20 +++++++++ .../implicittostring/implicitToString.ql | 19 ++++++++ 3 files changed, 82 insertions(+) create mode 100644 csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.cs create mode 100644 csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.expected create mode 100644 csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.ql diff --git a/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.cs b/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.cs new file mode 100644 index 000000000000..0e1406e74ddb --- /dev/null +++ b/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.cs @@ -0,0 +1,43 @@ +using System; + +public class TestClass +{ + public class MyClass() + { + public override string ToString() + { + return "tainted"; + } + } + + public static void Sink(object o) { } + + public void M1() + { + var x1 = new MyClass(); + var x2 = "Hello" + x1.ToString(); + Sink(x2); + } + + public void M2() + { + var x1 = new MyClass(); + var x2 = "Hello" + x1; + Sink(x2); // MISSING + } + + public void M3() + { + var x1 = new MyClass(); + var x2 = $"Hello {x1.ToString()}"; + Sink(x2); + } + + public void M4() + { + var x1 = new MyClass(); + var x2 = $"Hello {x1}"; + Sink(x2); // Missing + } + +} diff --git a/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.expected b/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.expected new file mode 100644 index 000000000000..245c2f091f0d --- /dev/null +++ b/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.expected @@ -0,0 +1,20 @@ +models +edges +| implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:18:28:18:40 | call to method ToString : String | provenance | | +| implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:32:27:32:39 | call to method ToString : String | provenance | | +| implicitToString.cs:18:13:18:14 | access to local variable x2 : String | implicitToString.cs:19:14:19:15 | access to local variable x2 | provenance | | +| implicitToString.cs:18:28:18:40 | call to method ToString : String | implicitToString.cs:18:13:18:14 | access to local variable x2 : String | provenance | | +| implicitToString.cs:32:13:32:14 | access to local variable x2 : String | implicitToString.cs:33:14:33:15 | access to local variable x2 | provenance | | +| implicitToString.cs:32:27:32:39 | call to method ToString : String | implicitToString.cs:32:13:32:14 | access to local variable x2 : String | provenance | | +nodes +| implicitToString.cs:9:20:9:28 | "tainted" : String | semmle.label | "tainted" : String | +| implicitToString.cs:18:13:18:14 | access to local variable x2 : String | semmle.label | access to local variable x2 : String | +| implicitToString.cs:18:28:18:40 | call to method ToString : String | semmle.label | call to method ToString : String | +| implicitToString.cs:19:14:19:15 | access to local variable x2 | semmle.label | access to local variable x2 | +| implicitToString.cs:32:13:32:14 | access to local variable x2 : String | semmle.label | access to local variable x2 : String | +| implicitToString.cs:32:27:32:39 | call to method ToString : String | semmle.label | call to method ToString : String | +| implicitToString.cs:33:14:33:15 | access to local variable x2 | semmle.label | access to local variable x2 | +subpaths +#select +| implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:19:14:19:15 | access to local variable x2 | $@ | implicitToString.cs:19:14:19:15 | access to local variable x2 | access to local variable x2 | +| implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:33:14:33:15 | access to local variable x2 | $@ | implicitToString.cs:33:14:33:15 | access to local variable x2 | access to local variable x2 | diff --git a/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.ql b/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.ql new file mode 100644 index 000000000000..85797414ee5d --- /dev/null +++ b/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.ql @@ -0,0 +1,19 @@ +import csharp +import utils.test.ProvenancePathGraph::ShowProvenance + +module TtConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr().(StringLiteral).getValue() = "tainted" } + + predicate isSink(DataFlow::Node sink) { + exists(MethodCall mc | + mc.getTarget().hasUndecoratedName("Sink") and + mc.getAnArgument() = sink.asExpr() + ) + } +} + +module Tt = TaintTracking::Global; + +from Tt::PathNode source, Tt::PathNode sink +where Tt::flowPath(source, sink) +select source, source, sink, "$@", sink, sink.toString() From fc7681800edb85c5dcf695bc5ad6335bec775054 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 9 Jan 2025 10:39:05 +0100 Subject: [PATCH 02/10] C#: Add a RedundantToStringCall test case. --- .../RedundantToStringCall/RedundantToStringCall.cs | 3 +++ .../RedundantToStringCall/RedundantToStringCall.expected | 1 + 2 files changed, 4 insertions(+) diff --git a/csharp/ql/test/query-tests/Useless Code/RedundantToStringCall/RedundantToStringCall.cs b/csharp/ql/test/query-tests/Useless Code/RedundantToStringCall/RedundantToStringCall.cs index 2f863966f351..981b36002663 100644 --- a/csharp/ql/test/query-tests/Useless Code/RedundantToStringCall/RedundantToStringCall.cs +++ b/csharp/ql/test/query-tests/Useless Code/RedundantToStringCall/RedundantToStringCall.cs @@ -9,5 +9,8 @@ public void M(object o) Console.WriteLine($"Hello: {o.ToString()}"); // BAD Console.WriteLine($"Hello: {o}"); // GOOD + + Console.WriteLine("Hello: " + o.ToString()); // BAD + Console.WriteLine("Hello: " + o); // GOOD } } diff --git a/csharp/ql/test/query-tests/Useless Code/RedundantToStringCall/RedundantToStringCall.expected b/csharp/ql/test/query-tests/Useless Code/RedundantToStringCall/RedundantToStringCall.expected index 8021c21df2ae..28775378f049 100644 --- a/csharp/ql/test/query-tests/Useless Code/RedundantToStringCall/RedundantToStringCall.expected +++ b/csharp/ql/test/query-tests/Useless Code/RedundantToStringCall/RedundantToStringCall.expected @@ -1,3 +1,4 @@ | RedundantToStringCall.cs:7:27:7:38 | call to method ToString | Redundant call to 'ToString' on a String object. | | RedundantToStringCall.cs:10:37:10:48 | call to method ToString | Redundant call to 'ToString' on a String object. | +| RedundantToStringCall.cs:13:39:13:50 | call to method ToString | Redundant call to 'ToString' on a String object. | | RedundantToStringCallBad.cs:7:45:7:56 | call to method ToString | Redundant call to 'ToString' on a String object. | From 87e313688feb058ebadf8dcf6eb7d6d75f08535a Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 8 Jan 2025 14:11:01 +0100 Subject: [PATCH 03/10] C#: Make synthetic ToString calls in binary add expressions. --- .../Entities/Expression.cs | 2 +- .../Entities/ExpressionNodeInfo.cs | 14 ++++- .../Entities/Expressions/Binary.cs | 28 ++++++++- .../Entities/Expressions/ImplicitCast.cs | 6 ++ .../Entities/Expressions/ImplicitToString.cs | 59 +++++++++++++++++++ 5 files changed, 105 insertions(+), 4 deletions(-) create mode 100644 csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitToString.cs diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs index 9241528eb752..a5cb6e316f41 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs @@ -129,7 +129,7 @@ public static void CreateDeferred(Context cx, ExpressionSyntax node, IExpression cx.PopulateLater(() => Create(cx, node, parent, child)); } - private static bool ContainsPattern(SyntaxNode node) => + protected static bool ContainsPattern(SyntaxNode node) => node is PatternSyntax || node is VariableDesignationSyntax || node.ChildNodes().Any(ContainsPattern); /// diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/ExpressionNodeInfo.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/ExpressionNodeInfo.cs index 924382a55507..5a2cad8a520a 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/ExpressionNodeInfo.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/ExpressionNodeInfo.cs @@ -129,7 +129,13 @@ public Location Location public ExprKind Kind { get; set; } = ExprKind.UNKNOWN; - public bool IsCompilerGenerated { get; set; } + public bool IsCompilerGenerated { get; init; } + + /// + /// Whether the expression should have a compiler generated `ToString` call added, + /// if there is no suitable implicit cast. + /// + public bool ImplicitToString { get; private set; } public ExpressionNodeInfo SetParent(IExpressionParentEntity parent, int child) { @@ -157,6 +163,12 @@ public ExpressionNodeInfo SetNode(ExpressionSyntax node) return this; } + public ExpressionNodeInfo SetImplicitToString(bool value) + { + ImplicitToString = value; + return this; + } + private SymbolInfo cachedSymbolInfo; public SymbolInfo SymbolInfo diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Binary.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Binary.cs index 3cdfb32277b2..eeb1b9ba63b2 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Binary.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Binary.cs @@ -14,11 +14,35 @@ private Binary(ExpressionNodeInfo info) public static Expression Create(ExpressionNodeInfo info) => new Binary(info).TryPopulate(); + private Expression CreateChild(Context cx, ExpressionSyntax node, int child) + { + // If this is a "+" expression we might need to wrap the child expressions + // in ToString calls + return Kind == ExprKind.ADD + ? ImplicitToString.Create(cx, node, this, child) + : Create(cx, node, this, child); + } + + /// + /// Creates an expression from a syntax node. + /// Inserts type conversion as required. + /// Population is deferred to avoid overflowing the stack. + /// + private void CreateDeferred(Context cx, ExpressionSyntax node, int child) + { + if (ContainsPattern(node)) + // Expressions with patterns should be created right away, as they may introduce + // local variables referenced in `LocalVariable::GetAlreadyCreated()` + CreateChild(cx, node, child); + else + cx.PopulateLater(() => CreateChild(cx, node, child)); + } + protected override void PopulateExpression(TextWriter trapFile) { OperatorCall(trapFile, Syntax); - CreateDeferred(Context, Syntax.Left, this, 0); - CreateDeferred(Context, Syntax.Right, this, 1); + CreateDeferred(Context, Syntax.Left, 0); + CreateDeferred(Context, Syntax.Right, 1); } private static ExprKind GetKind(Context cx, BinaryExpressionSyntax node) diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitCast.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitCast.cs index b0508bf83b7e..c1ce5dcbfe99 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitCast.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitCast.cs @@ -156,6 +156,12 @@ convertedType.Symbol is IPointerTypeSymbol && return new ImplicitCast(info); } + if (info.ImplicitToString) + { + // x -> x.ToString() in "abc" + x + return ImplicitToString.Wrap(info); + } + // Default: Just create the expression without a conversion. return Factory.Create(info); } diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitToString.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitToString.cs new file mode 100644 index 000000000000..9358a2d5b01c --- /dev/null +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitToString.cs @@ -0,0 +1,59 @@ +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Semmle.Extraction.CSharp.Util; +using Semmle.Extraction.Kinds; + + +namespace Semmle.Extraction.CSharp.Entities.Expressions +{ + internal sealed class ImplicitToString : Expression + { + /// + /// Gets the `ToString` method for the given type. + /// + private static IMethodSymbol? GetToStringMethod(ITypeSymbol? type) + { + return type? + .GetMembers() + .OfType() + .Where(method => + method.GetName() == "ToString" && + method.Parameters.Length == 0 + ) + .FirstOrDefault(); + } + + private ImplicitToString(ExpressionNodeInfo info, IMethodSymbol toString) : base(new ExpressionInfo(info.Context, info.ConvertedType, info.Location, ExprKind.METHOD_INVOCATION, info.Parent, info.Child, isCompilerGenerated: true, info.ExprValue)) + { + Factory.Create(info.SetParent(this, -1)); + + var target = Method.Create(Context, toString); + Context.TrapWriter.Writer.expr_call(this, target); + } + + private static bool IsStringType(AnnotatedTypeSymbol? type) => + type.HasValue && type.Value.Symbol?.SpecialType == SpecialType.System_String; + + /// + /// Creates a new expression, adding a compiler generated `ToString` call if required. + /// + public static Expression Create(Context cx, ExpressionSyntax node, Expression parent, int child) + { + var info = new ExpressionNodeInfo(cx, node, parent, child); + return CreateFromNode(info.SetImplicitToString(IsStringType(parent.Type) && !IsStringType(info.Type))); + } + + /// + /// Wraps the resulting expression in a `ToString` call, if a suitable `ToString` method is available. + /// + public static Expression Wrap(ExpressionNodeInfo info) + { + if (GetToStringMethod(info.Type?.Symbol) is IMethodSymbol toString) + { + return new ImplicitToString(info, toString); + } + return Factory.Create(info); + } + } +} From 829dbd7b92e42b3985f6e485f42a7fa7c34a4e4b Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 8 Jan 2025 14:15:05 +0100 Subject: [PATCH 04/10] C#: Update test expected output. --- .../dataflow/implicittostring/implicitToString.cs | 2 +- .../dataflow/implicittostring/implicitToString.expected | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.cs b/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.cs index 0e1406e74ddb..132100bf38a0 100644 --- a/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.cs +++ b/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.cs @@ -23,7 +23,7 @@ public void M2() { var x1 = new MyClass(); var x2 = "Hello" + x1; - Sink(x2); // MISSING + Sink(x2); } public void M3() diff --git a/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.expected b/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.expected index 245c2f091f0d..448e08b48452 100644 --- a/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.expected +++ b/csharp/ql/test/library-tests/dataflow/implicittostring/implicitToString.expected @@ -1,9 +1,12 @@ models edges | implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:18:28:18:40 | call to method ToString : String | provenance | | +| implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:25:28:25:29 | call to method ToString : String | provenance | | | implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:32:27:32:39 | call to method ToString : String | provenance | | | implicitToString.cs:18:13:18:14 | access to local variable x2 : String | implicitToString.cs:19:14:19:15 | access to local variable x2 | provenance | | | implicitToString.cs:18:28:18:40 | call to method ToString : String | implicitToString.cs:18:13:18:14 | access to local variable x2 : String | provenance | | +| implicitToString.cs:25:13:25:14 | access to local variable x2 : String | implicitToString.cs:26:14:26:15 | access to local variable x2 | provenance | | +| implicitToString.cs:25:28:25:29 | call to method ToString : String | implicitToString.cs:25:13:25:14 | access to local variable x2 : String | provenance | | | implicitToString.cs:32:13:32:14 | access to local variable x2 : String | implicitToString.cs:33:14:33:15 | access to local variable x2 | provenance | | | implicitToString.cs:32:27:32:39 | call to method ToString : String | implicitToString.cs:32:13:32:14 | access to local variable x2 : String | provenance | | nodes @@ -11,10 +14,14 @@ nodes | implicitToString.cs:18:13:18:14 | access to local variable x2 : String | semmle.label | access to local variable x2 : String | | implicitToString.cs:18:28:18:40 | call to method ToString : String | semmle.label | call to method ToString : String | | implicitToString.cs:19:14:19:15 | access to local variable x2 | semmle.label | access to local variable x2 | +| implicitToString.cs:25:13:25:14 | access to local variable x2 : String | semmle.label | access to local variable x2 : String | +| implicitToString.cs:25:28:25:29 | call to method ToString : String | semmle.label | call to method ToString : String | +| implicitToString.cs:26:14:26:15 | access to local variable x2 | semmle.label | access to local variable x2 | | implicitToString.cs:32:13:32:14 | access to local variable x2 : String | semmle.label | access to local variable x2 : String | | implicitToString.cs:32:27:32:39 | call to method ToString : String | semmle.label | call to method ToString : String | | implicitToString.cs:33:14:33:15 | access to local variable x2 | semmle.label | access to local variable x2 | subpaths #select | implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:19:14:19:15 | access to local variable x2 | $@ | implicitToString.cs:19:14:19:15 | access to local variable x2 | access to local variable x2 | +| implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:26:14:26:15 | access to local variable x2 | $@ | implicitToString.cs:26:14:26:15 | access to local variable x2 | access to local variable x2 | | implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:9:20:9:28 | "tainted" : String | implicitToString.cs:33:14:33:15 | access to local variable x2 | $@ | implicitToString.cs:33:14:33:15 | access to local variable x2 | access to local variable x2 | From ac317c3cfc73b6d6aaf144b50e3d8a301232e2f9 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 9 Jan 2025 11:36:17 +0100 Subject: [PATCH 05/10] C#: Modify compiler generated strip logic to also take generated ToString calls into account. --- csharp/ql/examples/snippets/ternary_conditional.ql | 2 +- .../lib/semmle/code/csharp/commons/Constants.qll | 2 +- .../ql/lib/semmle/code/csharp/commons/Strings.qll | 4 ++-- .../lib/semmle/code/csharp/dispatch/Dispatch.qll | 2 +- csharp/ql/lib/semmle/code/csharp/exprs/Call.qll | 4 ++++ csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll | 14 +++++++++++--- .../ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql | 4 ++-- csharp/ql/src/Likely Bugs/ObjectComparison.ql | 2 +- 8 files changed, 23 insertions(+), 11 deletions(-) diff --git a/csharp/ql/examples/snippets/ternary_conditional.ql b/csharp/ql/examples/snippets/ternary_conditional.ql index 22e26e71ea8f..009c56fb2afe 100644 --- a/csharp/ql/examples/snippets/ternary_conditional.ql +++ b/csharp/ql/examples/snippets/ternary_conditional.ql @@ -11,7 +11,7 @@ import csharp from ConditionalExpr e where - e.getThen().stripImplicitCasts() != e.getElse().stripImplicitCasts() and + e.getThen().stripImplicit() != e.getElse().stripImplicit() and not e.getThen().getType() instanceof NullType and not e.getElse().getType() instanceof NullType select e diff --git a/csharp/ql/lib/semmle/code/csharp/commons/Constants.qll b/csharp/ql/lib/semmle/code/csharp/commons/Constants.qll index 1d6e67aa488d..508ba0e5e87d 100644 --- a/csharp/ql/lib/semmle/code/csharp/commons/Constants.qll +++ b/csharp/ql/lib/semmle/code/csharp/commons/Constants.qll @@ -43,7 +43,7 @@ predicate isConstantComparison(ComparisonOperation co, boolean b) { private module ConstantComparisonOperation { private import semmle.code.csharp.commons.ComparisonTest - private SimpleType convertedType(Expr expr) { result = expr.stripImplicitCasts().getType() } + private SimpleType convertedType(Expr expr) { result = expr.stripImplicit().getType() } private int maxValue(Expr expr) { if convertedType(expr) instanceof IntegralType and exists(expr.getValue()) diff --git a/csharp/ql/lib/semmle/code/csharp/commons/Strings.qll b/csharp/ql/lib/semmle/code/csharp/commons/Strings.qll index 4e007d61737e..4b0963977b52 100644 --- a/csharp/ql/lib/semmle/code/csharp/commons/Strings.qll +++ b/csharp/ql/lib/semmle/code/csharp/commons/Strings.qll @@ -44,8 +44,8 @@ class ImplicitToStringExpr extends Expr { ) or exists(AddExpr add, Expr o | o = add.getAnOperand() | - o.stripImplicitCasts().getType() instanceof StringType and - this = add.getOtherOperand(o) + o.stripImplicit().getType() instanceof StringType and + this = add.getOtherOperand(o).stripImplicit() ) or this = any(InterpolatedStringExpr ise).getAnInsert() diff --git a/csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll b/csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll index 080b7d5c7325..7de6c30eb13c 100644 --- a/csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll +++ b/csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll @@ -857,7 +857,7 @@ private module Internal { private predicate hasDynamicArg(int i, Type argumentType) { exists(Expr argument | argument = this.getArgument(i) and - argument.stripImplicitCasts().getType() instanceof DynamicType and + argument.stripImplicit().getType() instanceof DynamicType and argumentType = getAPossibleType(argument, _) ) } diff --git a/csharp/ql/lib/semmle/code/csharp/exprs/Call.qll b/csharp/ql/lib/semmle/code/csharp/exprs/Call.qll index be4577d760eb..eecbc35900aa 100644 --- a/csharp/ql/lib/semmle/code/csharp/exprs/Call.qll +++ b/csharp/ql/lib/semmle/code/csharp/exprs/Call.qll @@ -281,6 +281,10 @@ class MethodCall extends Call, QualifiableExpr, LateBindableExpr, @method_invoca result = this.getArgument(i - 1) else result = this.getArgument(i) } + + override Expr stripImplicit() { + if this.isImplicit() then result = this.getQualifier().stripImplicit() else result = this + } } /** diff --git a/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll b/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll index 935162523a16..85676bbd2701 100644 --- a/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll +++ b/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll @@ -82,10 +82,18 @@ class Expr extends ControlFlowElement, @expr { Expr stripCasts() { result = this } /** + * DEPRECATED: Use `stripImplicit` instead. + * * Gets an expression that is the result of stripping (recursively) all * implicit casts from this expression, if any. */ - Expr stripImplicitCasts() { result = this } + deprecated Expr stripImplicitCasts() { result = this.stripImplicit() } + + /** + * Gets an expression that is the result of stripping (recursively) all + * implicit casts and implicit ToString calls from this expression, if any. + */ + Expr stripImplicit() { result = this } /** * Gets the explicit parameter name used to pass this expression as an @@ -714,8 +722,8 @@ class Cast extends Expr { override Expr stripCasts() { result = this.getExpr().stripCasts() } - override Expr stripImplicitCasts() { - if this.isImplicit() then result = this.getExpr().stripImplicitCasts() else result = this + override Expr stripImplicit() { + if this.isImplicit() then result = this.getExpr().stripImplicit() else result = this } } diff --git a/csharp/ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql b/csharp/ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql index 6044ebbbb5eb..2efac6773f72 100644 --- a/csharp/ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql +++ b/csharp/ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql @@ -44,11 +44,11 @@ abstract class BadDynamicCall extends DynamicExpr { ultimateSsaDef = ssaDef.getAnUltimateDefinition() | ultimateSsaDef.getADefinition() = - any(AssignableDefinition def | source = def.getSource().stripImplicitCasts()) + any(AssignableDefinition def | source = def.getSource().stripImplicit()) or ultimateSsaDef.getADefinition() = any(AssignableDefinitions::ImplicitParameterDefinition p | - source = p.getParameter().getAnAssignedValue().stripImplicitCasts() + source = p.getParameter().getAnAssignedValue().stripImplicit() ) ) } diff --git a/csharp/ql/src/Likely Bugs/ObjectComparison.ql b/csharp/ql/src/Likely Bugs/ObjectComparison.ql index 53b525b6072c..eec1961fbf50 100644 --- a/csharp/ql/src/Likely Bugs/ObjectComparison.ql +++ b/csharp/ql/src/Likely Bugs/ObjectComparison.ql @@ -28,7 +28,7 @@ class ReferenceEqualityTestOnObject extends EqualityOperation { exists(getObjectOperand(this)) and // Neither operand is 'null'. not this.getAnOperand() instanceof NullLiteral and - not exists(Type t | t = this.getAnOperand().stripImplicitCasts().getType() | + not exists(Type t | t = this.getAnOperand().stripImplicit().getType() | t instanceof NullType or t instanceof ValueType ) and From f0e5bd997404c1213f8b4de8608cec1ec0a7c50a Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 9 Jan 2025 11:40:42 +0100 Subject: [PATCH 06/10] C#: Update other existing tests expected output. --- .../security/dataflow/flowsources/StoredFlowSources.expected | 1 + 1 file changed, 1 insertion(+) diff --git a/csharp/ql/test/library-tests/security/dataflow/flowsources/StoredFlowSources.expected b/csharp/ql/test/library-tests/security/dataflow/flowsources/StoredFlowSources.expected index 28156f12272e..e27ea53adbd1 100644 --- a/csharp/ql/test/library-tests/security/dataflow/flowsources/StoredFlowSources.expected +++ b/csharp/ql/test/library-tests/security/dataflow/flowsources/StoredFlowSources.expected @@ -6,6 +6,7 @@ | data.cs:28:35:28:71 | ... + ... | | data.cs:28:51:28:64 | access to local variable customerReader | | data.cs:28:51:28:71 | access to indexer | +| data.cs:28:51:28:71 | call to method ToString | | data.cs:30:13:30:26 | access to local variable customerReader | | entity.cs:31:29:31:82 | DbRawSqlQuery blogs = ... | | entity.cs:31:37:31:82 | call to method SqlQuery | From cdcc25756b4fc407b34e9e2dd46857cc89c927ac Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 9 Jan 2025 11:42:35 +0100 Subject: [PATCH 07/10] C#: Add change note. --- csharp/ql/lib/change-notes/2025-01-09-implicit-to-string.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/lib/change-notes/2025-01-09-implicit-to-string.md diff --git a/csharp/ql/lib/change-notes/2025-01-09-implicit-to-string.md b/csharp/ql/lib/change-notes/2025-01-09-implicit-to-string.md new file mode 100644 index 000000000000..ffd474fd4f5e --- /dev/null +++ b/csharp/ql/lib/change-notes/2025-01-09-implicit-to-string.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added extractor support for extracting implicit `ToString` calls in binary `+` expressions. From 3754177cc65fbddc4e352cf515aae30216bec684 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 9 Jan 2025 13:50:10 +0100 Subject: [PATCH 08/10] C#: Add PrintAst test to implicit ToString test. --- .../implicittostring/PrintAst.expected | 93 +++++++++++++++++++ .../dataflow/implicittostring/PrintAst.qlref | 1 + 2 files changed, 94 insertions(+) create mode 100644 csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.expected create mode 100644 csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.qlref diff --git a/csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.expected b/csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.expected new file mode 100644 index 000000000000..fc9405d420bf --- /dev/null +++ b/csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.expected @@ -0,0 +1,93 @@ +implicitToString.cs: +# 3| [Class] TestClass +# 5| 5: [Class] MyClass +# 5| 4: [InstanceConstructor,PrimaryConstructor] MyClass +# 7| 5: [Method] ToString +# 7| -1: [TypeMention] string +# 8| 4: [BlockStmt] {...} +# 9| 0: [ReturnStmt] return ...; +# 9| 0: [StringLiteralUtf16] "tainted" +# 13| 6: [Method] Sink +# 13| -1: [TypeMention] Void +#-----| 2: (Parameters) +# 13| 0: [Parameter] o +# 13| -1: [TypeMention] object +# 13| 4: [BlockStmt] {...} +# 15| 7: [Method] M1 +# 15| -1: [TypeMention] Void +# 16| 4: [BlockStmt] {...} +# 17| 0: [LocalVariableDeclStmt] ... ...; +# 17| 0: [LocalVariableDeclAndInitExpr] MyClass x1 = ... +# 17| -1: [TypeMention] MyClass +# 17| 0: [LocalVariableAccess] access to local variable x1 +# 17| 1: [ObjectCreation] object creation of type MyClass +# 17| 0: [TypeMention] MyClass +# 18| 1: [LocalVariableDeclStmt] ... ...; +# 18| 0: [LocalVariableDeclAndInitExpr] String x2 = ... +# 18| -1: [TypeMention] string +# 18| 0: [LocalVariableAccess] access to local variable x2 +# 18| 1: [AddExpr] ... + ... +# 18| 0: [StringLiteralUtf16] "Hello" +# 18| 1: [MethodCall] call to method ToString +# 18| -1: [LocalVariableAccess] access to local variable x1 +# 19| 2: [ExprStmt] ...; +# 19| 0: [MethodCall] call to method Sink +# 19| 0: [LocalVariableAccess] access to local variable x2 +# 22| 8: [Method] M2 +# 22| -1: [TypeMention] Void +# 23| 4: [BlockStmt] {...} +# 24| 0: [LocalVariableDeclStmt] ... ...; +# 24| 0: [LocalVariableDeclAndInitExpr] MyClass x1 = ... +# 24| -1: [TypeMention] MyClass +# 24| 0: [LocalVariableAccess] access to local variable x1 +# 24| 1: [ObjectCreation] object creation of type MyClass +# 24| 0: [TypeMention] MyClass +# 25| 1: [LocalVariableDeclStmt] ... ...; +# 25| 0: [LocalVariableDeclAndInitExpr] String x2 = ... +# 25| -1: [TypeMention] string +# 25| 0: [LocalVariableAccess] access to local variable x2 +# 25| 1: [AddExpr] ... + ... +# 25| 0: [StringLiteralUtf16] "Hello" +# 26| 2: [ExprStmt] ...; +# 26| 0: [MethodCall] call to method Sink +# 26| 0: [LocalVariableAccess] access to local variable x2 +# 29| 9: [Method] M3 +# 29| -1: [TypeMention] Void +# 30| 4: [BlockStmt] {...} +# 31| 0: [LocalVariableDeclStmt] ... ...; +# 31| 0: [LocalVariableDeclAndInitExpr] MyClass x1 = ... +# 31| -1: [TypeMention] MyClass +# 31| 0: [LocalVariableAccess] access to local variable x1 +# 31| 1: [ObjectCreation] object creation of type MyClass +# 31| 0: [TypeMention] MyClass +# 32| 1: [LocalVariableDeclStmt] ... ...; +# 32| 0: [LocalVariableDeclAndInitExpr] String x2 = ... +# 32| -1: [TypeMention] string +# 32| 0: [LocalVariableAccess] access to local variable x2 +# 32| 1: [InterpolatedStringExpr] $"..." +# 32| 0: [StringLiteralUtf16] "Hello " +# 32| 1: [MethodCall] call to method ToString +# 32| -1: [LocalVariableAccess] access to local variable x1 +# 33| 2: [ExprStmt] ...; +# 33| 0: [MethodCall] call to method Sink +# 33| 0: [LocalVariableAccess] access to local variable x2 +# 36| 10: [Method] M4 +# 36| -1: [TypeMention] Void +# 37| 4: [BlockStmt] {...} +# 38| 0: [LocalVariableDeclStmt] ... ...; +# 38| 0: [LocalVariableDeclAndInitExpr] MyClass x1 = ... +# 38| -1: [TypeMention] MyClass +# 38| 0: [LocalVariableAccess] access to local variable x1 +# 38| 1: [ObjectCreation] object creation of type MyClass +# 38| 0: [TypeMention] MyClass +# 39| 1: [LocalVariableDeclStmt] ... ...; +# 39| 0: [LocalVariableDeclAndInitExpr] String x2 = ... +# 39| -1: [TypeMention] string +# 39| 0: [LocalVariableAccess] access to local variable x2 +# 39| 1: [InterpolatedStringExpr] $"..." +# 39| 0: [StringLiteralUtf16] "Hello " +# 39| 1: [LocalVariableAccess] access to local variable x1 +# 40| 2: [ExprStmt] ...; +# 40| 0: [MethodCall] call to method Sink +# 40| 0: [LocalVariableAccess] access to local variable x2 +# 25| [LocalVariableAccess] access to local variable x1 diff --git a/csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.qlref b/csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.qlref new file mode 100644 index 000000000000..f867dd01f9f8 --- /dev/null +++ b/csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.qlref @@ -0,0 +1 @@ +shared/PrintAst.ql \ No newline at end of file From 33bc6049b33c91f8c7d4f50b68cb34bbeef1b5cd Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 9 Jan 2025 13:04:47 +0100 Subject: [PATCH 09/10] C#: Update AST printing to include generated cast (wrapping) expressions and generated ToString calls. --- csharp/ql/lib/semmle/code/csharp/PrintAst.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/csharp/ql/lib/semmle/code/csharp/PrintAst.qll b/csharp/ql/lib/semmle/code/csharp/PrintAst.qll index 281e157975ab..fd4bf1cb86b0 100644 --- a/csharp/ql/lib/semmle/code/csharp/PrintAst.qll +++ b/csharp/ql/lib/semmle/code/csharp/PrintAst.qll @@ -32,7 +32,9 @@ private predicate shouldPrint(Element e, Location l) { } private predicate isImplicitExpression(ControlFlowElement element) { - element.(Expr).isImplicit() and + // Include compiler generated cast expressions and `ToString` calls if + // they wrap actual source expressions. + element.(Expr).stripImplicit().isImplicit() and not element instanceof CastExpr and not element.(OperatorCall).getTarget() instanceof ImplicitConversionOperator and not element instanceof ElementInitializer From b58605978ceedb708b8f6f190c8ba7ef4ea227f3 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 9 Jan 2025 13:52:37 +0100 Subject: [PATCH 10/10] C#: Update PrintAst expected test output. --- .../library-tests/dataflow/implicittostring/PrintAst.expected | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.expected b/csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.expected index fc9405d420bf..c8e5ec397111 100644 --- a/csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.expected +++ b/csharp/ql/test/library-tests/dataflow/implicittostring/PrintAst.expected @@ -48,6 +48,8 @@ implicitToString.cs: # 25| 0: [LocalVariableAccess] access to local variable x2 # 25| 1: [AddExpr] ... + ... # 25| 0: [StringLiteralUtf16] "Hello" +# 25| 1: [MethodCall] call to method ToString +# 25| -1: [LocalVariableAccess] access to local variable x1 # 26| 2: [ExprStmt] ...; # 26| 0: [MethodCall] call to method Sink # 26| 0: [LocalVariableAccess] access to local variable x2 @@ -90,4 +92,3 @@ implicitToString.cs: # 40| 2: [ExprStmt] ...; # 40| 0: [MethodCall] call to method Sink # 40| 0: [LocalVariableAccess] access to local variable x2 -# 25| [LocalVariableAccess] access to local variable x1