JS: add query js/cleartext-logging

2025-12-16 16:53:25 +01:00 · 2018-08-08 13:20:31 +02:00
parent b4952e7bfd
commit 0c4fb15651
19 changed files with 469 additions and 0 deletions
--- a/change-notes/1.18/analysis-javascript.md
+++ b/change-notes/1.18/analysis-javascript.md
@@ -28,6 +28,7 @@

 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
+| Clear text logging of sensitive information (`js/cleartext-logging`) | security, external/cwe/cwe-312, external/cwe/cwe-315, external/cwe/cwe-359 | Highlights logging of sensitive information, indicating a violation of [CWE-312](https://cwe.mitre.org/data/definitions/312.html). Results shown on lgtm by default. |
 | Disabling Electron webSecurity (`js/disabling-electron-websecurity`) | security, frameworks/electron | Highlights Electron browser objects that are created with the `webSecurity` property set to false. Results shown on LGTM by default. |
 | Enabling Electron allowRunningInsecureContent (`js/enabling-electron-insecure-content`) | security, frameworks/electron | Highlights Electron browser objects that are created with the `allowRunningInsecureContent` property set to true. Results shown on LGTM by default. |
 | Use of externally-controlled format string (`js/tainted-format-string`) | security, external/cwe/cwe-134 | Highlights format strings containing user-provided data, indicating a violation of [CWE-134](https://cwe.mitre.org/data/definitions/134.html). Results shown on LGTM by default. |
--- a/javascript/config/suites/javascript/security
+++ b/javascript/config/suites/javascript/security
@@ -9,6 +9,7 @@
 + semmlecode-javascript-queries/Security/CWE-134/TaintedFormatString.ql: /Security/CWE/CWE-134
 + semmlecode-javascript-queries/Security/CWE-209/StackTraceExposure.ql: /Security/CWE/CWE-209
 + semmlecode-javascript-queries/Security/CWE-312/CleartextStorage.ql: /Security/CWE/CWE-312
+ semmlecode-javascript-queries/Security/CWE-312/CleartextLogging.ql: /Security/CWE/CWE-312
 + semmlecode-javascript-queries/Security/CWE-313/PasswordInConfigurationFile.ql: /Security/CWE/CWE-313
 + semmlecode-javascript-queries/Security/CWE-327/BrokenCryptoAlgorithm.ql: /Security/CWE/CWE-327
 + semmlecode-javascript-queries/Security/CWE-338/InsecureRandomness.ql: /Security/CWE/CWE-338
--- a/javascript/ql/src/Security/CWE-312/CleartextLogging.qhelp
+++ b/javascript/ql/src/Security/CWE-312/CleartextLogging.qhelp
@@ -0,0 +1,5 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<include src="CleartextStorage.qhelp" /></qhelp>
--- a/javascript/ql/src/Security/CWE-312/CleartextLogging.ql
+++ b/javascript/ql/src/Security/CWE-312/CleartextLogging.ql
@@ -0,0 +1,55 @@
+/**
+ * @name Clear text logging of sensitive information
+ * @description Sensitive information logged without encryption or hashing can expose it to an
+ *              attacker.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id js/cleartext-logging
+ * @tags security
+ *       external/cwe/cwe-312
+ *       external/cwe/cwe-315
+ *       external/cwe/cwe-359
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.CleartextLogging::CleartextLogging
+
+/**
+ * Holds if `tl` is used in a browser environment.
+ */
+predicate inBrowserEnvironment(TopLevel tl) {
+  tl instanceof InlineScript or
+  tl instanceof CodeInAttribute or
+  exists (GlobalVarAccess e |
+    e.getTopLevel() = tl |
+    e.getName() = "window"
+  ) or
+  exists (Module m | inBrowserEnvironment(m) |
+    tl = m.getAnImportedModule() or
+    m = tl.(Module).getAnImportedModule()
+  )
+}
+
+/**
+ * Holds if `sink` only is reachable in a "test" environment.
+ */
+predicate inTestEnvironment(Sink sink) {
+  exists (IfStmt guard, Identifier id |
+    // heuristic: a deliberate environment choice by the programmer related to passwords implies a test environment
+    id.getName().regexpMatch("(?i).*(test|develop|production).*") and
+    id.(Expr).getParentExpr*() = guard.getCondition() and
+    (
+      guard.getAControlledStmt() = sink.asExpr().getEnclosingStmt() or
+      guard.getAControlledStmt().(BlockStmt).getAChildStmt() = sink.asExpr().getEnclosingStmt()
+    )
+  )
+}
+
+from Configuration cfg, Source source, DataFlow::Node sink
+where cfg.hasFlow(source, sink) and
+      // ignore logging to the browser console (even though it is not a good practice)
+      not inBrowserEnvironment(sink.asExpr().getTopLevel()) and
+      // ignore logging when testing
+      not inTestEnvironment(sink)
+select sink, "Sensitive data returned by $@ is logged here.", source, source.describe()
--- a/javascript/ql/src/Security/CWE-312/CleartextStorage.qhelp
+++ b/javascript/ql/src/Security/CWE-312/CleartextStorage.qhelp
@@ -22,6 +22,15 @@ sensitive information.
 In general, decrypt sensitive information only at the point where it is
 necessary for it to be used in cleartext.
 </p>
+
+<p>
+
+Be aware that external processes often store the <code>standard
+out</code> and <code>standard error</code> streams of the application,
+causing logged sensitive information to be stored as well.
+
+</p>
+
 </recommendation>

 <example>
--- a/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll
@@ -0,0 +1,224 @@
+/**
+ * Provides a dataflow tracking configuration for reasoning about cleartext logging of sensitive information.
+ */
+import javascript
+private import semmle.javascript.dataflow.InferredTypes
+private import semmle.javascript.security.SensitiveActions::HeuristicNames
+
+module CleartextLogging {
+  /**
+   * A data flow source for cleartext logging of sensitive information.
+   */
+  abstract class Source extends DataFlow::Node {
+    /** Gets a string that describes the type of this data flow source. */
+    abstract string describe();
+  }
+
+  /**
+   * A data flow sink for cleartext logging of sensitive information.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A barrier for cleartext logging of sensitive information.
+   */
+  abstract class Barrier extends DataFlow::Node { }
+
+  /**
+   * A dataflow tracking configuration for cleartext logging of sensitive information.
+   *
+   * This configuration identifies flows from `Source`s, which are sources of
+   * sensitive data, to `Sink`s, which is an abstract class representing all
+   * the places sensitive data may be stored in cleartext. Additional sources or sinks can be
+   * added either by extending the relevant class, or by subclassing this configuration itself,
+   * and amending the sources and sinks.
+   */
+  class Configuration extends DataFlow::Configuration {
+    Configuration() { this = "CleartextLogging" }
+
+    override
+    predicate isSource(DataFlow::Node source) {
+      source instanceof Source
+    }
+
+    override
+    predicate isSink(DataFlow::Node sink) {
+      sink instanceof Sink
+    }
+
+    override
+    predicate isBarrier(DataFlow::Node node) {
+      node instanceof Barrier
+    }
+
+    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"
+          )
+        )
+      )
+      or
+      // A taint propagating data flow edge through objects: a tainted write taints the entire object.
+      exists (DataFlow::PropWrite write |
+        write.getRhs() = src and
+        trg.(DataFlow::SourceNode).flowsTo(write.getBase())
+      )
+    }
+  }
+
+  /**
+   * An argument to a logging mechanism.
+   */
+  class LoggerSink extends Sink {
+    LoggerSink() {
+      this = any(LoggerCall log).getAMessageComponent()
+    }
+  }
+
+  /**
+   * A data flow node that does not contain a clear text password, according to its syntactic name.
+   */
+  private class NameGuidedNonCleartextPassword extends NonCleartextPassword {
+
+    NameGuidedNonCleartextPassword() {
+      exists (string name |
+        name.regexpMatch(nonSuspicious()) |
+        this.asExpr().(VarAccess).getName() = name
+        or
+        this.(DataFlow::PropRead).getPropertyName() = name
+        or
+        this.(DataFlow::InvokeNode).getCalleeName() = name
+      )
+      or
+      // avoid i18n strings
+      this.(DataFlow::PropRead).getBase().asExpr().(VarRef).getName().regexpMatch("(?is).*(messages|strings).*")
+    }
+
+  }
+
+  /**
+   * A data flow node that is definitely not an object.
+   */
+  private class NonObject extends NonCleartextPassword {
+
+    NonObject() {
+      forall (AbstractValue v | v = analyze().getAValue() |
+        not v.getType() = TTObject()
+      )
+    }
+
+  }
+
+  /**
+   * A data flow node that receives flow that is not a clear text password.
+   */
+  private class NonCleartextPasswordFlow extends NonCleartextPassword {
+
+    NonCleartextPasswordFlow() {
+      any(NonCleartextPassword other).(DataFlow::SourceNode).flowsTo(this)
+    }
+
+  }
+
+  /**
+   * A call that might obfuscate a password, for example through hashing.
+   */
+  private class ObfuscatorCall extends Barrier, DataFlow::InvokeNode {
+
+    ObfuscatorCall() {
+      getCalleeName().regexpMatch(nonSuspicious())
+    }
+
+  }
+
+  /**
+   * A data flow node that does not contain a clear text password.
+   */
+  private abstract class NonCleartextPassword extends DataFlow::Node { }
+
+  /**
+   * An object with a property that may contain password information
+   *
+   * This is a source since `toString()` on this object will show the property value.
+   */
+  private class ObjectPasswordPropertySource extends DataFlow::ValueNode, Source {
+    string name;
+
+    ObjectPasswordPropertySource() {
+      exists (DataFlow::PropWrite write |
+        write.getPropertyName() = name and
+        name.regexpMatch(suspiciousPassword()) and
+        not name.regexpMatch(nonSuspicious()) and
+        this.(DataFlow::SourceNode).flowsTo(write.getBase()) and
+        // avoid safe values assigned to presumably unsafe names
+        not write.getRhs() instanceof NonCleartextPassword
+      )
+    }
+
+    override string describe() {
+      result = "an access to " + name
+    }
+  }
+
+  /** An access to a variable or property that might contain a password. */
+  private class ReadPasswordSource extends DataFlow::ValueNode, Source {
+    string name;
+
+    ReadPasswordSource() {
+      // avoid safe values assigned to presumably unsafe names
+      not this instanceof NonCleartextPassword and
+      name.regexpMatch(suspiciousPassword()) and
+      (
+        this.asExpr().(VarAccess).getName() = name
+        or
+        exists (DataFlow::PropRead read, DataFlow::Node base |
+          this = read and
+          base = read.getBase() and
+          read.getPropertyName() = name and
+          // avoid safe values assigned to presumably unsafe names
+          exists (DataFlow::SourceNode baseObj | baseObj.flowsTo(base) |
+            not baseObj.getAPropertyWrite(name).getRhs() instanceof NonCleartextPassword
+          )
+        )
+      )
+    }
+
+    override string describe() {
+      result = "an access to " + name
+    }
+  }
+
+  /** A call that might return a password. */
+  private class CallPasswordSource extends DataFlow::ValueNode, DataFlow::InvokeNode, Source {
+    string name;
+
+    CallPasswordSource() {
+      name = getCalleeName() and
+      name.regexpMatch("(?is)getPassword")
+    }
+
+    override string describe() {
+      result = "a call to " + name
+    }
+  }
+
+}
--- a/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected
@@ -0,0 +1,19 @@
+| passwords.js:2:17:2:24 | password | Sensitive data returned by $@ is logged here. | passwords.js:2:17:2:24 | password | an access to password |
+| passwords.js:3:17:3:26 | o.password | Sensitive data returned by $@ is logged here. | passwords.js:3:17:3:26 | o.password | an access to password |
+| passwords.js:4:17:4:29 | getPassword() | Sensitive data returned by $@ is logged here. | passwords.js:4:17:4:29 | getPassword() | a call to getPassword |
+| passwords.js:5:17:5:31 | o.getPassword() | Sensitive data returned by $@ is logged here. | passwords.js:5:17:5:31 | o.getPassword() | a call to getPassword |
+| passwords.js:8:21:8:21 | x | Sensitive data returned by $@ is logged here. | passwords.js:10:11:10:18 | password | an access to password |
+| passwords.js:12:18:12:25 | password | Sensitive data returned by $@ is logged here. | passwords.js:12:18:12:25 | password | an access to password |
+| passwords.js:14:17:14:38 | name +  ... assword | Sensitive data returned by $@ is logged here. | passwords.js:14:31:14:38 | password | an access to password |
+| passwords.js:16:17:16:38 | `${name ... sword}` | Sensitive data returned by $@ is logged here. | passwords.js:16:29:16:36 | password | an access to password |
+| passwords.js:21:17:21:20 | obj1 | Sensitive data returned by $@ is logged here. | passwords.js:18:16:20:5 | {\\n      ... x\\n    } | an access to password |
+| passwords.js:26:17:26:20 | obj2 | Sensitive data returned by $@ is logged here. | passwords.js:24:12:24:19 | password | an access to password |
+| passwords.js:29:17:29:20 | obj3 | Sensitive data returned by $@ is logged here. | passwords.js:30:14:30:21 | password | an access to password |
+| 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_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 |
+| passwords_in_server_4.js:2:13:2:20 | password | Sensitive data returned by $@ is logged here. | passwords_in_server_4.js:2:13:2:20 | password | an access to password |
+| passwords_in_server_5.js:8:17:8:17 | x | Sensitive data returned by $@ is logged here. | passwords_in_server_5.js:4:7:4:24 | req.query.password | an access to password |
--- a/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.qlref
+++ b/javascript/ql/test/query-tests/Security/CWE-312/CleartextLogging.qlref
@@ -0,0 +1 @@
+Security/CWE-312/CleartextLogging.ql
--- a/javascript/ql/test/query-tests/Security/CWE-312/browser.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/browser.js
@@ -0,0 +1,2 @@
+import foo from "foo";
+window.location;
--- a/javascript/ql/test/query-tests/Security/CWE-312/passwords.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords.js
@@ -0,0 +1,122 @@
+(function() {
+    console.log(password); // NOT OK
+    console.log(o.password); // NOT OK
+    console.log(getPassword()); // NOT OK
+    console.log(o.getPassword()); // NOT OK
+
+    function myLog(x) {
+        console.log(x); // NOT OK
+    }
+    myLog(password);
+
+    console.info(password); // NOT OK
+
+    console.log(name + ", " + password); // NOT OK
+
+    console.log(`${name}, ${password}`); // NOT OK
+
+    var obj1 = {
+        password: x
+    };
+    console.log(obj1); // NOT OK
+
+    var obj2 = {
+        x: password
+    };
+    console.log(obj2); // NOT OK
+
+    var obj3 = {};
+    console.log(obj3);
+    obj3.x = password; // NOT OK
+
+    var fixed_password = "123";
+    console.log(fixed_password); // OK
+
+    console.log(messages.IncorrectPasswordError); // OK
+
+    console.log(this.hashed_password); // OK
+    console.log(login.wrappedJSObject.encryptedPassword); // OK
+    console.log(HTML5QQ.encodedPassword); // OK
+
+    console.log({password: crypt(pw)}); // OK
+    var actually_secure_password = crypt(password); // OK
+    console.log(actually_secure_password); // OK
+
+    var user1 = {};
+    user1.crypted_password = x();
+    console.log(user1); // OK
+
+    var user2 = {};
+    user2.password = hash();
+    console.log(user2); // OK
+
+    var user3 = {
+        password: encryptLib.encryptPassword(req.body.password)
+    };
+    console.log(user3); // OK
+
+    var actually_ok_password_1 = hashed1();
+    console.log(actually_ok_password_1);
+    var hashed2 = actually_ok_password_2();
+    console.log(hashed2); // OK
+
+    console.log(this._passwordMD5); // OK
+    console.log(this._password_sha); // OK
+
+    var utilityObject = {
+        passwordSet: function(){}
+    }
+    console.log(utilityObject); // OK
+
+    var displayedPassword = password? null: "(hidden)";
+    var config = {
+        password: displayedPassword
+    };
+    console.log(config); // OK
+
+    var temp = { encryptedPassword: req.body.password };
+    console.log(temp.encryptedPassword); // OK XXX
+
+    var secret = password;
+    console.log(`pw: ${secret}`); // NOT OK
+
+    function redact(kind, value) {
+        if (value && kind == 'password') {
+            return "********"
+        }
+        return value;
+    }
+
+    console.log("Password is: " + redact('password', password));
+
+    if (environment.isTestEnv()) {
+        console.log("Password is: " + password); // OK
+    }
+
+    if (environment.is(TEST)) {
+        // NB: for security reasons, we only log passwords in test environments
+        console.log("Password is: " + password); // OK
+    }
+
+
+    if (x.test(y)) {
+        f();
+        // ...
+        console.log("Password is: " + password); // NOT OK, but not flagged
+        // ...
+    }
+
+    if (environment.isTestEnv())
+        console.log("Password is: " + password); // OK
+
+    if (x.test(y)) {
+        if (f()) {
+            console.log("Password is: " + password); // NOT OK
+        }
+    }
+
+    if (!environment.isProduction()) {
+        console.log("Password is: " + password); // OK
+    }
+
+});
--- a/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_browser1.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_browser1.js
@@ -0,0 +1,2 @@
+window.location;
+console.log(password);
--- a/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_browser2.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_browser2.js
@@ -0,0 +1,2 @@
+import browser from "./browser";
+console.log(password);
--- a/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_server_1.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_server_1.js
@@ -0,0 +1,6 @@
+var express = require('express');
+var app = express();
+app.get('/some/path', function() {
+})
+
+console.log(password);
--- a/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_server_2.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_server_2.js
@@ -0,0 +1,3 @@
+require("foo");
+(function (req, res){});
+console.log(password);
--- a/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_server_3.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_server_3.js
@@ -0,0 +1,2 @@
+var server = require("./server");
+console.log(password);
--- a/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_server_4.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_server_4.js
@@ -0,0 +1,2 @@
+require("foo");
+console.log(password);
--- a/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_server_5.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/passwords_in_server_5.js
@@ -0,0 +1,9 @@
+var express = require('express');
+var app = express();
+app.get('/some/path', function() {
+    f(req.query.password);
+})
+
+function f(x) {
+    console.log(x);
+}
--- a/javascript/ql/test/query-tests/Security/CWE-312/server.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/server.js
@@ -0,0 +1,2 @@
+require("foo");
+(function (req, res){})
--- a/javascript/ql/test/query-tests/Security/CWE-312/server2.js
+++ b/javascript/ql/test/query-tests/Security/CWE-312/server2.js
@@ -0,0 +1,2 @@
+require("./server.js")
+require("./passwords_in_server_4.js")