Merge pull request #17459 from michaelnebel/csharp/accessormad

C#: Add MaD support for `Attribute.Getter` and `Attribute.Setter`.

csharp/ql/lib/change-notes/2024-09-16-accessor-mad.md

@@ -0,0 +1,4 @@
+---
+category: breaking
+---
+* C#: Add support for MaD directly on properties and indexers using *attributes*. Using `Attribute.Getter` or `Attribute.Setter` in the model `ext` field applies the model to the getter or setter for properties and indexers. Prior to this change `Attribute` models unintentionally worked for property setters (if the property is decorated with the matching attribute). That is, a model that uses the `Attribute` feature directly on a property for a property setter needs to be changed to `Attribute.Setter`.

csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll

@@ -28,11 +28,14 @@
  *    types can be short names or fully qualified names (mixing these two options
  *    is not allowed within a single signature).
  * 6. The `ext` column specifies additional API-graph-like edges. Currently
- *    there are only two valid values: "" and "Attribute". The empty string has no
- *    effect. "Attribute" applies if `name` and `signature` were left blank and
- *    acts by selecting an element that is attributed with the attribute type
- *    selected by the first 4 columns. This can be another member such as a field,
- *    property, method, or parameter.
+ *    there are only a few valid values: "", "Attribute", "Attribute.Getter" and "Attribute.Setter".
+ *    The empty string has no effect. "Attribute" applies if `name` and `signature` were left blank
+ *    and acts by selecting an element (except for properties and indexers) that is attributed with
+ *    the attribute type selected by the first 4 columns. This can be another member such as
+ *    a field, method, or parameter. "Attribute.Getter" and "Attribute.Setter" work similar to
+ *    "Attribute", except that they can only be applied to properties and indexers.
+ *    "Attribute.Setter" selects the setter element of a property/indexer and "Attribute.Getter"
+ *    selects the getter.
  * 7. The `input` column specifies how data enters the element selected by the
  *    first 6 columns, and the `output` column specifies how data leaves the
  *    element selected by the first 6 columns. For sinks, an `input` can be either "",
@@ -96,6 +99,7 @@ private import FlowSummaryImpl::Private::External
 private import semmle.code.csharp.commons.QualifiedName
 private import semmle.code.csharp.dispatch.OverridableCallable
 private import semmle.code.csharp.frameworks.System
+private import codeql.dataflow.internal.AccessPathSyntax as AccessPathSyntax
 private import codeql.mad.ModelValidation as SharedModelVal
 
 /**
@@ -194,8 +198,6 @@ predicate modelCoverage(string namespace, int namespaces, string kind, string pa
 
 /** Provides a query predicate to check the MaD models for validation errors. */
 module ModelValidation {
-  private import codeql.dataflow.internal.AccessPathSyntax as AccessPathSyntax
-
   private predicate getRelevantAccessPath(string path) {
     summaryModel(_, _, _, _, _, _, path, _, _, _, _) or
     summaryModel(_, _, _, _, _, _, _, path, _, _, _) or
@@ -289,7 +291,7 @@ module ModelValidation {
       not signature.regexpMatch("|\\([a-zA-Z0-9_<>\\.\\+\\*,\\[\\]]*\\)") and
       result = "Dubious signature \"" + signature + "\" in " + pred + " model."
       or
-      not ext.regexpMatch("|Attribute") and
+      not ext = ["", "Attribute", "Attribute.Getter", "Attribute.Setter"] and
       result = "Unrecognized extra API graph element \"" + ext + "\" in " + pred + " model."
       or
       invalidProvenance(provenance) and
@@ -406,6 +408,30 @@ Declaration interpretBaseDeclaration(string namespace, string type, string name,
   )
 }
 
+pragma[inline]
+private Declaration interpretExt(Declaration d, ExtPath ext) {
+  ext = "" and result = d
+  or
+  ext.getToken(0) = "Attribute" and
+  (
+    not exists(ext.getToken(1)) and
+    result.(Attributable).getAnAttribute().getType() = d and
+    not result instanceof Property and
+    not result instanceof Indexer
+    or
+    exists(string accessor | accessor = ext.getToken(1) |
+      result.(Accessor).getDeclaration().getAnAttribute().getType() = d and
+      (
+        result instanceof Getter and
+        accessor = "Getter"
+        or
+        result instanceof Setter and
+        accessor = "Setter"
+      )
+    )
+  )
+}
+
 /** Gets the source/sink/summary/neutral element corresponding to the supplied parameters. */
 pragma[nomagic]
 Declaration interpretElement(
@@ -425,12 +451,18 @@ Declaration interpretElement(
       )
     )
   |
-    ext = "" and result = d
-    or
-    ext = "Attribute" and result.(Attributable).getAnAttribute().getType() = d
+    result = interpretExt(d, ext)
   )
 }
 
+private predicate relevantExt(string ext) {
+  summaryModel(_, _, _, _, _, ext, _, _, _, _, _) or
+  sourceModel(_, _, _, _, _, ext, _, _, _, _) or
+  sinkModel(_, _, _, _, _, ext, _, _, _, _)
+}
+
+private class ExtPath = AccessPathSyntax::AccessPath<relevantExt/1>::AccessPath;
+
 private predicate parseSynthField(AccessPathToken c, string name) {
   c.getName() = "SyntheticField" and name = c.getAnArgument()
 }

csharp/ql/test/library-tests/dataflow/external-models/Sinks.cs

@@ -12,6 +12,12 @@ namespace My.Qltest
 
             object fieldWrite = new object();
             TaggedField = fieldWrite;
+
+            object propertyWrite = new object();
+            TaggedPropertySetter = propertyWrite;
+
+            object indexerWrite = new object();
+            this[0] = indexerWrite;
         }
 
         object SinkMethod()
