Merge master into next.

Author: adityasharad

change-notes/1.20/analysis-javascript.md

@@ -4,7 +4,11 @@
 
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features:
   - client-side code, for example [React](https://reactjs.org/)
+  - cookies and webstorage, for example [js-cookie](https://github.com/js-cookie/js-cookie)
   - server-side code, for example [hapi](https://hapijs.com/)
+* File classification has been improved to recognize additional generated files, for example files from [HTML Tidy](html-tidy.org).
+
+* The taint tracking library now recognizes flow through persistent storage, this may give more results for the security queries. 
 
 ## New queries
 
@@ -20,6 +24,7 @@
 | **Query**                                  | **Expected impact**          | **Change**                                                                   |
 |--------------------------------------------|------------------------------|------------------------------------------------------------------------------|
 | Client-side cross-site scripting           | More results                 | This rule now recognizes WinJS functions that are vulnerable to HTML injection. |
+| Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. |
 | Unused parameter                           | Fewer false-positive results | This rule no longer flags parameters with leading underscore. |
 | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. |
 

cpp/ql/src/Metrics/queries.xml

@@ -34,11 +34,5 @@ private predicate readsEnvironment(Expr read, string sourceDescription) {
     read = call and
     call.getTarget().hasGlobalName(name) and
     (name = "getenv" or name = "secure_getenv" or name = "_wgetenv") and
-    sourceDescription = name) or
-  exists(MessageExpr getObjectKey, MessageExpr getEnviron |
-    read = getObjectKey and
-    getObjectKey.getTarget().getQualifiedName().matches("NSDictionary%::-objectForKey:") and
-    getObjectKey.getQualifier() = getEnviron and
-    getEnviron.getTarget().getQualifiedName().matches("NSProcessInfo%:-environment") and
-    sourceDescription = "NSProcessInfo")
+    sourceDescription = name)
 }

cpp/ql/src/semmle/code/cpp/commons/Environment.qll

@@ -203,18 +203,5 @@ predicate shellCommand(Expr command, string callChain) {
     and arrayInitializer.getChild(idx) = command
     and shellCommandPreface(commandInterpreter.getValue(), flag.getValue())
     and idx > 1)
-      
-  // Creation of NSTask
-  or exists(
-    MessageExpr launchedTaskCall, TextLiteral commandInterpreter,
-    Expr arrayLiteral, TextLiteral flag
-  |
-    launchedTaskCall.getStaticTarget().getQualifiedName().matches("NSTask%::+launchedTaskWithLaunchPath:arguments:")
-    and commandInterpreter = launchedTaskCall.getArgument(0)
-    and arrayLiteral = launchedTaskCall.getArgument(1)
-    and arrayElement(arrayLiteral, 0, flag)
-    and arrayElement(arrayLiteral, 1, command)
-    and shellCommandPreface(commandInterpreter.getValue(), flag.getValue())
-    and callChain = "NSTask")
 }
 

cpp/ql/src/semmle/code/cpp/security/CommandExecution.qll

@@ -35,25 +35,3 @@ class SensitiveCall extends SensitiveExpr {
     )
   } 
 }
-
-class SensitivePropAccess extends SensitiveExpr {
-  SensitivePropAccess() {
-    exists (PropertyAccess acc, string name |
-      acc = this and
-      name = acc.getProperty().getName().toLowerCase() and
-      name.matches(suspicious()) and
-      not name.matches(nonSuspicious()))
-  }
-}
-
-/**
- * A read from the value of a text widget.
- */
-class SensitiveTextRead extends SensitiveExpr {
-  SensitiveTextRead() {
-    exists (PropertyAccess facc |
-      facc = this and
-      facc.getReceiver() instanceof SensitiveExpr and
-      facc.getProperty().getName() = "text")
-  }
-}

cpp/ql/src/semmle/code/cpp/security/SensitiveExprs.qll

@@ -238,21 +238,12 @@ predicate insideFunctionValueMoveTo(Element src, Element dest)
       returnArgument(c.getTarget(), sourceArg)
       and src = c.getArgument(sourceArg)
       and dest = c)
