Updates based on PR feedback. 1 pending change

Author: raulgarciamsft

csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll

@@ -2,20 +2,14 @@ import csharp
 import DataFlow
 
 /**
- * An abstract PropertyWrite for `TokenValidationParameters`.
- * Not really necessary anymore, but keeping it in case we want to extend the queries to check on other properties.
+ * A sensitive property for `TokenValidationParameters` that updates the underlying value.
  */
-abstract class TokenValidationParametersPropertyWrite extends PropertyWrite { }
-
-/**
- * An access to a sensitive property for `TokenValidationParameters` that updates the underlying value.
- */
-class TokenValidationParametersPropertyWriteToBypassSensitiveValidation extends TokenValidationParametersPropertyWrite {
-  TokenValidationParametersPropertyWriteToBypassSensitiveValidation() {
+class TokenValidationParametersPropertySensitiveValidation extends Property {
+  TokenValidationParametersPropertySensitiveValidation() {
     exists(Property p, Class c |
       c.hasQualifiedName("Microsoft.IdentityModel.Tokens.TokenValidationParameters")
     |
-      p.getAnAccess() = this and
+      p = this and
       c.getAProperty() = p and
       p.getName() in [
           "ValidateIssuer", "ValidateAudience", "ValidateLifetime", "RequireExpirationTime",
@@ -28,7 +22,7 @@ class TokenValidationParametersPropertyWriteToBypassSensitiveValidation extends
 /**
  * A dataflow from a `false` value to a write sensitive property for `TokenValidationParameters`.
  */
-class FalseValueFlowsToTokenValidationParametersPropertyWriteToBypassValidation extends TaintTracking::Configuration {
+class FalseValueFlowsToTokenValidationParametersPropertyWriteToBypassValidation extends DataFlow::Configuration {
   FalseValueFlowsToTokenValidationParametersPropertyWriteToBypassValidation() {
     this = "FlowsToTokenValidationResultIsValidCall"
   }
@@ -41,7 +35,7 @@ class FalseValueFlowsToTokenValidationParametersPropertyWriteToBypassValidation
   override predicate isSink(DataFlow::Node sink) {
     exists(Assignment a |
       sink.asExpr() = a.getRValue() and
-      a.getLValue() instanceof TokenValidationParametersPropertyWrite
+      a.getLValue().(PropertyAccess).getProperty() instanceof TokenValidationParametersPropertySensitiveValidation
     )
   }
 }
@@ -83,7 +77,7 @@ class JsonWebTokenHandlerValidateTokenCall extends MethodCall {
 /**
  * A read access for properties `IsValid` or `Exception` for `Microsoft.IdentityModel.JsonWebTokens.JsonWebTokenHandler.ValidateToken`
  */
-class TokenValidationResultIsValidCall extends PropertyRead {
+private class TokenValidationResultIsValidCall extends PropertyRead {
   TokenValidationResultIsValidCall() {
     exists(Property p | p.getAnAccess().(PropertyRead) = this |
       p.hasName("IsValid") or
@@ -95,7 +89,7 @@ class TokenValidationResultIsValidCall extends PropertyRead {
 /**
  * Dataflow from the output of `Microsoft.IdentityModel.JsonWebTokens.JsonWebTokenHandler.ValidateToken` call to access the `IsValid` or `Exception` property
  */
-private class FlowsToTokenValidationResultIsValidCall extends TaintTracking::Configuration {
+private class FlowsToTokenValidationResultIsValidCall extends DataFlow::Configuration {
   FlowsToTokenValidationResultIsValidCall() { this = "FlowsToTokenValidationResultIsValidCall" }
 
   override predicate isSource(DataFlow::Node source) {
@@ -108,25 +102,14 @@ private class FlowsToTokenValidationResultIsValidCall extends TaintTracking::Con
 }
 
 /**
- * Holds if the call to `Microsoft.IdentityModel.JsonWebTokens.JsonWebTokenHandler.ValidateToken` flows to any `IsValid` or `Exception` property access
+ * A security-sensitive property for `Microsoft.IdentityModel.Tokens.TokenValidationParameters`
  */
-predicate hasAFlowToTokenValidationResultIsValidCall(JsonWebTokenHandlerValidateTokenCall call) {
-  exists(FlowsToTokenValidationResultIsValidCall config, DataFlow::Node source |
-    call = source.asExpr()
-  |
-    config.hasFlow(source, _)
-  )
-}
-
-/**
- * A property write for security-sensitive properties for `Microsoft.IdentityModel.Tokens.TokenValidationParameters`
- */
-class TokenValidationParametersPropertyWriteToValidationDelegated extends PropertyWrite {
-  TokenValidationParametersPropertyWriteToValidationDelegated() {
+class TokenValidationParametersProperty extends Property {
+  TokenValidationParametersProperty() {
     exists(Property p, Class c |
       c.hasQualifiedName("Microsoft.IdentityModel.Tokens.TokenValidationParameters")
     |
-      p.getAnAccess() = this and
+      p = this and
       c.getAProperty() = p and
       p.getName() in [
           "SignatureValidator", "TokenReplayValidator", "AlgorithmValidator", "AudienceValidator",
@@ -170,7 +153,7 @@ class CallableAlwaysReturnsTrue extends Callable {
     or
     lambdaExprReturnsOnlyLiteralTrue(this)
     or
-    exists(AnonymousFunctionExpr le, Call call, CallableAlwaysReturnsTrue cat, Callable callable |
+    exists(AnonymousFunctionExpr le, Call call, Callable callable |
       this = le
     |
       callable.getACall() = call and
@@ -216,21 +199,6 @@ class CallableAlwaysReturnsTrueHigherPrecision extends CallableAlwaysReturnsTrue
   }
 }
 
-/**
- * A property Write for the `IssuerValidator` property for `Microsoft.IdentityModel.Tokens.TokenValidationParameters`
- */
-class TokenValidationParametersPropertyWriteToValidationDelegatedIssuerValidator extends PropertyWrite {
-  TokenValidationParametersPropertyWriteToValidationDelegatedIssuerValidator() {
-    exists(Property p, Class c |
-      c.hasQualifiedName("Microsoft.IdentityModel.Tokens.TokenValidationParameters")
-    |
-      p.getAnAccess() = this and
-      c.getAProperty() = p and
-      p.hasName("IssuerValidator")
-    )
-  }
-}
-
 /**
  * A callable that returns a `string` and has a `string` as 1st argument
  */

csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/delegated-security-validations-always-return-true.ql

@@ -16,8 +16,7 @@ import DataFlow
 import JsonWebTokenHandlerLib
 
 from
-  TokenValidationParametersPropertyWriteToValidationDelegated tv, Assignment a,
-  CallableAlwaysReturnsTrueHigherPrecision e
-where a.getLValue() = tv and a.getRValue().getAChild*() = e
-select tv, "JsonWebTokenHandler security-sensitive property $@ is being delegated to $@.", tv,
-  tv.getTarget().toString(), e, "a callable that always returns \"true\""
+  TokenValidationParametersProperty p , CallableAlwaysReturnsTrueHigherPrecision e
+where e = p.getAnAssignedValue()
+select e, "JsonWebTokenHandler security-sensitive property $@ is being delegated to $@.", p,
+  p.getQualifiedName().toString(), e, "a callable that always returns \"true\""

csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/security-validation-disabled.ql

@@ -16,9 +16,9 @@ import JsonWebTokenHandlerLib
 from
   FalseValueFlowsToTokenValidationParametersPropertyWriteToBypassValidation config,
   DataFlow::Node source, DataFlow::Node sink,
-  TokenValidationParametersPropertyWriteToBypassSensitiveValidation pw
+  TokenValidationParametersPropertySensitiveValidation pw
 where
   config.hasFlow(source, sink) and
-  exists(Assignment a | a.getLValue() = pw | sink.asExpr() = a.getRValue())
+  sink.asExpr() = pw.getAnAssignedValue()
 select sink, "The security sensitive property $@ is being disabled by the following value: $@.", pw,
-  pw.getTarget().toString(), source, "false"
+  pw.getQualifiedName().toString(), source, "false"

csharp/ql/test/experimental/Security Features/JsonWebTokenHandler/delegated-security-validations-always-return-true.expected

@@ -1,7 +1,7 @@
-| delegation-test.cs:101:13:101:59 | access to property LifetimeValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:101:13:101:59 | access to property LifetimeValidator | LifetimeValidator | delegation-test.cs:101:63:101:186 | (...) => ... | a callable that always returns "true" |
-| delegation-test.cs:102:13:102:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:102:13:102:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:102:63:102:178 | (...) => ... | a callable that always returns "true" |
-| delegation-test.cs:115:13:115:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:115:13:115:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:115:63:115:190 | (...) => ... | a callable that always returns "true" |
-| delegation-test.cs:116:13:116:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:116:13:116:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:116:63:116:180 | (...) => ... | a callable that always returns "true" |
-| delegation-test.cs:117:13:117:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:117:13:117:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:117:63:117:217 | (...) => ... | a callable that always returns "true" |
-| delegation-test.cs:118:13:118:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:118:13:118:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:118:63:118:248 | (...) => ... | a callable that always returns "true" |
-| delegation-test.cs:119:13:119:59 | access to property AudienceValidator | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | delegation-test.cs:119:13:119:59 | access to property AudienceValidator | AudienceValidator | delegation-test.cs:119:63:119:177 | (...) => ... | a callable that always returns "true" |
+| delegation-test.cs:101:63:101:186 | (...) => ... | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | stubs.cs:54:34:54:50 | LifetimeValidator | Microsoft.IdentityModel.Tokens.TokenValidationParameters.LifetimeValidator | delegation-test.cs:101:63:101:186 | (...) => ... | a callable that always returns "true" |
+| delegation-test.cs:102:63:102:178 | (...) => ... | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | stubs.cs:55:34:55:50 | AudienceValidator | Microsoft.IdentityModel.Tokens.TokenValidationParameters.AudienceValidator | delegation-test.cs:102:63:102:178 | (...) => ... | a callable that always returns "true" |
+| delegation-test.cs:115:63:115:190 | (...) => ... | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | stubs.cs:55:34:55:50 | AudienceValidator | Microsoft.IdentityModel.Tokens.TokenValidationParameters.AudienceValidator | delegation-test.cs:115:63:115:190 | (...) => ... | a callable that always returns "true" |
+| delegation-test.cs:116:63:116:180 | (...) => ... | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | stubs.cs:55:34:55:50 | AudienceValidator | Microsoft.IdentityModel.Tokens.TokenValidationParameters.AudienceValidator | delegation-test.cs:116:63:116:180 | (...) => ... | a callable that always returns "true" |
+| delegation-test.cs:117:63:117:217 | (...) => ... | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | stubs.cs:55:34:55:50 | AudienceValidator | Microsoft.IdentityModel.Tokens.TokenValidationParameters.AudienceValidator | delegation-test.cs:117:63:117:217 | (...) => ... | a callable that always returns "true" |
+| delegation-test.cs:118:63:118:248 | (...) => ... | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | stubs.cs:55:34:55:50 | AudienceValidator | Microsoft.IdentityModel.Tokens.TokenValidationParameters.AudienceValidator | delegation-test.cs:118:63:118:248 | (...) => ... | a callable that always returns "true" |
+| delegation-test.cs:119:63:119:177 | (...) => ... | JsonWebTokenHandler security-sensitive property $@ is being delegated to $@. | stubs.cs:55:34:55:50 | AudienceValidator | Microsoft.IdentityModel.Tokens.TokenValidationParameters.AudienceValidator | delegation-test.cs:119:63:119:177 | (...) => ... | a callable that always returns "true" |

csharp/ql/test/experimental/Security Features/JsonWebTokenHandler/security-validation-disabled.expected

@@ -1,5 +1,5 @@
-| security-validation-disabled-test.cs:31:34:31:38 | false | The security sensitive property $@ is being disabled by the following value: $@. | security-validation-disabled-test.cs:31:17:31:30 | access to property ValidateIssuer | ValidateIssuer | security-validation-disabled-test.cs:31:34:31:38 | false | false |
-| security-validation-disabled-test.cs:32:36:32:40 | false | The security sensitive property $@ is being disabled by the following value: $@. | security-validation-disabled-test.cs:32:17:32:32 | access to property ValidateAudience | ValidateAudience | security-validation-disabled-test.cs:32:36:32:40 | false | false |
-| security-validation-disabled-test.cs:33:36:33:40 | false | The security sensitive property $@ is being disabled by the following value: $@. | security-validation-disabled-test.cs:33:17:33:32 | access to property ValidateLifetime | ValidateLifetime | security-validation-disabled-test.cs:33:36:33:40 | false | false |
-| security-validation-disabled-test.cs:34:41:34:45 | false | The security sensitive property $@ is being disabled by the following value: $@. | security-validation-disabled-test.cs:34:17:34:37 | access to property RequireExpirationTime | RequireExpirationTime | security-validation-disabled-test.cs:34:41:34:45 | false | false |
-| security-validation-disabled-test.cs:37:35:37:39 | false | The security sensitive property $@ is being disabled by the following value: $@. | security-validation-disabled-test.cs:37:17:37:31 | access to property RequireAudience | RequireAudience | security-validation-disabled-test.cs:37:35:37:39 | false | false |
+| security-validation-disabled-test.cs:31:34:31:38 | false | The security sensitive property $@ is being disabled by the following value: $@. | stubs.cs:43:21:43:34 | ValidateIssuer | Microsoft.IdentityModel.Tokens.TokenValidationParameters.ValidateIssuer | security-validation-disabled-test.cs:31:34:31:38 | false | false |
+| security-validation-disabled-test.cs:32:36:32:40 | false | The security sensitive property $@ is being disabled by the following value: $@. | stubs.cs:44:21:44:36 | ValidateAudience | Microsoft.IdentityModel.Tokens.TokenValidationParameters.ValidateAudience | security-validation-disabled-test.cs:32:36:32:40 | false | false |
+| security-validation-disabled-test.cs:33:36:33:40 | false | The security sensitive property $@ is being disabled by the following value: $@. | stubs.cs:45:21:45:36 | ValidateLifetime | Microsoft.IdentityModel.Tokens.TokenValidationParameters.ValidateLifetime | security-validation-disabled-test.cs:33:36:33:40 | false | false |
+| security-validation-disabled-test.cs:34:41:34:45 | false | The security sensitive property $@ is being disabled by the following value: $@. | stubs.cs:51:21:51:41 | RequireExpirationTime | Microsoft.IdentityModel.Tokens.TokenValidationParameters.RequireExpirationTime | security-validation-disabled-test.cs:34:41:34:45 | false | false |
+| security-validation-disabled-test.cs:37:35:37:39 | false | The security sensitive property $@ is being disabled by the following value: $@. | stubs.cs:50:21:50:35 | RequireAudience | Microsoft.IdentityModel.Tokens.TokenValidationParameters.RequireAudience | security-validation-disabled-test.cs:37:35:37:39 | false | false |