@@ -34,7 +40,21 @@ namespace My.Qltest
 
         [SinkAttribute]
         object TaggedField;
+
+        [SinkPropertyAttribute]
+        object TaggedPropertySetter { get; set; }
+
+        [SinkIndexerAttribute]
+        object this[int index]
+        {
+            get { return null; }
+            set { }
+        }
     }
 
     class SinkAttribute : System.Attribute { }
-}
+
+    class SinkPropertyAttribute : System.Attribute { }
+
+    class SinkIndexerAttribute : System.Attribute { }
+}

csharp/ql/test/library-tests/dataflow/external-models/Sources.cs

@@ -18,14 +18,17 @@ namespace My.Qltest
             x = TaggedSrcField;
 
             x = SrcTwoArg("", "");
+
+            x = TaggedSrcPropertyGetter;
+            x = this[0];
         }
 
-        [SourceAttribute()]
+        [SourceAttribute]
         void Tagged1(object taggedMethodParam)
         {
         }
 
-        void Tagged2([SourceAttribute()] object taggedSrcParam)
+        void Tagged2([SourceAttribute] object taggedSrcParam)
         {
         }
 
@@ -49,14 +52,20 @@ namespace My.Qltest
 
         void SrcArg(object src) { }
 
-        [SourceAttribute()]
+        [SourceAttribute]
         object TaggedSrcMethod() { return null; }
 
-        [SourceAttribute()]
+        [SourceAttribute]
         object TaggedSrcField;
 
         object SrcTwoArg(string s1, string s2) { return null; }
+
+        [SourceAttribute]
+        object TaggedSrcPropertyGetter { get; }
+
+        [SourceAttribute]
+        object this[int i] => null;
     }
 
     class SourceAttribute : System.Attribute { }
-}
+}

csharp/ql/test/library-tests/dataflow/external-models/sinks.expected

@@ -4,5 +4,7 @@ invalidModelRow
 | Sinks.cs:11:13:11:41 | this access | file-content-store |
 | Sinks.cs:11:30:11:40 | access to local variable argToTagged | file-content-store |
 | Sinks.cs:14:27:14:36 | access to local variable fieldWrite | sql-injection |
-| Sinks.cs:20:20:20:22 | access to local variable res | js-injection |
-| Sinks.cs:27:20:27:25 | access to local variable resTag | html-injection |
+| Sinks.cs:17:36:17:48 | access to local variable propertyWrite | sql-injection |
+| Sinks.cs:20:23:20:34 | access to local variable indexerWrite | sql-injection |
+| Sinks.cs:26:20:26:22 | access to local variable res | js-injection |
+| Sinks.cs:33:20:33:25 | access to local variable resTag | html-injection |

csharp/ql/test/library-tests/dataflow/external-models/sinks.ext.yml

@@ -9,3 +9,5 @@ extensions:
       - ["My.Qltest", "SinkAttribute", false, "", "", "Attribute", "ReturnValue", "html-injection", "manual"]
       - ["My.Qltest", "SinkAttribute", false, "", "", "Attribute", "Argument", "file-content-store", "manual"]
       - ["My.Qltest", "SinkAttribute", false, "", "", "Attribute", "", "sql-injection", "manual"]
+      - ["My.Qltest", "SinkPropertyAttribute", false, "", "", "Attribute.Setter", "Argument[0]", "sql-injection", "manual"]
+      - ["My.Qltest", "SinkIndexerAttribute", false, "", "", "Attribute.Setter", "Argument[1]", "sql-injection", "manual"]

csharp/ql/test/library-tests/dataflow/external-models/srcs.expected

@@ -9,9 +9,11 @@ invalidModelRow
 | Sources.cs:17:17:17:33 | call to method TaggedSrcMethod | local |
 | Sources.cs:18:17:18:30 | access to field TaggedSrcField | local |
 | Sources.cs:20:17:20:33 | call to method SrcTwoArg | local |
-| Sources.cs:24:14:24:20 | this | local |
-| Sources.cs:24:29:24:45 | taggedMethodParam | local |
-| Sources.cs:28:49:28:62 | taggedSrcParam | local |
-| Sources.cs:40:45:40:45 | p | local |
-| Sources.cs:47:50:47:50 | p | local |
-| Sources.cs:53:16:53:30 | this | local |
+| Sources.cs:22:17:22:39 | access to property TaggedSrcPropertyGetter | local |
+| Sources.cs:23:17:23:23 | access to indexer | local |
+| Sources.cs:27:14:27:20 | this | local |
+| Sources.cs:27:29:27:45 | taggedMethodParam | local |
+| Sources.cs:31:47:31:60 | taggedSrcParam | local |
+| Sources.cs:43:45:43:45 | p | local |
+| Sources.cs:50:50:50:50 | p | local |
+| Sources.cs:56:16:56:30 | this | local |

csharp/ql/test/library-tests/dataflow/external-models/srcs.ext.yml

@@ -17,4 +17,5 @@ extensions:
       - ["My.Qltest", "SourceAttribute", false, "", "", "Attribute", "ReturnValue", "local", "manual"]
       - ["My.Qltest", "SourceAttribute", false, "", "", "Attribute", "Parameter", "local", "manual"]
       - ["My.Qltest", "SourceAttribute", false, "", "", "Attribute", "", "local", "manual"]
-      - ["My.Qltest", "A", false, "SrcTwoArg", "(System.String,System.String)", "", "ReturnValue", "local", "manual"]
+      - ["My.Qltest", "SourceAttribute", false, "", "", "Attribute.Getter", "ReturnValue", "local", "manual"]
+      - ["My.Qltest", "A", false, "SrcTwoArg", "(System.String,System.String)", "", "ReturnValue", "local", "manual"]