C#: Also include property reads in possible new sink discovery. Only include public fields and properties.

Author: michaelnebel

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

@@ -1461,7 +1461,10 @@ private class InstanceFieldOrProperty extends FieldOrProperty {
   InstanceFieldOrProperty() { not this.isStatic() }
 }
 
-private class FieldOrPropertyAccess extends AssignableAccess, QualifiableExpr {
+/**
+ * An access to a field or a property.
+ */
+class FieldOrPropertyAccess extends AssignableAccess, QualifiableExpr {
   FieldOrPropertyAccess() { this.getTarget() instanceof FieldOrProperty }
 }
 

csharp/ql/src/utils/model-generator/internal/CaptureModelsSpecific.qll

@@ -38,8 +38,20 @@ predicate isRelevantSinkKind(string kind) { any() }
 class PropagateToSinkConfigurationSpecific extends TaintTracking::Configuration {
   PropagateToSinkConfigurationSpecific() { this = "parameters or fields flowing into sinks" }
 
+  private predicate isRelevantMemberAccess(DataFlow::Node node) {
+    exists(MemberAccess access | access = node.asExpr() |
+      access.hasThisQualifier() and
+      access.getTarget().isEffectivelyPublic() and
+      (
+        access instanceof FieldAccess
+        or
+        access.getTarget().(Property).getSetter().isPublic()
+      )
+    )
+  }
+
   override predicate isSource(DataFlow::Node source) {
-    (source.asExpr() instanceof FieldAccess or source instanceof DataFlow::ParameterNode) and
+    (isRelevantMemberAccess(source) or source instanceof DataFlow::ParameterNode) and
     source.getEnclosingCallable().(Modifiable).isEffectivelyPublic() and
     isRelevantForModels(source.getEnclosingCallable())
   }
@@ -54,7 +66,7 @@ string asInputArgument(DataFlow::Node source) {
     result = "Argument[" + pos + "]"
   )
   or
-  source.asExpr() instanceof FieldAccess and
+  source.asExpr() instanceof FieldOrPropertyAccess and
   result = qualifierString()
 }
 

csharp/ql/test/utils/model-generator/CaptureSinkModels.expected

@@ -1,3 +1,4 @@
 | Sinks;NewSinks;false;WrapFieldResponseWriteFile;();Argument[Qualifier];html |
+| Sinks;NewSinks;false;WrapPropResponseWriteFile;();Argument[Qualifier];html |
 | Sinks;NewSinks;false;WrapResponseWrite;(System.Object);Argument[0];html |
 | Sinks;NewSinks;false;WrapResponseWriteFile;(System.String);Argument[0];html |

csharp/ql/test/utils/model-generator/Sinks.cs

@@ -5,7 +5,12 @@ namespace Sinks;
 
 public class NewSinks
 {
-    private string tainted;
+    private string privateTainted;
+    public string tainted;
+
+    private string PrivateTaintedProp { get; set; }
+    public string TaintedProp { get; set; }
+    public string PrivateSetTaintedProp { get; private set; }
 
     // New sink
     public void WrapResponseWrite(object o)
@@ -35,4 +40,31 @@ public void WrapFieldResponseWriteFile()
         response.WriteFile(tainted);
     }
 
+    // NOT new sink as field is private
+    public void WrapPrivateFieldResponseWriteFile()
+    {
+        var response = new HttpResponse();
+        response.WriteFile(privateTainted);
+    }
+
+    // New sink
+    public void WrapPropResponseWriteFile()
+    {
+        var response = new HttpResponse();
+        response.WriteFile(TaintedProp);
+    }
+
+    // NOT new sink as property is private
+    public void WrapPrivatePropResponseWriteFile()
+    {
+        var response = new HttpResponse();
+        response.WriteFile(PrivateTaintedProp);
+    }
+
+    // NOT new sink as property setter is private
+    public void WrapPropPrivateSetResponseWriteFile()
+    {
+        var response = new HttpResponse();
+        response.WriteFile(PrivateSetTaintedProp);
+    }
 }