Merge pull request #891 from xiemaisi/js/simplify-sensitive-actions

Approved by esben-semmle

Author: semmle-qlci

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

@@ -35,8 +35,7 @@ causing logged sensitive information to be stored as well.
 
 <example>
 <p>
-The following example code stores user credentials (in this case, their account
-name) in a cookie in plain text:
+The following example code stores user credentials (in this case, their password) in a cookie in plain text:
 </p>
 <sample src="examples/CleartextStorage.js"/>
 <p>

javascript/ql/src/Security/CWE-312/examples/CleartextStorage.js

@@ -1,8 +1,8 @@
 var express = require('express');
 
 var app = express();
-app.get('/', function (req, res) {
-  let accountName = req.param("AccountName");
+app.get('/remember-password', function (req, res) {
+  let pw = req.param("current_password");
   // BAD: Setting a cookie value with cleartext sensitive data.
-  res.cookie("AccountName", accountName);
+  res.cookie("password", pw);
 });

javascript/ql/src/Security/CWE-312/examples/CleartextStorageGood.js

@@ -8,8 +8,8 @@ function encrypt(text){
 }
 
 var app = express();
-app.get('/', function (req, res) {
-  let accountName = req.param("AccountName");
+app.get('/remember-password', function (req, res) {
+  let pw = req.param("current_password");
   // GOOD: Encoding the value before setting it.
-  res.cookie("AccountName", encrypt(accountName));
+  res.cookie("password", encrypt(pw));
 });

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

@@ -17,31 +17,56 @@ import javascript
  * INTERNAL: Do not use directly.
  */
 module HeuristicNames {
-  /** Gets 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).*" }
+  /**
+   * Gets a regular expression that identifies strings that may indicate the presence of secret
+   * or trusted data.
+   */
+  string maybeSecret() { result = "(?is).*((?<!is)secret|(?<!un|is)trusted).*" }
 
-  /** Gets a regular expression that identifies strings that look like they represent secret data that are passwords. */
-  string suspiciousPassword() { result = "(?is).*(password|passwd).*" }
+  /**
+   * Gets a regular expression that identifies strings that may indicate the presence of
+   * user names or other account information.
+   */
+  string maybeAccountInfo() {
+    result = "(?is).*acc(ou)?nt.*" or
+    result = "(?is).*(puid|username|userid).*"
+  }
 
-  /** Gets a regular expression that identifies strings that look like they represent secret data. */
-  string suspicious() { result = suspiciousPassword() or result = suspiciousNonPassword() }
+  /**
+   * Gets a regular expression that identifies strings that may indicate the presence of
+   * a password or an authorization key.
+   */
+  string maybePassword() {
+    result = "(?is).*pass(wd|word|code|phrase)(?!.*question).*" or
+    result = "(?is).*(auth(entication|ori[sz]ation)?)key.*"
+  }
 
   /**
-   * Gets a regular expression that identifies strings that look like they represent data that is
-   * hashed or encrypted.
+   * Gets a regular expression that identifies strings that may indicate the presence of
+   * a certificate.
    */
-  string nonSuspicious() {
-    result = "(?is).*(redact|censor|obfuscate|hash|md5|sha|((?<!un)(en))?(crypt|code)).*"
+  string maybeCertificate() { result = "(?is).*(cert)(?!.*(format|name)).*" }
+
+  /**
+   * Gets a regular expression that identifies strings that may indicate the presence
+   * of sensitive data, with `classification` describing the kind of sensitive data involved.
+   */
+  string maybeSensitive(SensitiveExpr::Classification classification) {
+    result = maybeSecret() and classification = SensitiveExpr::secret()
+    or
+    result = maybeAccountInfo() and classification = SensitiveExpr::id()
+    or
+    result = maybePassword() and classification = SensitiveExpr::password()
+    or
+    result = maybeCertificate() and classification = SensitiveExpr::certificate()
   }
 
   /**
-   * Gets a regular expression that identifies names that look like they represent credential information.
+   * Gets a regular expression that identifies strings that may indicate the presence of data
+   * that is hashed or encrypted, and hence rendered non-sensitive.
    */
-  string suspiciousCredentials() {
-    result = "(?i).*pass(wd|word|code|phrase)(?!.*question).*" or
-    result = "(?i).*(puid|username|userid).*" or
-    result = "(?i).*(cert)(?!.*(format|name)).*" or
-    result = "(?i).*(auth(entication|ori[sz]ation)?)key.*"
+  string notSensitive() {
+    result = "(?is).*(redact|censor|obfuscate|hash|md5|sha|((?<!un)(en))?(crypt|code)).*"
   }
 }
 private import HeuristicNames
@@ -50,22 +75,60 @@ private import HeuristicNames
 abstract class SensitiveExpr extends Expr {
   /** Gets a human-readable description of this expression for use in alert messages. */
   abstract string describe();
+
+  /** Gets a classification of the kind of sensitive data this expression might contain. */
+  abstract SensitiveExpr::Classification getClassification();
+}
+
+module SensitiveExpr {
+  /**
+   * A classification of different kinds of sensitive data:
+   *
+   *   - secret: generic secret or trusted data;
+   *   - id: a user name or other account information;
+   *   - password: a password or authorization key;
+   *   - certificate: a certificate.
+   *
+   * While classifications are represented as strings, this should not be relied upon.
+   * Instead, use the predicates below to work with classifications.
+   */
+  class Classification extends string {
+    Classification() {
+      this = "secret" or this = "id" or this = "password" or this = "certificate"
+    }
+  }
+
+  /** Gets the classification for secret or trusted data. */
+  Classification secret() { result = "secret" }
+
+  /** Gets the classification for user names or other account information. */
+  Classification id() { result = "id" }
+
+  /** Gets the classification for passwords or authorization keys. */
+  Classification password() { result = "password" }
+
+  /** Gets the classification for certificates. */
+  Classification certificate() { result = "certificate" }
 }
 
 /** A function call that might produce sensitive data. */
 class SensitiveCall extends SensitiveExpr, InvokeExpr {
+  SensitiveExpr::Classification classification;
+
   SensitiveCall() {
-    this.getCalleeName() instanceof SensitiveDataFunctionName
+    classification = this.getCalleeName().(SensitiveDataFunctionName).getClassification()
     or
     // This is particularly to pick up methods with an argument like "password", which
     // may indicate a lookup.
     exists(string s | this.getAnArgument().mayHaveStringValue(s) |
-      s.regexpMatch(suspicious()) and
-      not s.regexpMatch(nonSuspicious())
+      s.regexpMatch(maybeSensitive(classification)) and
+      not s.regexpMatch(notSensitive())
     )
   }
 
   override string describe() { result = "a call to " + getCalleeName() }
+
+  override SensitiveExpr::Classification getClassification() { result = classification }
 }
 
 /** An access to a variable or property that might contain sensitive data. */
@@ -85,14 +148,17 @@ abstract class SensitiveVariableAccess extends SensitiveExpr {
 }
 
 /** A write to a location that might contain sensitive data. */
-abstract class SensitiveWrite extends DataFlow::Node { }
+abstract class SensitiveWrite extends DataFlow::Node {
+}
 
 /** A write to a variable or property that might contain sensitive data. */
 private class BasicSensitiveWrite extends SensitiveWrite {
+  SensitiveExpr::Classification classification;
+
   BasicSensitiveWrite() {
     exists(string name |
-      name.regexpMatch(suspicious()) and
-      not name.regexpMatch(nonSuspicious())
+      name.regexpMatch(maybeSensitive(classification)) and
+      not name.regexpMatch(notSensitive())
     |
       exists(DataFlow::PropWrite pwn |
         pwn.getPropertyName() = name and
@@ -110,13 +176,20 @@ private class BasicSensitiveWrite extends SensitiveWrite {
       )
     )
   }
+
+  /** Gets a classification of the kind of sensitive data the write might handle. */
+  SensitiveExpr::Classification getClassification() { result = classification }
 }
 
 /** An access to a variable or property that might contain sensitive data. */
 private class BasicSensitiveVariableAccess extends SensitiveVariableAccess {
+  SensitiveExpr::Classification classification;
+
   BasicSensitiveVariableAccess() {
-    name.regexpMatch(suspicious()) and not name.regexpMatch(nonSuspicious())
+    name.regexpMatch(maybeSensitive(classification)) and not name.regexpMatch(notSensitive())
   }
+
+  override SensitiveExpr::Classification getClassification() { result = classification }
 }
 
 /** A function name that suggests it may be sensitive. */
@@ -129,11 +202,18 @@ abstract class SensitiveFunctionName extends string {
 }
 
 /** A function name that suggests it may produce sensitive data. */
-abstract class SensitiveDataFunctionName extends SensitiveFunctionName { }
+abstract class SensitiveDataFunctionName extends SensitiveFunctionName {
+  /** Gets a classification of the kind of sensitive data this function may produce. */
+  abstract SensitiveExpr::Classification getClassification();
+}
 
 /** A method that might return sensitive data, based on the name. */
 class CredentialsFunctionName extends SensitiveDataFunctionName {
-  CredentialsFunctionName() { this.regexpMatch(suspicious()) }
+  SensitiveExpr::Classification classification;
+
+  CredentialsFunctionName() { this.regexpMatch(maybeSensitive(classification)) }
+
+  override SensitiveExpr::Classification getClassification() { result = classification }
 }
 
 /**
@@ -163,41 +243,10 @@ class ProtectCall extends DataFlow::CallNode {
   }
 }
 
-/**
- * Classes for expressions containing cleartext passwords.
- */
-private module CleartextPasswords {
-  bindingset[name]
-  private predicate isCleartextPasswordIndicator(string name) {
-    name.regexpMatch(suspiciousPassword()) and
-    not name.regexpMatch(nonSuspicious())
-  }
-
-  /** An expression that might contain a cleartext password. */
-  abstract class CleartextPasswordExpr extends SensitiveExpr { }
-
-  /** A function name that suggests it may produce a cleartext password. */
-  private class CleartextPasswordDataFunctionName extends SensitiveDataFunctionName {
-    CleartextPasswordDataFunctionName() { isCleartextPasswordIndicator(this) }
-  }
+/** An expression that might contain a clear-text password. */
+class CleartextPasswordExpr extends SensitiveExpr {
+  CleartextPasswordExpr() { this.(SensitiveExpr).getClassification() = SensitiveExpr::password() }
 
-  /** A call that might return a cleartext password. */
-  private class CleartextPasswordCallExpr extends CleartextPasswordExpr, SensitiveCall {
-    CleartextPasswordCallExpr() {
-      this.getCalleeName() instanceof CleartextPasswordDataFunctionName
-      or
-      // This is particularly to pick up methods with an argument like "password", which
-      // may indicate a lookup.
-      exists(string s |
-        this.getAnArgument().mayHaveStringValue(s) and
-        isCleartextPasswordIndicator(s)
-      )
-    }
-  }
-
-  /** An access to a variable or property that might contain a cleartext password. */
-  private class CleartextPasswordLookupExpr extends CleartextPasswordExpr, SensitiveVariableAccess {
-    CleartextPasswordLookupExpr() { isCleartextPasswordIndicator(name) }
-  }
+  override string describe() { none() }
+  override SensitiveExpr::Classification getClassification() { none() }
 }
-import CleartextPasswords

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

@@ -70,7 +70,7 @@ module CleartextLogging {
    */
   private class NameGuidedNonCleartextPassword extends NonCleartextPassword {
     NameGuidedNonCleartextPassword() {
-      exists(string name | name.regexpMatch(nonSuspicious()) |
+      exists(string name | name.regexpMatch(notSensitive()) |
         this.asExpr().(VarAccess).getName() = name
         or
         this.(DataFlow::PropRead).getPropertyName() = name
@@ -111,7 +111,7 @@ module CleartextLogging {
    * A call that might obfuscate a password, for example through hashing.
    */
   private class ObfuscatorCall extends Barrier, DataFlow::InvokeNode {
-    ObfuscatorCall() { getCalleeName().regexpMatch(nonSuspicious()) }
+    ObfuscatorCall() { getCalleeName().regexpMatch(notSensitive()) }
   }
 
   /**
@@ -129,8 +129,8 @@ module CleartextLogging {
 
     ObjectPasswordPropertySource() {
       exists(DataFlow::PropWrite write |
-        name.regexpMatch(suspiciousPassword()) and
-        not name.regexpMatch(nonSuspicious()) and
+        name.regexpMatch(maybePassword()) and
+        not name.regexpMatch(notSensitive()) and
         write = this.(DataFlow::SourceNode).getAPropertyWrite(name) and
         // avoid safe values assigned to presumably unsafe names
         not write.getRhs() instanceof NonCleartextPassword
@@ -147,7 +147,7 @@ module CleartextLogging {
     ReadPasswordSource() {
       // avoid safe values assigned to presumably unsafe names
       not this instanceof NonCleartextPassword and
-      name.regexpMatch(suspiciousPassword()) and
+      name.regexpMatch(maybePassword()) and
       (
         this.asExpr().(VarAccess).getName() = name
         or

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

@@ -50,6 +50,11 @@ module CleartextStorage {
   class SensitiveExprSource extends Source, DataFlow::ValueNode {
     override SensitiveExpr astNode;
 
+    SensitiveExprSource() {
+      // storing user names or account names in plaintext isn't usually a problem
+      astNode.getClassification() != SensitiveExpr::id()
+    }
+
     override string describe() { result = astNode.describe() }
   }
 

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

@@ -52,7 +52,9 @@ module InsufficientPasswordHash {
    * with insufficient computational effort.
    */
   class CleartextPasswordSource extends Source, DataFlow::ValueNode {
-    override CleartextPasswordExpr astNode;
+    override SensitiveExpr astNode;
+
+    CleartextPasswordSource() { astNode.getClassification() = SensitiveExpr::password() }
 
     override string describe() { result = astNode.describe() }
   }

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

@@ -4,7 +4,7 @@
  */
 
 import javascript
-private import semmle.javascript.security.SensitiveActions
+private import semmle.javascript.security.SensitiveActions::HeuristicNames
 
 module PostMessageStar {
   /**
@@ -70,7 +70,7 @@ module PostMessageStar {
       )
       or
       // `toString` or `JSON.toString` on a partially tainted object gives a tainted value
-      exists (DataFlow::InvokeNode toString | toString = trg |
+      exists(DataFlow::InvokeNode toString | toString = trg |
         toString.(DataFlow::MethodCallNode).calls(src, "toString")
         or
         toString = DataFlow::globalVarRef("JSON").getAMemberCall("stringify") and
@@ -92,23 +92,6 @@ module PostMessageStar {
    */
   class SensitiveExprSource extends Source, DataFlow::ValueNode { override SensitiveExpr astNode; }
 
-  /**
-   * A variable/property access or function call whose name suggests that it may contain credentials,
-   * viewed as a data flow source for cross-window communication with unrestricted origin.
-   */
-  class CredentialsSource extends Source {
-    CredentialsSource() {
-      exists(string name |
-        name = this.(DataFlow::InvokeNode).getCalleeName() or
-        name = this.(DataFlow::PropRead).getPropertyName() or
-        name = this.asExpr().(VarUse).getVariable().getName()
-      |
-        name.regexpMatch(HeuristicNames::suspiciousCredentials()) and
-        not name.regexpMatch(HeuristicNames::nonSuspicious())
-      )
-    }
-  }
-
   /** A call to any function whose name suggests that it encodes or encrypts its arguments. */
   class ProtectSanitizer extends Sanitizer { ProtectSanitizer() { this instanceof ProtectCall } }