diff --git a/csharp/extractor/Semmle.Util/FileUtils.cs b/csharp/extractor/Semmle.Util/FileUtils.cs index d17a36c8ed1e..22467a921577 100644 --- a/csharp/extractor/Semmle.Util/FileUtils.cs +++ b/csharp/extractor/Semmle.Util/FileUtils.cs @@ -57,7 +57,7 @@ public static void TryDelete(string file) /// Finds the path for the program based on the /// PATH environment variable, and in the case of Windows the /// PATHEXT environment variable. - /// + /// /// Returns null of no path can be found. /// public static string FindProgramOnPath(string prog) diff --git a/csharp/ql/src/Security Features/CWE-134/UncontrolledFormatStringBad.cs b/csharp/ql/src/Security Features/CWE-134/UncontrolledFormatStringBad.cs index 720354e9a032..dc0c689eefa5 100644 --- a/csharp/ql/src/Security Features/CWE-134/UncontrolledFormatStringBad.cs +++ b/csharp/ql/src/Security Features/CWE-134/UncontrolledFormatStringBad.cs @@ -7,7 +7,7 @@ public class HttpHandler : IHttpHandler public void ProcessRequest(HttpContext ctx) { string format = ctx.Request.QueryString["nameformat"]; - + // BAD: Uncontrolled format string. FormattedName = string.Format(format, Surname, Forenames); } diff --git a/csharp/ql/src/semmle/code/csharp/dispatch/Dispatch.qll b/csharp/ql/src/semmle/code/csharp/dispatch/Dispatch.qll index 97f8c6034523..82a6c1536a6c 100644 --- a/csharp/ql/src/semmle/code/csharp/dispatch/Dispatch.qll +++ b/csharp/ql/src/semmle/code/csharp/dispatch/Dispatch.qll @@ -222,27 +222,55 @@ private module Internal { } } - private class FieldOrProperty extends Assignable { - FieldOrProperty() { - this instanceof Field or - this instanceof Property + private class DynamicFieldOrProperty extends Assignable { + DynamicFieldOrProperty() { + ( + this instanceof Field or + this instanceof Property + ) and + this.getName() = any(DynamicMemberAccess dma).getLateBoundTargetName() + } + + predicate isMemberOf(string name, ValueOrRefType t) { + name = this.getName() and t.hasMember(this) + } + } + + private class TypeWithDynamicFieldOrProperty extends ValueOrRefType { + DynamicFieldOrProperty fp; + + TypeWithDynamicFieldOrProperty() { fp.isMemberOf(_, this) } + + predicate isImplicitlyConvertibleTo(string name, Type t) { + name = fp.getName() and + this.isImplicitlyConvertibleTo(t) } } + pragma[noinline] + private predicate isPossibleDynamicMemberAccessQualifierType( + DynamicMemberAccess dma, string name, TypeWithDynamicFieldOrProperty t + ) { + exists(Type qt, boolean isExact | + qt = getAPossibleType(dma.getQualifier(), isExact) and + name = dma.getLateBoundTargetName() + | + isExact = true and t = qt + or + isExact = false and t.isImplicitlyConvertibleTo(name, qt) + ) + } + /** * Gets a possible type for expression `e`. Simple flow is used to track the * origin of `e`, and in case `e` is a dynamic member access, only types * corresponding to the type of a relevant field or property are included. */ private Type getAPossibleType(Expr e, boolean isExact) { - exists(ValueOrRefType qualifierType, FieldOrProperty fp, boolean qualifierTypeIsExact | - qualifierType = getAPossibleTypeDynamicMemberAccessQualifier(e, qualifierTypeIsExact, fp) + exists(DynamicFieldOrProperty fp, string name, TypeWithDynamicFieldOrProperty t | + isPossibleDynamicMemberAccessQualifierType(e, name, t) and + fp.isMemberOf(name, t) | - ( - if qualifierTypeIsExact = true - then qualifierType.hasMember(fp) - else fp.getDeclaringType().isImplicitlyConvertibleTo(qualifierType) - ) and result = fp.getType() and isExact = false ) @@ -251,13 +279,6 @@ private module Internal { result = getASourceType(e, isExact) } - private Type getAPossibleTypeDynamicMemberAccessQualifier( - DynamicMemberAccess dma, boolean isExact, FieldOrProperty fp - ) { - result = getAPossibleType(dma.getQualifier(), isExact) and - fp.getName() = dma.getLateBoundTargetName() - } - /** * Provides the predicate `getASourceType()` for finding all relevant source * types for a given expression. @@ -799,22 +820,14 @@ private module Internal { // conflicting types (for example, `Tuple` is considered // compatible with `Tuple`). override RuntimeCallable getADynamicTarget() { - // Condition 1 - result = getADynamicTargetCandidate() and - // Condition 2 - forall(int i | i in [0 .. getNumberOfArguments() - 1] | - result = getADynamicTargetCandidateWithCompatibleArg(i) - ) - } - - private RuntimeCallable getADynamicTargetCandidateWithCompatibleArg(int i) { - result = getADynamicTargetCandidateWithCompatibleArg1(i) or - result = getADynamicTargetCandidateWithCompatibleArg2(i) + result = this.getADynamicTarget(this.getNumberOfArguments() - 1) } - pragma[noinline] - private RuntimeCallable getADynamicTargetCandidateWithCompatibleArg1(int i) { - result = this.getADynamicTargetCandidate() and + private RuntimeCallable getADynamicTarget(int i) { + i = -1 and + result = this.getADynamicTargetCandidate() + or + result = this.getADynamicTarget(i - 1) and exists(Type parameterType, Type argumentType | parameterType = this.getAParameterType(result, i) and argumentType = getAPossibleType(this.getArgument(i), _) @@ -827,6 +840,12 @@ private module Internal { or reflectionOrDynamicArgEqualsParamModuloTypeParameters(argumentType, parameterType) ) + or + result = this.getADynamicTarget(i - 1) and + exists(Type parameterType, Type t | parameterType = this.getAParameterType(result, i) | + this.argumentConvConstExpr(i, t) and + t.isImplicitlyConvertibleTo(parameterType) + ) } private Type getAParameterType(RuntimeCallable c, int i) { @@ -840,15 +859,6 @@ private module Internal { ) } - pragma[noinline] - private RuntimeCallable getADynamicTargetCandidateWithCompatibleArg2(int i) { - result = this.getADynamicTargetCandidate() and - exists(Type parameterType, Type t | parameterType = this.getAParameterType(result, i) | - this.argumentConvConstExpr(i, t) and - t.isImplicitlyConvertibleTo(parameterType) - ) - } - pragma[noinline] private predicate argumentConvConstExpr(int i, Type t) { convConstantExpr(this.getArgument(i), t) @@ -954,7 +964,7 @@ private module Internal { */ private predicate isReflectionOrDynamicCallArgumentWithTypeParameters(Type argType, Type paramType) { exists(DispatchReflectionOrDynamicCall call, Parameter p, int i, int j | - p = call.getAStaticTarget().getParameter(i) and + p = call.getADynamicTargetCandidate().getParameter(i) and ( if p.isParams() then ( diff --git a/csharp/ql/src/semmle/code/csharp/exprs/Expr.qll b/csharp/ql/src/semmle/code/csharp/exprs/Expr.qll index c3b824bc4f13..005471a8c599 100644 --- a/csharp/ql/src/semmle/code/csharp/exprs/Expr.qll +++ b/csharp/ql/src/semmle/code/csharp/exprs/Expr.qll @@ -654,6 +654,14 @@ class QualifiableExpr extends Expr, @qualifiable_expr { predicate isConditional() { conditional_access(this) } } +private Expr getAnAssignOrForeachChild() { + result = any(AssignExpr e).getLValue() + or + result = any(ForeachStmt fs).getVariableDeclTuple() + or + result = getAnAssignOrForeachChild().getAChildExpr() +} + /** * An expression representing a tuple, for example * `(1, 2)` on line 2 or `(var x, var y)` on line 5 in @@ -678,10 +686,7 @@ class TupleExpr extends Expr, @tuple_expr { Expr getAnArgument() { result = getArgument(_) } /** Holds if this tuple is a read access. */ - predicate isReadAccess() { - not exists(AssignExpr e | this = e.getLValue().getAChildExpr*()) and - not exists(ForeachStmt fs | this = fs.getVariableDeclTuple().getAChildExpr*()) - } + predicate isReadAccess() { not this = getAnAssignOrForeachChild() } } /** diff --git a/csharp/ql/test/library-tests/controlflow/splits/SplittingStressTest.cs b/csharp/ql/test/library-tests/controlflow/splits/SplittingStressTest.cs index 7e76e7a9838b..f3a4d061c392 100644 --- a/csharp/ql/test/library-tests/controlflow/splits/SplittingStressTest.cs +++ b/csharp/ql/test/library-tests/controlflow/splits/SplittingStressTest.cs @@ -166,4 +166,4 @@ void M(bool b1, bool b2, bool b3, bool b4, bool b5, bool b6, bool b7, bool b8, b ; ; } -} \ No newline at end of file +} diff --git a/csharp/ql/test/query-tests/Concurrency/UnsafeLazyInitialization/UnsafeLazyInitialization.cs b/csharp/ql/test/query-tests/Concurrency/UnsafeLazyInitialization/UnsafeLazyInitialization.cs index 73c2c8fb485a..dea506b26d29 100644 --- a/csharp/ql/test/query-tests/Concurrency/UnsafeLazyInitialization/UnsafeLazyInitialization.cs +++ b/csharp/ql/test/query-tests/Concurrency/UnsafeLazyInitialization/UnsafeLazyInitialization.cs @@ -44,7 +44,7 @@ void Fn() { if (obj2 == null) { - obj2 = null; + obj2 = null; } } } diff --git a/csharp/ql/test/query-tests/Security Features/CWE-094/CodeInjection.cs b/csharp/ql/test/query-tests/Security Features/CWE-094/CodeInjection.cs index e5264ac153d8..87d24b21fdde 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-094/CodeInjection.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-094/CodeInjection.cs @@ -55,6 +55,6 @@ public bool IsReusable void OnButtonClicked() { // BAD: Use the Roslyn APIs to dynamically evaluate C# - CSharpScript.EvaluateAsync(box1.Text); + CSharpScript.EvaluateAsync(box1.Text); } } diff --git a/csharp/ql/test/query-tests/Security Features/CWE-094/CodeInjection.expected b/csharp/ql/test/query-tests/Security Features/CWE-094/CodeInjection.expected index a2b76dc0b9d9..e831e232c983 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-094/CodeInjection.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-094/CodeInjection.expected @@ -5,8 +5,8 @@ nodes | CodeInjection.cs:25:23:25:45 | access to property QueryString | | CodeInjection.cs:31:64:31:67 | access to local variable code | | CodeInjection.cs:42:36:42:39 | access to local variable code | -| CodeInjection.cs:58:33:58:41 | access to property Text | +| CodeInjection.cs:58:36:58:44 | access to property Text | #select | CodeInjection.cs:31:64:31:67 | access to local variable code | CodeInjection.cs:25:23:25:45 | access to property QueryString | CodeInjection.cs:31:64:31:67 | access to local variable code | $@ flows to here and is compiled as code. | CodeInjection.cs:25:23:25:45 | access to property QueryString | User-provided value | | CodeInjection.cs:42:36:42:39 | access to local variable code | CodeInjection.cs:25:23:25:45 | access to property QueryString | CodeInjection.cs:42:36:42:39 | access to local variable code | $@ flows to here and is compiled as code. | CodeInjection.cs:25:23:25:45 | access to property QueryString | User-provided value | -| CodeInjection.cs:58:33:58:41 | access to property Text | CodeInjection.cs:58:33:58:41 | access to property Text | CodeInjection.cs:58:33:58:41 | access to property Text | $@ flows to here and is compiled as code. | CodeInjection.cs:58:33:58:41 | access to property Text | User-provided value | +| CodeInjection.cs:58:36:58:44 | access to property Text | CodeInjection.cs:58:36:58:44 | access to property Text | CodeInjection.cs:58:36:58:44 | access to property Text | $@ flows to here and is compiled as code. | CodeInjection.cs:58:36:58:44 | access to property Text | User-provided value | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs index f29c67118ed6..a0408813a6a7 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs @@ -9,16 +9,16 @@ public class TaintedPathHandler : IHttpHandler public void ProcessRequest(HttpContext ctx) { String path = ctx.Request.QueryString["page"]; - + // BAD: Uncontrolled format string. String.Format(path, "Do not do this"); - + // BAD: Using an IFormatProvider. String.Format((IFormatProvider)null, path, "Do not do this"); // GOOD: Not the format string. String.Format("Do not do this", path); - + // GOOD: Not the format string. String.Format((IFormatProvider)null, "Do not do this", path); } @@ -27,7 +27,7 @@ public void ProcessRequest(HttpContext ctx) void OnButtonClicked() { - // BAD: Uncontrolled format string. - String.Format(box1.Text, "Do not do this"); + // BAD: Uncontrolled format string. + String.Format(box1.Text, "Do not do this"); } } diff --git a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected index e6ccd6e37a68..a927157534ff 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected @@ -8,11 +8,11 @@ nodes | UncontrolledFormatString.cs:17:46:17:49 | access to local variable path | | UncontrolledFormatString.cs:20:23:20:38 | "Do not do this" | | UncontrolledFormatString.cs:23:46:23:61 | "Do not do this" | -| UncontrolledFormatString.cs:31:20:31:28 | access to property Text | +| UncontrolledFormatString.cs:31:23:31:31 | access to property Text | | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString | | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format | #select | UncontrolledFormatString.cs:14:23:14:26 | access to local variable path | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString | UncontrolledFormatString.cs:14:23:14:26 | access to local variable path | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString | access to property QueryString | | UncontrolledFormatString.cs:17:46:17:49 | access to local variable path | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString | UncontrolledFormatString.cs:17:46:17:49 | access to local variable path | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString | access to property QueryString | -| UncontrolledFormatString.cs:31:20:31:28 | access to property Text | UncontrolledFormatString.cs:31:20:31:28 | access to property Text | UncontrolledFormatString.cs:31:20:31:28 | access to property Text | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:31:20:31:28 | access to property Text | access to property Text | +| UncontrolledFormatString.cs:31:23:31:31 | access to property Text | UncontrolledFormatString.cs:31:23:31:31 | access to property Text | UncontrolledFormatString.cs:31:23:31:31 | access to property Text | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:31:23:31:31 | access to property Text | access to property Text | | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format | $@ flows to here and is used as a format string. | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString | access to property QueryString | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs index 720354e9a032..dc0c689eefa5 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs @@ -7,7 +7,7 @@ public class HttpHandler : IHttpHandler public void ProcessRequest(HttpContext ctx) { string format = ctx.Request.QueryString["nameformat"]; - + // BAD: Uncontrolled format string. FormattedName = string.Format(format, Surname, Forenames); } diff --git a/csharp/ql/test/resources/stubs/JsonNET.cs b/csharp/ql/test/resources/stubs/JsonNET.cs index ce31b2c81eba..707ba3583147 100644 --- a/csharp/ql/test/resources/stubs/JsonNET.cs +++ b/csharp/ql/test/resources/stubs/JsonNET.cs @@ -43,7 +43,7 @@ public class JToken : IEnumerable, IEnumerable public static explicit operator string(JToken t) => null; - public IEnumerable SelectToken(string s) => null; + public IEnumerable SelectToken(string s) => null; } public class JObject : JToken diff --git a/csharp/ql/test/resources/stubs/System.Windows.cs b/csharp/ql/test/resources/stubs/System.Windows.cs index 875e60662a22..504a13b41ccc 100644 --- a/csharp/ql/test/resources/stubs/System.Windows.cs +++ b/csharp/ql/test/resources/stubs/System.Windows.cs @@ -31,7 +31,7 @@ class TextBox : TextBoxBase public char PasswordChar { get; set; } public bool UseSystemPasswordChar { get; set; } } - + class RichTextBox : TextBoxBase { public string Rtf => null;