Merge pull request #73 from esben-semmle/js/cleartext-logging-query

Approved by xiemaisi

Author: semmle-qlci

change-notes/1.18/analysis-javascript.md

@@ -85,6 +85,7 @@
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
+| Clear-text logging of sensitive information (`js/clear-text-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. |

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

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>

javascript/ql/src/Security/CWE-312/CleartextLogging.ql

@@ -0,0 +1,38 @@
+/**
+ * @name Clear-text logging of sensitive information
+ * @description Logging sensitive information without encryption or hashing can
+ *              expose it to an attacker.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id js/clear-text-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()
+  )
+}
+
+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())
+select sink, "Sensitive data returned by $@ is logged here.", source, source.describe()

javascript/ql/src/Security/CWE-312/CleartextStorage.qhelp

@@ -15,13 +15,22 @@ which are stored on the machine of the end-user.
 <p>
 Ensure that sensitive information is always encrypted before being stored.
 If possible, avoid placing sensitive information in cookies altogether.
-Instead, prefer storing, in the cookie, a key that can be used to lookup the
+Instead, prefer storing, in the cookie, a key that can be used to look up the
 sensitive information.
 </p>
 <p>
 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>

javascript/ql/src/javascript.qll

@@ -59,6 +59,7 @@ import semmle.javascript.frameworks.DigitalOcean
 import semmle.javascript.frameworks.Electron
 import semmle.javascript.frameworks.jQuery
 import semmle.javascript.frameworks.LodashUnderscore
+import semmle.javascript.frameworks.Logging
 import semmle.javascript.frameworks.HttpFrameworks
 import semmle.javascript.frameworks.NoSQL
 import semmle.javascript.frameworks.PkgCloud

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

@@ -316,13 +316,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
       (
@@ -336,8 +335,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/frameworks/Logging.qll

@@ -0,0 +1,139 @@
+/**
+ * Provides classes for working with logging libraries.
+ */
+
+import javascript
+
+/**
+ * A call to a logging mechanism.
+ */
+abstract class LoggerCall extends DataFlow::CallNode {
+
+  /**
+   * Gets a node that contributes to the logged message.
+   */
+  abstract DataFlow::Node getAMessageComponent();
+
+}
+
+/**
+ * Gets a log level name that is used in RFC5424, `npm`, `console`.
+ */
+private string getAStandardLoggerMethodName() {
+  result = "crit" or
+  result = "debug" or
+  result = "error" or
+  result = "emerg" or
+  result = "fatal" or
+  result = "info" or
+  result = "log" or
+  result = "notice" or
+  result = "silly" or
+  result = "trace" or
+  result = "warn"
+}
+
+/**
+ * Provides classes for working the builtin Node.js/Browser `console`.
+ */
+private module Console {
+
+  /**
+   * Gets a data flow source node for the console library.
+   */
+  private DataFlow::SourceNode console() {
+    result = DataFlow::moduleImport("console") or
+    result = DataFlow::globalVarRef("console")
+  }
+
+  /**
+   * A call to the console logging mechanism.
+   */
+  class ConsoleLoggerCall extends LoggerCall {
+    string name;
+    
+    ConsoleLoggerCall() {
+      (
+        name = getAStandardLoggerMethodName() or
+        name = "assert"
+      ) and
+      this = console().getAMethodCall(name)
+    }
+
+    override DataFlow::Node getAMessageComponent() {
+      if name = "assert" then
+        result = getArgument([1..getNumArgument()])
+      else
+        result = getAnArgument()
+    }
+
+  }
+
+}
+
+/**
+ * Provides classes for working with [loglevel](https://github.com/pimterry/loglevel).
+ */
+private module Loglevel {
+
+  /**
+   * A call to the loglevel logging mechanism.
+   */
+  class LoglevelLoggerCall extends LoggerCall {
+    LoglevelLoggerCall() {
+      this = DataFlow::moduleMember("loglevel", getAStandardLoggerMethodName()).getACall()
+    }
+
+    override DataFlow::Node getAMessageComponent() {
+      result = getAnArgument()
+    }
+
+  }
+
+}
+
+
+/**
+ * Provides classes for working with [winston](https://github.com/winstonjs/winston).
+ */
+private module Winston {
+
+  /**
+   * A call to the winston logging mechanism.
+   */
+  class WinstonLoggerCall extends LoggerCall, DataFlow::MethodCallNode {
+    WinstonLoggerCall() {
+      this = DataFlow::moduleMember("winston", "createLogger").getACall().getAMethodCall(getAStandardLoggerMethodName())
+    }
+
+    override DataFlow::Node getAMessageComponent() {
+      if getMethodName() = "log" then
+        result = getOptionArgument(0, "message")
+      else
+        result = getAnArgument()
+    }
+
+  }
+
+}
+
+/**
+ * Provides classes for working with [log4js](https://github.com/log4js-node/log4js-node).
+ */
+private module log4js {
+
+  /**
+   * A call to the log4js logging mechanism.
+   */
+  class Log4jsLoggerCall extends LoggerCall {
+    Log4jsLoggerCall() {
+      this = DataFlow::moduleMember("log4js", "getLogger").getACall().getAMethodCall(getAStandardLoggerMethodName())
+    }
+
+    override DataFlow::Node getAMessageComponent() {
+      result = getAnArgument()
+    }
+
+  }
+
+}

javascript/ql/src/semmle/javascript/security/SensitiveActions.qll

@@ -11,27 +11,38 @@
 
 import javascript
 
-/** A regular expression that identifies strings that look like they represent secret data that are not passwords. */
-private string suspiciousNonPassword() {
-  result = "(?is).*(secret|account|accnt|(?<!un)trusted).*"
-}
-/** A regular expression that identifies strings that look like they represent secret data that are passwords. */
-private string suspiciousPassword() {
-  result = "(?is).*(password|passwd).*"
-}
-
-/** A regular expression that identifies strings that look like they represent secret data. */
-private string suspicious() {
-  result = suspiciousPassword() or result = suspiciousNonPassword()
-}
-
 /**
- * A string for `match` that identifies strings that look like they represent secret data that is
- * hashed or encrypted.
+ * Provides heuristics for identifying names related to sensitive information.
+ *
+ * INTERNAL: Do not use directly.
  */
-private string nonSuspicious() {
-  result = "(?is).*(hash|(?<!un)encrypted|\\bcrypt\\b).*"
+module HeuristicNames {
+
+  /** A regular expression that identifies strings that look like they represent secret data that are not passwords. */
+  string suspiciousNonPassword() {
+    result = "(?is).*(secret|account|accnt|(?<!un)trusted).*"
+  }
+
+  /** A regular expression that identifies strings that look like they represent secret data that are passwords. */
+  string suspiciousPassword() {
+    result = "(?is).*(password|passwd).*"
+  }
+
+  /** A regular expression that identifies strings that look like they represent secret data. */
+  string suspicious() {
+    result = suspiciousPassword() or result = suspiciousNonPassword()
+  }
+
+  /**
+   * A regular expression that identifies strings that look like they represent data that is
+   * hashed or encrypted.
+   */
+  string nonSuspicious() {
+    result = "(?is).*(redact|censor|obfuscate|hash|md5|sha|((?<!un)(en))?(crypt|code)).*"
+  }
+
 }
+private import HeuristicNames
 
 /** An expression that might contain sensitive data. */
 abstract class SensitiveExpr extends Expr {