-    or exists (MessageExpr send |
-      methodReturningAnyArgument(send.getStaticTarget())
-      and not send instanceof FormattingFunctionCall
-      and src = send.getAnArgument()
-      and dest = send)
     or exists(FormattingFunctionCall formattingSend, int arg, FormatLiteral format, string argFormat |
       dest = formattingSend
       and formattingSend.getArgument(arg) = src
       and format = formattingSend.getFormat()
       and format.getConversionChar(arg - formattingSend.getTarget().getNumberOfParameters()) = argFormat
       and (argFormat = "s" or argFormat = "S" or argFormat = "@"))
-    or exists (ExprMessageExpr send |
-      methodReturningReceiver(send.getStaticTarget())
-      and src = send.getReceiver()
-      and dest = send)
     // Expressions computed from tainted data are also tainted
     or (exists (FunctionCall call | dest = call and isPureFunction(call.getTarget().getName()) |
       call.getAnArgument() = src
@@ -457,60 +448,6 @@ private predicate returnArgument(Function f, int sourceArg)
   or (f.hasGlobalName("gethostbyaddr") and sourceArg = 0)
 }
 
-/** A method where if any argument is tainted, the return value should be, too */
-private predicate methodReturningAnyArgument(MemberFunction method) {
-  method.getQualifiedName().matches("NS%Array%::+array%") or
-  method.getQualifiedName().matches("NS%Array%::-arrayBy%") or
-  method.getQualifiedName().matches("NS%Array%::-componentsJoinedByString:") or
-  method.getQualifiedName().matches("NS%Array%::-init%") or
-  method.getQualifiedName().matches("NS%Data%::+dataWith%") or
-  method.getQualifiedName().matches("NS%Data%::-initWith%") or
-  method.getQualifiedName().matches("NS%String%::+pathWithComponents:") or
-  method.getQualifiedName().matches("NS%String%::+stringWith%") or
-  method.getQualifiedName().matches("NS%String%::-initWithCString:") or
-  method.getQualifiedName().matches("NS%String%::-initWithCString:length:") or
-  method.getQualifiedName().matches("NS%String%::-initWithCStringNoCopy:length:") or
-  method.getQualifiedName().matches("NS%String%::-initWithCharacters:length:") or
-  method.getQualifiedName().matches("NS%String%::-initWithCharactersNoCopy:length:freeWhenDone:") or
-  method.getQualifiedName().matches("NS%String%::-initWithFormat:") or
-  method.getQualifiedName().matches("NS%String%::-initWithFormat:arguments:") or
-  method.getQualifiedName().matches("NS%String%::-initWithString:") or
-  method.getQualifiedName().matches("NS%String%::-initWithUTF8String:") or
-  method.getQualifiedName().matches("NS%String%::-stringByAppendingFormat:") or
-  method.getQualifiedName().matches("NS%String%::-stringByAppendingString:") or
-  method.getQualifiedName().matches("NS%String%::-stringByPaddingToLength:withString:startingAtIndex:") or
-  method.getQualifiedName().matches("NS%String%::-stringByReplacing%") or
-  method.getQualifiedName().matches("NS%String%::-stringsByAppendingPaths:")
-}
-
-/** A method where if the receiver is tainted, the return value should be, too */
-private predicate methodReturningReceiver(MemberFunction method) {
-  method.getQualifiedName().matches("NS%Array%::-arrayBy%") or
-  method.getQualifiedName().matches("NS%Array%::-componentsJoinedByString:") or
-  method.getQualifiedName().matches("NS%Array%::-firstObject") or
-  method.getQualifiedName().matches("NS%Array%::-lastObject") or
-  method.getQualifiedName().matches("NS%Array%::-objectAt%") or
-  method.getQualifiedName().matches("NS%Array%::-pathsMatchingExtensions:") or
-  method.getQualifiedName().matches("NS%Array%::-sortedArray%") or
-  method.getQualifiedName().matches("NS%Array%::-subarrayWithRange:") or
-  method.getQualifiedName().matches("NS%Data%::-bytes") or
-  method.getQualifiedName().matches("NS%Data%::-subdataWithRange:") or
-  method.getQualifiedName().matches("NS%String%::-capitalizedString%") or
-  method.getQualifiedName().matches("NS%String%::-componentsSeparatedByCharactersInSet:") or
-  method.getQualifiedName().matches("NS%String%::-componentsSeparatedByString:") or
-  method.getQualifiedName().matches("NS%String%::-cStringUsingEncoding:") or
-  method.getQualifiedName().matches("NS%String%::-dataUsingEncoding:%") or
-  method.getQualifiedName().matches("NS%String%::-lowercaseString%") or
-  method.getQualifiedName().matches("NS%String%::-pathComponents") or
-  method.getQualifiedName().matches("NS%String%::-stringBy%") or
-  method.getQualifiedName().matches("NS%String%::-stringsByAppendingPaths:") or
-  method.getQualifiedName().matches("NS%String%::-substringFromIndex:") or
-  method.getQualifiedName().matches("NS%String%::-substringToIndex:") or
-  method.getQualifiedName().matches("NS%String%::-substringWithRange:") or
-  method.getQualifiedName().matches("NS%String%::-uppercaseString%") or
-  method.getQualifiedName().matches("NS%String%::-UTF8String")
-}
-
 /**
  * Resolve potential target function(s) for `call`.
  *

cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll

@@ -118,8 +118,9 @@ class ExplicitUpcast extends ExplicitCast {
   }
 
   pragma [nomagic]
-  private predicate isDisambiguatingStaticCall0(StaticCall c, StaticCallable target, ValueOrRefType t) {
+  private predicate isDisambiguatingStaticCall0(StaticCall c, StaticCallable target, string name, ValueOrRefType t) {
     this.isArgument(c, target) and
+    name = target.getName() and
     (
       t = c.(QualifiableExpr).getQualifier().getType()
       or
@@ -131,9 +132,9 @@ class ExplicitUpcast extends ExplicitCast {
   /** Holds if this upcast may be used to disambiguate the target of a static call. */
   pragma [nomagic]
   private predicate isDisambiguatingStaticCall(StaticCallable other, int args) {
-    exists(StaticCall c, StaticCallable target, ValueOrRefType t |
-      this.isDisambiguatingStaticCall0(c, target, t) |
-      hasStaticCallable(t, other, target.getName()) and
+    exists(StaticCall c, StaticCallable target, ValueOrRefType t, string name |
+      this.isDisambiguatingStaticCall0(c, target, name, t) |
+      hasStaticCallable(t, other, name) and
       args = c.getNumberOfArguments() and
       other != target
     )

csharp/ql/src/Language Abuse/UselessUpcast.ql

@@ -25,7 +25,7 @@ string metachar() {
 string getAMatchedString(Expr e) {
   result = getAMatchedConstant(e.(RegExpLiteral).getRoot()).getValue()
   or
-  result = e.(StringLiteral).getValue()
+  result = e.getStringValue()
 }
 
 /** Gets a constant matched by `t`. */

javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql

@@ -60,6 +60,7 @@ import semmle.javascript.frameworks.Azure
 import semmle.javascript.frameworks.Babel
 import semmle.javascript.frameworks.ComposedFunctions
 import semmle.javascript.frameworks.ClientRequests
+import semmle.javascript.frameworks.CookieLibraries
 import semmle.javascript.frameworks.Credentials
 import semmle.javascript.frameworks.CryptoLibraries
 import semmle.javascript.frameworks.DigitalOcean

javascript/ql/src/javascript.qll

@@ -65,3 +65,27 @@ abstract class DatabaseAccess extends DataFlow::Node {
   /** Gets an argument to this database access that is interpreted as a query. */
   abstract DataFlow::Node getAQueryArgument();
 }
+
+/**
+ * A data flow node that reads persistent data.
+ */
+abstract class PersistentReadAccess extends DataFlow::Node {
+
+  /**
+   * Gets a corresponding persistent write, if any.
+   */
+  abstract PersistentWriteAccess getAWrite();
+
+}
+
+/**
+ * A data flow node that writes persistent data.
+ */
+abstract class PersistentWriteAccess extends DataFlow::Node {
+
+  /**
+   * Gets the data flow node corresponding to the written value.
+   */
+  abstract DataFlow::Node getValue();
+
+}