refactor the CredentialsExpr to be a dataflow node

erik-krogh · erik-krogh · commit 0c4f08c84178 · 2022-09-05T16:11:54.000+02:00
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/AWS.qll b/javascript/ql/lib/semmle/javascript/frameworks/AWS.qll
@@ -8,19 +8,19 @@ module AWS {
   /**
    * Holds if the `i`th argument of `invk` is an object hash for `AWS.Config`.
    */
-  private predicate takesConfigurationObject(InvokeExpr invk, int i) {
+  private predicate takesConfigurationObject(DataFlow::InvokeNode invk, int i) {
     exists(DataFlow::ModuleImportNode mod | mod.getPath() = "aws-sdk" |
       // `AWS.config.update(nd)`
-      invk = mod.getAPropertyRead("config").getAMemberCall("update").asExpr() and
+      invk = mod.getAPropertyRead("config").getAMemberCall("update") and
       i = 0
       or
       exists(DataFlow::SourceNode cfg | cfg = mod.getAConstructorInvocation("Config") |
         // `new AWS.Config(nd)`
-        invk = cfg.asExpr() and
+        invk = cfg and
         i = 0
         or
         // `var config = new AWS.Config(...); config.update(nd);`
-        invk = cfg.getAMemberCall("update").asExpr() and
+        invk = cfg.getAMemberCall("update") and
         i = 0
       )
     )
@@ -29,13 +29,13 @@ module AWS {
   /**
    * An expression that is used as an AWS config value: `{ accessKeyId: <user>, secretAccessKey: <password>}`.
    */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
-      exists(string prop, InvokeExpr invk, int i |
+      exists(string prop, DataFlow::InvokeNode invk, int i |
         takesConfigurationObject(invk, i) and
-        invk.hasOptionArgument(i, prop, this)
+        this = invk.getOptionArgument(i, prop)
       |
         prop = "accessKeyId" and kind = "user name"
         or
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Azure.qll b/javascript/ql/lib/semmle/javascript/frameworks/Azure.qll
@@ -8,13 +8,14 @@ module Azure {
   /**
    * An expression that is used for authentication at Azure`.
    */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
-      exists(CallExpr mce, string methodName |
-        (methodName = "loginWithUsernamePassword" or methodName = "loginWithServicePrincipalSecret") and
-        mce = DataFlow::moduleMember("ms-rest-azure", methodName).getACall().asExpr()
+      exists(DataFlow::CallNode mce |
+        mce =
+          DataFlow::moduleMember("ms-rest-azure",
+            ["loginWithUsernamePassword", "loginWithServicePrincipalSecret"]).getACall()
       |
         this = mce.getArgument(0) and kind = "user name"
         or
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/lib/semmle/javascript/frameworks/ClientRequests.qll
@@ -270,16 +270,16 @@ module ClientRequest {
   }
 
   /** An expression that is used as a credential in a request. */
-  private class AuthorizationHeader extends CredentialsExpr {
+  private class AuthorizationHeader extends CredentialsNode {
     AuthorizationHeader() {
       exists(DataFlow::PropWrite write | write.getPropertyName().regexpMatch("(?i)authorization") |
-        this = write.getRhs().asExpr()
+        this = write.getRhs()
       )
       or
       exists(DataFlow::MethodCallNode call | call.getMethodName() = ["append", "set"] |
         call.getNumArgument() = 2 and
         call.getArgument(0).getStringValue().regexpMatch("(?i)authorization") and
-        this = call.getArgument(1).asExpr()
+        this = call.getArgument(1)
       )
     }
 
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Connect.qll b/javascript/ql/lib/semmle/javascript/frameworks/Connect.qll
@@ -101,12 +101,12 @@ module Connect {
   }
 
   /** An expression that is passed as `basicAuthConnect(<user>, <password>)`. */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
-      exists(CallExpr call |
-        call = DataFlow::moduleImport("basic-auth-connect").getAnInvocation().asExpr() and
+      exists(DataFlow::CallNode call |
+        call = DataFlow::moduleImport("basic-auth-connect").getAnInvocation() and
         call.getNumArgument() = 2
       |
         this = call.getArgument(0) and kind = "user name"
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Credentials.qll b/javascript/ql/lib/semmle/javascript/frameworks/Credentials.qll
@@ -6,23 +6,38 @@
 import javascript
 
 /**
+ * DEPRECATED: Use `CredentialsNode` instead.
  * An expression whose value is used to supply credentials such
  * as a user name, a password, or a key.
  */
-abstract class CredentialsExpr extends Expr {
+deprecated class CredentialsExpr extends Expr {
+  CredentialsNode node;
+
+  CredentialsExpr() { node.asExpr() = this }
+
+  /**
+   * Gets a description of the kind of credential this expression is used as,
+   * such as `"user name"`, `"password"`, `"key"`.
+   */
+  deprecated string getCredentialsKind() { result = node.getCredentialsKind() }
+}
+
+/**
+ * An expression whose value is used to supply credentials such
+ * as a user name, a password, or a key.
+ */
+abstract class CredentialsNode extends DataFlow::Node {
   /**
    * Gets a description of the kind of credential this expression is used as,
    * such as `"user name"`, `"password"`, `"key"`.
    */
   abstract string getCredentialsKind();
 }
 
-private class CredentialsFromModel extends CredentialsExpr {
+private class CredentialsFromModel extends CredentialsNode {
   string kind;
 
-  CredentialsFromModel() {
-    this = ModelOutput::getASinkNode("credentials[" + kind + "]").asSink().asExpr()
-  }
+  CredentialsFromModel() { this = ModelOutput::getASinkNode("credentials[" + kind + "]").asSink() }
 
   override string getCredentialsKind() { result = kind }
 }
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll b/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll
@@ -46,11 +46,9 @@ abstract class CryptographicKeyCreation extends DataFlow::Node {
 }
 
 /**
- * A key used in a cryptographic algorithm, viewed as a `CredentialsExpr`.
+ * A key used in a cryptographic algorithm, viewed as a `CredentialsNode`.
  */
-class CryptographicKeyCredentialsExpr extends CredentialsExpr {
-  CryptographicKeyCredentialsExpr() { this = any(CryptographicKey k).asExpr() }
-
+class CryptographicKeyCredentialsExpr extends CredentialsNode instanceof CryptographicKey {
   override string getCredentialsKind() { result = "key" }
 }
 
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/DigitalOcean.qll b/javascript/ql/lib/semmle/javascript/frameworks/DigitalOcean.qll
@@ -8,12 +8,12 @@ module DigitalOcean {
   /**
    * An expression that is used for authentication at DigitalOcean: `digitalocean.client(<token>)`.
    */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
-      exists(CallExpr mce |
-        mce = DataFlow::moduleMember("digitalocean", "client").getACall().asExpr()
+      exists(DataFlow::CallNode mce |
+        mce = DataFlow::moduleMember("digitalocean", "client").getACall()
       |
         this = mce.getArgument(0) and kind = "token"
       )
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Express.qll b/javascript/ql/lib/semmle/javascript/frameworks/Express.qll
@@ -995,7 +995,7 @@ module Express {
   }
 
   /** An expression that is passed as `expressBasicAuth({ users: { <user>: <password> }})`. */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
@@ -1006,9 +1006,9 @@ module Express {
           usersSrc.flowsTo(call.getOptionArgument(0, "users")) and
           usersSrc.flowsTo(pwn.getBase())
         |
-          this = pwn.getPropertyNameExpr() and kind = "user name"
+          this = pwn.getPropertyNameExpr().flow() and kind = "user name"
           or
-          this = pwn.getRhs().asExpr() and kind = "password"
+          this = pwn.getRhs() and kind = "password"
         )
       )
     }
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/JWT.qll b/javascript/ql/lib/semmle/javascript/frameworks/JWT.qll
@@ -40,12 +40,10 @@ private module JsonWebToken {
   }
 
   /**
-   * The private key for a JWT as a `CredentialsExpr`.
+   * The private key for a JWT as a `CredentialsNode`.
    */
-  private class JwtKey extends CredentialsExpr {
-    JwtKey() {
-      this = DataFlow::moduleMember("jsonwebtoken", "sign").getACall().getArgument(1).asExpr()
-    }
+  private class JwtKey extends CredentialsNode {
+    JwtKey() { this = DataFlow::moduleMember("jsonwebtoken", "sign").getACall().getArgument(1) }
 
     override string getCredentialsKind() { result = "key" }
   }
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll
@@ -496,13 +496,11 @@ private module Mongoose {
   /**
    * An expression passed to `mongoose.createConnection` to supply credentials.
    */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
-      exists(string prop |
-        this = createConnection().getParameter(3).getMember(prop).asSink().asExpr()
-      |
+      exists(string prop | this = createConnection().getParameter(3).getMember(prop).asSink() |
         prop = "user" and kind = "user name"
         or
         prop = "pass" and kind = "password"
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll
@@ -426,11 +426,10 @@ module NodeJSLib {
   }
 
   /** An expression that is passed as `http.request({ auth: <expr> }, ...)`. */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     Credentials() {
       exists(string http | http = "http" or http = "https" |
-        this =
-          DataFlow::moduleMember(http, "request").getACall().getOptionArgument(0, "auth").asExpr()
+        this = DataFlow::moduleMember(http, "request").getACall().getOptionArgument(0, "auth")
       )
     }
 
@@ -1038,11 +1037,9 @@ module NodeJSLib {
   /**
    * A data flow node that is the username passed to the login callback provided by an HTTP or HTTPS request made by a Node.js process, for example `username` in `http.request(url).on('login', (res, cb) => {cb(username, password)})`.
    */
-  private class ClientRequestLoginUsername extends CredentialsExpr {
+  private class ClientRequestLoginUsername extends CredentialsNode {
     ClientRequestLoginUsername() {
-      exists(ClientRequestLoginCallback callback |
-        this = callback.getACall().getArgument(0).asExpr()
-      )
+      exists(ClientRequestLoginCallback callback | this = callback.getACall().getArgument(0))
     }
 
     override string getCredentialsKind() { result = "Node.js http(s) client login username" }
@@ -1051,11 +1048,9 @@ module NodeJSLib {
   /**
    * A data flow node that is the password passed to the login callback provided by an HTTP or HTTPS request made by a Node.js process, for example `password` in `http.request(url).on('login', (res, cb) => {cb(username, password)})`.
    */
-  private class ClientRequestLoginPassword extends CredentialsExpr {
+  private class ClientRequestLoginPassword extends CredentialsNode {
     ClientRequestLoginPassword() {
-      exists(ClientRequestLoginCallback callback |
-        this = callback.getACall().getArgument(1).asExpr()
-      )
+      exists(ClientRequestLoginCallback callback | this = callback.getACall().getArgument(1))
     }
 
     override string getCredentialsKind() { result = "Node.js http(s) client login password" }
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/PkgCloud.qll b/javascript/ql/lib/semmle/javascript/frameworks/PkgCloud.qll
@@ -31,13 +31,13 @@ module PkgCloud {
   /**
    * An expression that is used for authentication through pkgcloud.
    */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
       exists(string propertyName, DataFlow::InvokeNode invk, int i |
         takesConfigurationObject(invk, i) and
-        this = invk.getOptionArgument(0, propertyName).asExpr()
+        this = invk.getOptionArgument(0, propertyName)
       |
         /*
          * Catch-all support for the following providers:
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Request.qll b/javascript/ql/lib/semmle/javascript/frameworks/Request.qll
@@ -6,7 +6,7 @@ import javascript
 
 module Request {
   /** A credentials expression that is used for authentication. */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
@@ -20,9 +20,9 @@ module Request {
           action = mod.getAMemberCall(any(HTTP::RequestMethodName n).toLowerCase())
         )
       |
-        exists(MethodCallExpr auth, int argIndex |
+        exists(DataFlow::MethodCallNode auth, int argIndex |
           // request.get(url).auth('username', 'password', _, 'token');
-          auth = action.getAMemberCall("auth").asExpr() and
+          auth = action.getAMemberCall("auth") and
           this = auth.getArgument(argIndex)
         |
           argIndex = 0 and kind = "user name"
@@ -35,7 +35,7 @@ module Request {
         exists(DataFlow::ObjectLiteralNode auth, string propertyName |
           // request.get(url, { auth: {user: 'username', pass: 'password', bearer: 'token'}})
           auth.flowsTo(action.getOptionArgument(1, "auth")) and
-          auth.hasPropertyWrite(propertyName, DataFlow::valueNode(this))
+          auth.hasPropertyWrite(propertyName, this)
         |
           (propertyName = "user" or propertyName = "username") and
           kind = "user name"
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/SQL.qll
@@ -103,13 +103,13 @@ private module MySql {
   }
 
   /** An expression that is passed as user name or password to `mysql.createConnection`. */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
       exists(API::Node callee, string prop |
         callee in [createConnection(), createPool()] and
-        this = callee.getParameter(0).getMember(prop).asSink().asExpr() and
+        this = callee.getParameter(0).getMember(prop).asSink() and
         (
           prop = "user" and kind = "user name"
           or
@@ -205,14 +205,14 @@ private module Postgres {
   }
 
   /** An expression that is passed as user name or password when creating a client or a pool. */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
       exists(string prop |
-        this = [newClient(), newPool()].getParameter(0).getMember(prop).asSink().asExpr()
+        this = [newClient(), newPool()].getParameter(0).getMember(prop).asSink()
         or
-        this = pgPromise().getParameter(0).getMember(prop).asSink().asExpr()
+        this = pgPromise().getParameter(0).getMember(prop).asSink()
       |
         prop = "user" and kind = "user name"
         or
@@ -485,7 +485,7 @@ private module MsSql {
   }
 
   /** An expression that is passed as user name or password when creating a client or a pool. */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
@@ -495,7 +495,7 @@ private module MsSql {
           or
           callee = mssql().getMember("ConnectionPool")
         ) and
-        this = callee.getParameter(0).getMember(prop).asSink().asExpr() and
+        this = callee.getParameter(0).getMember(prop).asSink() and
         (
           prop = "user" and kind = "user name"
           or
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/HardcodedCredentialsCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/HardcodedCredentialsCustomizations.qll
@@ -33,12 +33,10 @@ module HardcodedCredentials {
   }
 
   /**
-   * A subclass of `Sink` that includes every `CredentialsExpr`
+   * A subclass of `Sink` that includes every `CredentialsNode`
    * as a credentials sink.
    */
-  class DefaultCredentialsSink extends Sink, DataFlow::ValueNode {
-    override CredentialsExpr astNode;
-
-    override string getKind() { result = astNode.getCredentialsKind() }
+  class DefaultCredentialsSink extends Sink instanceof CredentialsNode {
+    override string getKind() { result = super.getCredentialsKind() }
   }
 }
diff --git a/javascript/ql/test/library-tests/frameworks/SQL/Credentials.ql b/javascript/ql/test/library-tests/frameworks/SQL/Credentials.ql
@@ -1,4 +1,4 @@
 import javascript
 
-from CredentialsExpr ce
+from CredentialsNode ce
 select ce, ce.getCredentialsKind()

Original file line number	Diff line number	Diff line change
`@@ -270,16 +270,16 @@ module ClientRequest {`
`270`	`270`	`}`
`271`	`271`
`272`	`272`	`/** An expression that is used as a credential in a request. */`
`273`		`- private class AuthorizationHeader extends CredentialsExpr {`
	`273`	`+ private class AuthorizationHeader extends CredentialsNode {`
`274`	`274`	`AuthorizationHeader() {`
`275`	`275`	`exists(DataFlow::PropWrite write \| write.getPropertyName().regexpMatch("(?i)authorization") \|`
`276`		`- this = write.getRhs().asExpr()`
	`276`	`+ this = write.getRhs()`
`277`	`277`	`)`
`278`	`278`	`or`
`279`	`279`	`exists(DataFlow::MethodCallNode call \| call.getMethodName() = ["append", "set"] \|`
`280`	`280`	`call.getNumArgument() = 2 and`
`281`	`281`	`call.getArgument(0).getStringValue().regexpMatch("(?i)authorization") and`
`282`		`- this = call.getArgument(1).asExpr()`
	`282`	`+ this = call.getArgument(1)`
`283`	`283`	`)`
`284`	`284`	`}`
`285`	`285`
Original file line number	Diff line number	Diff line change
`@@ -101,12 +101,12 @@ module Connect {`
`101`	`101`	`}`
`102`	`102`
`103`	`103`	/** An expression that is passed as `basicAuthConnect(<user>, <password>)`. */
`104`		`- class Credentials extends CredentialsExpr {`
	`104`	`+ class Credentials extends CredentialsNode {`
`105`	`105`	`string kind;`
`106`	`106`
`107`	`107`	`Credentials() {`
`108`		`- exists(CallExpr call \|`
`109`		`- call = DataFlow::moduleImport("basic-auth-connect").getAnInvocation().asExpr() and`
	`108`	`+ exists(DataFlow::CallNode call \|`
	`109`	`+ call = DataFlow::moduleImport("basic-auth-connect").getAnInvocation() and`
`110`	`110`	`call.getNumArgument() = 2`
`111`	`111`	`\|`
`112`	`112`	`this = call.getArgument(0) and kind = "user name"`
Original file line number	Diff line number	Diff line change
`@@ -46,11 +46,9 @@ abstract class CryptographicKeyCreation extends DataFlow::Node {`
`46`	`46`	`}`
`47`	`47`
`48`	`48`	`/**`
`49`		- * A key used in a cryptographic algorithm, viewed as a `CredentialsExpr`.
	`49`	+ * A key used in a cryptographic algorithm, viewed as a `CredentialsNode`.
`50`	`50`	`*/`
`51`		`-class CryptographicKeyCredentialsExpr extends CredentialsExpr {`
`52`		`- CryptographicKeyCredentialsExpr() { this = any(CryptographicKey k).asExpr() }`
`53`		`-`
	`51`	`+class CryptographicKeyCredentialsExpr extends CredentialsNode instanceof CryptographicKey {`
`54`	`52`	`override string getCredentialsKind() { result = "key" }`
`55`	`53`	`}`
`56`	`54`
Original file line number	Diff line number	Diff line change
`@@ -995,7 +995,7 @@ module Express {`
`995`	`995`	`}`
`996`	`996`
`997`	`997`	/** An expression that is passed as `expressBasicAuth({ users: { <user>: <password> }})`. */
`998`		`- class Credentials extends CredentialsExpr {`
	`998`	`+ class Credentials extends CredentialsNode {`
`999`	`999`	`string kind;`
`1000`	`1000`
`1001`	`1001`	`Credentials() {`
`@@ -1006,9 +1006,9 @@ module Express {`
`1006`	`1006`	`usersSrc.flowsTo(call.getOptionArgument(0, "users")) and`
`1007`	`1007`	`usersSrc.flowsTo(pwn.getBase())`
`1008`	`1008`	`\|`
`1009`		`- this = pwn.getPropertyNameExpr() and kind = "user name"`
	`1009`	`+ this = pwn.getPropertyNameExpr().flow() and kind = "user name"`
`1010`	`1010`	`or`
`1011`		`- this = pwn.getRhs().asExpr() and kind = "password"`
	`1011`	`+ this = pwn.getRhs() and kind = "password"`
`1012`	`1012`	`)`
`1013`	`1013`	`)`
`1014`	`1014`	`}`
Original file line number	Diff line number	Diff line change
`@@ -40,12 +40,10 @@ private module JsonWebToken {`
`40`	`40`	`}`
`41`	`41`
`42`	`42`	`/**`
`43`		- * The private key for a JWT as a `CredentialsExpr`.
	`43`	+ * The private key for a JWT as a `CredentialsNode`.
`44`	`44`	`*/`
`45`		`- private class JwtKey extends CredentialsExpr {`
`46`		`- JwtKey() {`
`47`		`- this = DataFlow::moduleMember("jsonwebtoken", "sign").getACall().getArgument(1).asExpr()`
`48`		`- }`
	`45`	`+ private class JwtKey extends CredentialsNode {`
	`46`	`+ JwtKey() { this = DataFlow::moduleMember("jsonwebtoken", "sign").getACall().getArgument(1) }`
`49`	`47`
`50`	`48`	`override string getCredentialsKind() { result = "key" }`
`51`	`49`	`}`