JS: extract and expose StringConcatenationTaintStep in TaintTracking

Author: Esben Sparre Andreasen

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -309,13 +309,12 @@ module TaintTracking {
   }
 
   /**
-   * A taint propagating data flow edge arising from string append and other string
-   * operations defined in the standard library.
+   * A taint propagating data flow edge arising from string concatenations.
    *
-   * Note that since we cannot easily distinguish string append from addition, we consider
-   * any `+` operation to propagate taint.
+   * Note that since we cannot easily distinguish string append from addition,
+   * we consider any `+` operation to propagate taint.
    */
-  private class StringManipulationTaintStep extends AdditionalTaintStep, DataFlow::ValueNode {
+  class StringConcatenationTaintStep extends AdditionalTaintStep, DataFlow::ValueNode {
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
       succ = this and
       (
@@ -329,8 +328,19 @@ module TaintTracking {
         or
         // templating propagates taint
         astNode.(TemplateLiteral).getAnElement() = pred.asExpr()
-        or
-        // other string operations that propagate taint
+      )
+    }
+  }
+  
+  /**
+   * A taint propagating data flow edge arising from string manipulation 
+   * functions defined in the standard library.
+   */
+  private class StringManipulationTaintStep extends AdditionalTaintStep, DataFlow::ValueNode {
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      succ = this and
+      (
+        // string operations that propagate taint
         exists (string name | name = astNode.(MethodCallExpr).getMethodName() |
           pred.asExpr() = astNode.(MethodCallExpr).getReceiver() and
           ( // sorted, interesting, properties of String.prototype

javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll

@@ -52,30 +52,11 @@ module CleartextLogging {
     }
 
     override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg) {
-      // A taint propagating data flow edge arising from string operations
-      exists (AST::ValueNode astNode |
-        astNode = trg.(DataFlow::ValueNode).getAstNode() |
-        // addition propagates
-        astNode.(AddExpr).getAnOperand() = src.asExpr() or
-        astNode.(AssignAddExpr).getAChildExpr() = src.asExpr() or
-        exists (SsaExplicitDefinition ssa |
-          astNode = ssa.getVariable().getAUse() and
-          src.asExpr().(AssignAddExpr) = ssa.getDef()
-        )
-        or
-        // templating propagates
-        astNode.(TemplateLiteral).getAnElement() = src.asExpr()
-        or
-        // other string operations that propagate
-        exists (string name | name = astNode.(MethodCallExpr).getMethodName() |
-          src.asExpr() = astNode.(MethodCallExpr).getReceiver() and
-          (
-            // sorted, interesting, properties of Object.prototype
-            name = "toString" or
-            name = "valueOf"
-          )
-        )
-      )
+      any (TaintTracking::StringConcatenationTaintStep s).step(src, trg)
+      or
+      exists (string name | name = "toString" or name = "valueOf" |
+        src.(DataFlow::SourceNode).getAMethodCall(name) = trg
+      ) 
       or
       // A taint propagating data flow edge through objects: a tainted write taints the entire object.
       exists (DataFlow::PropWrite write |

javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected

@@ -12,6 +12,8 @@
 | passwords.js:78:17:78:38 | temp.en ... assword | Sensitive data returned by $@ is logged here. | passwords.js:77:37:77:53 | req.body.password | an access to password |
 | passwords.js:81:17:81:31 | `pw: ${secret}` | Sensitive data returned by $@ is logged here. | passwords.js:80:18:80:25 | password | an access to password |
 | passwords.js:114:25:114:50 | "Passwo ... assword | Sensitive data returned by $@ is logged here. | passwords.js:114:43:114:50 | password | an access to password |
+| passwords.js:122:17:122:49 | name +  ... tring() | Sensitive data returned by $@ is logged here. | passwords.js:122:31:122:38 | password | an access to password |
+| passwords.js:123:17:123:48 | name +  ... lueOf() | Sensitive data returned by $@ is logged here. | passwords.js:123:31:123:38 | password | an access to password |
 | passwords_in_server_1.js:6:13:6:20 | password | Sensitive data returned by $@ is logged here. | passwords_in_server_1.js:6:13:6:20 | password | an access to password |
 | passwords_in_server_2.js:3:13:3:20 | password | Sensitive data returned by $@ is logged here. | passwords_in_server_2.js:3:13:3:20 | password | an access to password |
 | passwords_in_server_3.js:2:13:2:20 | password | Sensitive data returned by $@ is logged here. | passwords_in_server_3.js:2:13:2:20 | password | an access to password |

javascript/ql/test/query-tests/Security/CWE-312/passwords.js

@@ -119,4 +119,6 @@
         console.log("Password is: " + password); // OK
     }
 
+    console.log(name + ", " + password.toString()); // NOT OK
+    console.log(name + ", " + password.valueOf()); // NOT OK
 });