diff --git a/change-notes/1.24/analysis-javascript.md b/change-notes/1.24/analysis-javascript.md index c56e5f551d1..c6c544e2f3d 100644 --- a/change-notes/1.24/analysis-javascript.md +++ b/change-notes/1.24/analysis-javascript.md @@ -12,9 +12,9 @@ ## New queries -| **Query** | **Tags** | **Purpose** | -|---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| - +| **Query** | **Tags** | **Purpose** | +|---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| Cross-site scripting through exception (`js/xss-through-exception`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities where an exception is written to the DOM. Results are not shown on LGTM by default. | ## Changes to existing queries @@ -25,3 +25,6 @@ | Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | This query now recognizes additional ways event handler receivers can be bound. | ## Changes to libraries + +* The predicates `RegExpTerm.getSuccessor` and `RegExpTerm.getPredecessor` have been changed to reflect textual, not operational, matching order. This only makes a difference in lookbehind assertions, which are operationally matched backwards. Previously, `getSuccessor` would mimick this, so in an assertion `(?<=ab)` the term `b` would be considered the predecessor, not the successor, of `a`. Textually, however, `a` is still matched before `b`, and this is the order we now follow. + diff --git a/cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql b/cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql index 1142cc1a45b..e137e90c716 100644 --- a/cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql +++ b/cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql @@ -84,10 +84,10 @@ predicate hasZeroParamDecl(Function f) { } // True if this file (or header) was compiled as a C file -predicate isCompiledAsC(Function f) { - exists(File file | file.compiledAsC() | - file = f.getFile() or file.getAnIncludedFile+() = f.getFile() - ) +predicate isCompiledAsC(File f) { + f.compiledAsC() + or + exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f) } from FunctionCall fc, Function f, Parameter p @@ -95,7 +95,7 @@ where f = fc.getTarget() and p = f.getAParameter() and hasZeroParamDecl(f) and - isCompiledAsC(f) and + isCompiledAsC(f.getFile()) and not f.isVarargs() and not f instanceof BuiltInFunction and p.getIndex() < fc.getNumberOfArguments() and diff --git a/cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.ql b/cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.ql index a3e78fe61c4..705c1ec4dc8 100644 --- a/cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.ql +++ b/cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.ql @@ -24,10 +24,10 @@ predicate hasZeroParamDecl(Function f) { } // True if this file (or header) was compiled as a C file -predicate isCompiledAsC(Function f) { - exists(File file | file.compiledAsC() | - file = f.getFile() or file.getAnIncludedFile+() = f.getFile() - ) +predicate isCompiledAsC(File f) { + f.compiledAsC() + or + exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f) } from FunctionCall fc, Function f @@ -36,7 +36,7 @@ where not f.isVarargs() and not f instanceof BuiltInFunction and hasZeroParamDecl(f) and - isCompiledAsC(f) and + isCompiledAsC(f.getFile()) and // There is an explicit declaration of the function whose parameter count is larger // than the number of call arguments exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | diff --git a/cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.ql b/cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.ql index b0bd7746c15..59368236bbd 100644 --- a/cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.ql +++ b/cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.ql @@ -25,10 +25,10 @@ predicate hasZeroParamDecl(Function f) { } // True if this file (or header) was compiled as a C file -predicate isCompiledAsC(Function f) { - exists(File file | file.compiledAsC() | - file = f.getFile() or file.getAnIncludedFile+() = f.getFile() - ) +predicate isCompiledAsC(File f) { + f.compiledAsC() + or + exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f) } from FunctionCall fc, Function f @@ -36,7 +36,7 @@ where f = fc.getTarget() and not f.isVarargs() and hasZeroParamDecl(f) and - isCompiledAsC(f) and + isCompiledAsC(f.getFile()) and exists(f.getBlock()) and // There must not exist a declaration with the number of parameters // at least as large as the number of call arguments diff --git a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java index 3a4160c2d1f..f0e154213c0 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java +++ b/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java @@ -21,8 +21,10 @@ import com.semmle.util.language.LegacyLanguage; import com.semmle.util.process.Env; import com.semmle.util.projectstructure.ProjectLayout; import com.semmle.util.trap.TrapWriter; +import java.io.BufferedReader; import java.io.File; import java.io.IOException; +import java.io.InputStreamReader; import java.io.Reader; import java.lang.ProcessBuilder.Redirect; import java.net.URI; @@ -195,6 +197,11 @@ public class AutoBuild { private final String defaultEncoding; private ExecutorService threadPool; private volatile boolean seenCode = false; + private boolean installDependencies = false; + private int installDependenciesTimeout; + + /** The default timeout when running yarn, in milliseconds. */ + public static final int INSTALL_DEPENDENCIES_DEFAULT_TIMEOUT = 10 * 60 * 1000; // 10 minutes public AutoBuild() { this.LGTM_SRC = toRealPath(getPathFromEnvVar("LGTM_SRC")); @@ -204,6 +211,11 @@ public class AutoBuild { this.typeScriptMode = getEnumFromEnvVar("LGTM_INDEX_TYPESCRIPT", TypeScriptMode.class, TypeScriptMode.FULL); this.defaultEncoding = getEnvVar("LGTM_INDEX_DEFAULT_ENCODING"); + this.installDependencies = Boolean.valueOf(getEnvVar("LGTM_INDEX_TYPESCRIPT_INSTALL_DEPS")); + this.installDependenciesTimeout = + Env.systemEnv() + .getInt( + "LGTM_INDEX_TYPESCRIPT_INSTALL_DEPS_TIMEOUT", INSTALL_DEPENDENCIES_DEFAULT_TIMEOUT); setupFileTypes(); setupXmlMode(); setupMatchers(); @@ -533,6 +545,10 @@ public class AutoBuild { List tsconfigFiles = new ArrayList<>(); findFilesToExtract(defaultExtractor, filesToExtract, tsconfigFiles); + if (!tsconfigFiles.isEmpty() && this.installDependencies) { + this.installDependencies(filesToExtract); + } + // extract TypeScript projects and files Set extractedFiles = extractTypeScript(defaultExtractor, filesToExtract, tsconfigFiles); @@ -549,6 +565,61 @@ public class AutoBuild { } } + /** Returns true if yarn is installed, otherwise prints a warning and returns false. */ + private boolean verifyYarnInstallation() { + ProcessBuilder pb = new ProcessBuilder(Arrays.asList("yarn", "-v")); + try { + Process process = pb.start(); + boolean completed = process.waitFor(this.installDependenciesTimeout, TimeUnit.MILLISECONDS); + if (!completed) { + System.err.println("Yarn could not be launched. Timeout during 'yarn -v'."); + return false; + } + BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream())); + String version = reader.readLine(); + System.out.println("Found yarn version: " + version); + return true; + } catch (IOException | InterruptedException ex) { + System.err.println( + "Yarn not found. Please put 'yarn' on the PATH for automatic dependency installation."); + Exceptions.ignore(ex, "Continue without dependency installation"); + return false; + } + } + + protected void installDependencies(Set filesToExtract) { + if (!verifyYarnInstallation()) { + return; + } + for (Path file : filesToExtract) { + if (file.getFileName().toString().equals("package.json")) { + System.out.println("Installing dependencies from " + file); + ProcessBuilder pb = + new ProcessBuilder( + Arrays.asList( + "yarn", + "install", + "--verbose", + "--non-interactive", + "--ignore-scripts", + "--ignore-platform", + "--ignore-engines", + "--ignore-optional", + "--no-default-rc", + "--no-bin-links", + "--pure-lockfile")); + pb.directory(file.getParent().toFile()); + pb.redirectOutput(Redirect.INHERIT); + pb.redirectError(Redirect.INHERIT); + try { + pb.start().waitFor(this.installDependenciesTimeout, TimeUnit.MILLISECONDS); + } catch (IOException | InterruptedException ex) { + throw new ResourceError("Could not install dependencies from " + file, ex); + } + } + } + } + private ExtractorConfig mkExtractorConfig() { ExtractorConfig config = new ExtractorConfig(true); config = config.withSourceType(getSourceType()); diff --git a/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java b/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java index 8db4ca22da5..3e39cfc6706 100644 --- a/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java +++ b/javascript/extractor/src/com/semmle/js/extractor/test/AutoBuildTests.java @@ -128,6 +128,11 @@ public class AutoBuildTests { } } + @Override + protected void installDependencies(Set filesToExtract) { + // never install dependencies during testing + } + @Override protected void extractXml() throws IOException { Files.walkFileTree( diff --git a/javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp b/javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp new file mode 100644 index 00000000000..f815794f6b8 --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/ExceptionXss.qhelp @@ -0,0 +1,54 @@ + + + + +

+Directly writing exceptions to a webpage without sanitization allows for a cross-site scripting +vulnerability if the value of the exception can be influenced by a user. +

+ + + +

+To guard against cross-site scripting, consider using contextual output encoding/escaping before +writing user input to the page, or one of the other solutions that are mentioned in the +references. +

+ + + +

+The following example shows an exception being written directly to the document, +and this exception can potentially be influenced by the page URL, +leaving the website vulnerable to cross-site scripting. +

+ + + + +
  • +OWASP: +DOM based +XSS Prevention Cheat Sheet. +
    • +
  • +OWASP: +XSS +(Cross Site Scripting) Prevention Cheat Sheet. +
    • +
  • +OWASP +DOM Based XSS. +
    • +
  • +OWASP +Types of Cross-Site +Scripting. +
    • +
  • +Wikipedia: Cross-site scripting. +
    • +     +     diff --git a/javascript/ql/src/Security/CWE-079/ExceptionXss.ql b/javascript/ql/src/Security/CWE-079/ExceptionXss.ql new file mode 100644 index 00000000000..cf22b54e465 --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/ExceptionXss.ql @@ -0,0 +1,24 @@ +/** + * @name Cross-site scripting through exception + * @description Inserting data from an exception containing user + * input into the DOM may enable cross-site scripting. + * @kind path-problem + * @problem.severity error + * @precision medium + * @id js/xss-through-exception + * @tags security + * external/cwe/cwe-079 + * external/cwe/cwe-116 + */ + +import javascript +import semmle.javascript.security.dataflow.ExceptionXss::ExceptionXss +import DataFlow::PathGraph + +from + Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where + cfg.hasFlowPath(source, sink) +select sink.getNode(), source, sink, + sink.getNode().(Xss::Shared::Sink).getVulnerabilityKind() + " vulnerability due to $@.", source.getNode(), + "user-provided value" diff --git a/javascript/ql/src/Security/CWE-079/examples/ExceptionXss.js b/javascript/ql/src/Security/CWE-079/examples/ExceptionXss.js new file mode 100644 index 00000000000..7f8897387ff --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/examples/ExceptionXss.js @@ -0,0 +1,10 @@ +function setLanguageOptions() { + var href = document.location.href, + deflt = href.substring(href.indexOf("default=")+8); + + try { + var parsed = unknownParseFunction(deflt); + } catch(e) { + document.write("Had an error: " + e + "."); + } +} diff --git a/javascript/ql/src/semmle/javascript/Expr.qll b/javascript/ql/src/semmle/javascript/Expr.qll index c6464767e8f..258a85904d2 100644 --- a/javascript/ql/src/semmle/javascript/Expr.qll +++ b/javascript/ql/src/semmle/javascript/Expr.qll @@ -245,6 +245,21 @@ class Expr extends @expr, ExprOrStmt, ExprOrType, AST::ValueNode { ctx.(ConditionalExpr).inNullSensitiveContext() ) } + + /** + * Gets the data-flow node where exceptions thrown by this expression will + * propagate if this expression causes an exception to be thrown. + */ + DataFlow::Node getExceptionTarget() { + if exists(this.getEnclosingStmt().getEnclosingTryCatchStmt()) + then + result = DataFlow::parameterNode(this + .getEnclosingStmt() + .getEnclosingTryCatchStmt() + .getACatchClause() + .getAParameter()) + else result = any(DataFlow::FunctionNode f | f.getFunction() = this.getContainer()).getExceptionalReturn() + } } /** diff --git a/javascript/ql/src/semmle/javascript/Regexp.qll b/javascript/ql/src/semmle/javascript/Regexp.qll index 42c29e3e9e2..c79548e04c5 100644 --- a/javascript/ql/src/semmle/javascript/Regexp.qll +++ b/javascript/ql/src/semmle/javascript/Regexp.qll @@ -73,21 +73,21 @@ class RegExpTerm extends Locatable, @regexpterm { /** Holds if this regular expression term can match the empty string. */ predicate isNullable() { none() } // Overridden in subclasses. - /** Gets the regular expression term that is matched before this one, if any. */ + /** Gets the regular expression term that is matched (textually) before this one, if any. */ RegExpTerm getPredecessor() { exists(RegExpSequence seq, int i | seq.getChild(i) = this and - seq.getChild(i - getDirection()) = result + seq.getChild(i - 1) = result ) or result = getParent().(RegExpTerm).getPredecessor() } - /** Gets the regular expression term that is matched after this one, if any. */ + /** Gets the regular expression term that is matched (textually) after this one, if any. */ RegExpTerm getSuccessor() { exists(RegExpSequence seq, int i | seq.getChild(i) = this and - seq.getChild(i + getDirection()) = result + seq.getChild(i + 1) = result ) or exists(RegExpTerm parent | @@ -98,12 +98,6 @@ class RegExpTerm extends Locatable, @regexpterm { ) } - /** - * Gets the matching direction of this term: `1` if it is in a forward-matching - * context, `-1` if it is in a backward-matching context. - */ - private int getDirection() { if isInBackwardMatchingContext() then result = -1 else result = 1 } - /** * Holds if this regular term is in a forward-matching context, that is, * it has no enclosing lookbehind assertions. diff --git a/javascript/ql/src/semmle/javascript/Stmt.qll b/javascript/ql/src/semmle/javascript/Stmt.qll index eca791ba565..c96d6509e8d 100644 --- a/javascript/ql/src/semmle/javascript/Stmt.qll +++ b/javascript/ql/src/semmle/javascript/Stmt.qll @@ -55,6 +55,18 @@ class Stmt extends @stmt, ExprOrStmt, Documentable { } override predicate isAmbient() { hasDeclareKeyword(this) or getParent().isAmbient() } + + /** + * Gets the `try` statement with a catch block containing this statement without + * crossing function boundaries or other `try ` statements with catch blocks. + */ + TryStmt getEnclosingTryCatchStmt() { + getParentStmt+() = result.getBody() and + exists(result.getACatchClause()) and + not exists(TryStmt mid | exists(mid.getACatchClause()) | + getParentStmt+() = mid.getBody() and mid.getParentStmt+() = result.getBody() + ) + } } /** diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index 426a2069fd5..9e61c627cdf 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -1125,6 +1125,9 @@ class MidPathNode extends PathNode, MkMidNode { // Skip to the top of big left-leaning string concatenation trees. nd = any(AddExpr add).flow() and nd = any(AddExpr add).getAnOperand().flow() + or + // Skip the exceptional return on functions, as this highlights the entire function. + nd = any(DataFlow::FunctionNode f).getExceptionalReturn() } } diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index c4106ffa17f..78779e9b881 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll @@ -558,6 +558,23 @@ module TaintTracking { succ = this } } + + + /** + * A taint propagating data flow edge arising from calling `String.prototype.match()`. + */ + private class StringMatchTaintStep extends AdditionalTaintStep, DataFlow::MethodCallNode { + StringMatchTaintStep() { + this.getMethodName() = "match" and + this.getNumArgument() = 1 and + this.getArgument(0) .analyze().getAType() = TTRegExp() + } + + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + pred = this.getReceiver() and + succ = this + } + } /** * A taint propagating data flow edge arising from JSON unparsing. diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll index e6154902fad..5544d7d03ff 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -66,19 +66,7 @@ predicate localExceptionStep(DataFlow::Node pred, DataFlow::Node succ) { or DataFlow::exceptionalInvocationReturnNode(pred, expr) | - // Propagate out of enclosing function. - not exists(getEnclosingTryStmt(expr.getEnclosingStmt())) and - exists(Function f | - f = expr.getEnclosingFunction() and - DataFlow::exceptionalFunctionReturnNode(succ, f) - ) - or - // Propagate to enclosing try/catch. - // To avoid false flow, we only propagate to an unguarded catch clause. - exists(TryStmt try | - try = getEnclosingTryStmt(expr.getEnclosingStmt()) and - DataFlow::parameterNode(succ, try.getCatchClause().getAParameter()) - ) + succ = expr.getExceptionTarget() ) } @@ -156,19 +144,6 @@ private module CachedSteps { cached predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { argumentPassing(_, pred, _, succ) } - /** - * Gets the `try` statement containing `stmt` without crossing function boundaries - * or other `try ` statements. - */ - cached - TryStmt getEnclosingTryStmt(Stmt stmt) { - result.getBody() = stmt - or - not stmt instanceof Function and - not stmt = any(TryStmt try).getBody() and - result = getEnclosingTryStmt(stmt.getParentStmt()) - } - /** * Holds if there is a flow step from `pred` to `succ` through: * - returning a value from a function call, or diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll index 1946099777d..24634eec94c 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll @@ -6,7 +6,7 @@ import javascript module DomBasedXss { - import Xss::DomBasedXss + import DomBasedXssCustomizations::DomBasedXss /** * A taint-tracking configuration for reasoning about XSS. @@ -33,16 +33,4 @@ module DomBasedXss { node instanceof Sanitizer } } - - /** A source of remote user input, considered as a flow source for DOM-based XSS. */ - class RemoteFlowSourceAsSource extends Source { - RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource } - } - - /** - * An access of the URL of this page, or of the referrer to this page. - */ - class LocationSource extends Source { - LocationSource() { this = DOM::locationSource() } - } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXssCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXssCustomizations.qll new file mode 100644 index 00000000000..b74977c8cb5 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXssCustomizations.qll @@ -0,0 +1,23 @@ +/** + * Provides default sources for reasoning about DOM-based + * cross-site scripting vulnerabilities. + */ + + +import javascript + +module DomBasedXss { + import Xss::DomBasedXss + + /** A source of remote user input, considered as a flow source for DOM-based XSS. */ + class RemoteFlowSourceAsSource extends Source { + RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource } + } + + /** + * An access of the URL of this page, or of the referrer to this page. + */ + class LocationSource extends Source { + LocationSource() { this = DOM::locationSource() } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll new file mode 100644 index 00000000000..2d088f42fa0 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll @@ -0,0 +1,80 @@ +/** + * Provides a taint-tracking configuration for reasoning about cross-site + * scripting vulnerabilities where the taint-flow passes through a thrown + * exception. + */ + +import javascript + +module ExceptionXss { + import DomBasedXssCustomizations::DomBasedXss as DomBasedXssCustom + import ReflectedXssCustomizations::ReflectedXss as ReflectedXssCustom + import Xss as Xss + + /** + * Holds if `node` is unlikely to cause an exception containing sensitive information to be thrown. + */ + private predicate isUnlikelyToThrowSensitiveInformation(DataFlow::Node node) { + node = any(DataFlow::CallNode call | call.getCalleeName() = "getElementById").getAnArgument() + or + node = any(DataFlow::CallNode call | call.getCalleeName() = "indexOf").getAnArgument() + or + node = any(DataFlow::CallNode call | call.getCalleeName() = "stringify").getAnArgument() + or + node = DataFlow::globalVarRef("console").getAMemberCall(_).getAnArgument() + } + + /** + * Holds if `node` can possibly cause an exception containing sensitive information to be thrown. + */ + predicate canThrowSensitiveInformation(DataFlow::Node node) { + not isUnlikelyToThrowSensitiveInformation(node) and + ( + // in the case of reflective calls the below ensures that both InvokeNodes have no known callee. + forex(DataFlow::InvokeNode call | node = call.getAnArgument() | not exists(call.getACallee())) + or + node.asExpr().getEnclosingStmt() instanceof ThrowStmt + ) + } + + /** + * A FlowLabel representing tainted data that has not been thrown in an exception. + * In the js/xss-through-exception query data-flow can only reach a sink after + * the data has been thrown as an exception, and data that has not been thrown + * as an exception therefore has this flow label, and only this flow label, associated with it. + */ + class NotYetThrown extends DataFlow::FlowLabel { + NotYetThrown() { this = "NotYetThrown" } + } + + /** + * A taint-tracking configuration for reasoning about XSS with possible exceptional flow. + * Flow labels are used to ensure that we only report taint-flow that has been thrown in + * an exception. + */ + class Configuration extends TaintTracking::Configuration { + Configuration() { this = "ExceptionXss" } + + override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { + source instanceof Xss::Shared::Source and label instanceof NotYetThrown + } + + override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { + sink instanceof Xss::Shared::Sink and not label instanceof NotYetThrown + } + + override predicate isSanitizer(DataFlow::Node node) { node instanceof Xss::Shared::Sanitizer } + + override predicate isAdditionalFlowStep( + DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl, + DataFlow::FlowLabel outlbl + ) { + inlbl instanceof NotYetThrown and (outlbl.isTaint() or outlbl instanceof NotYetThrown) and + succ = pred.asExpr().getExceptionTarget() and + canThrowSensitiveInformation(pred) + or + // All the usual taint-flow steps apply on data-flow before it has been thrown in an exception. + this.isAdditionalFlowStep(pred, succ) and inlbl instanceof NotYetThrown and outlbl instanceof NotYetThrown + } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXss.qll index e84fd372337..ece299d7fa0 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXss.qll @@ -6,7 +6,7 @@ import javascript module ReflectedXss { - import Xss::ReflectedXss + import ReflectedXssCustomizations::ReflectedXss /** * A taint-tracking configuration for reasoning about XSS. @@ -23,13 +23,4 @@ module ReflectedXss { node instanceof Sanitizer } } - - /** A third-party controllable request input, considered as a flow source for reflected XSS. */ - class ThirdPartyRequestInputAccessAsSource extends Source { - ThirdPartyRequestInputAccessAsSource() { - this.(HTTP::RequestInputAccess).isThirdPartyControllable() - or - this.(HTTP::RequestHeaderAccess).getAHeaderName() = "referer" - } - } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXssCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXssCustomizations.qll new file mode 100644 index 00000000000..6906d5122f6 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXssCustomizations.qll @@ -0,0 +1,19 @@ +/** + * Provides default sources for reasoning about reflected + * cross-site scripting vulnerabilities. + */ + +import javascript + +module ReflectedXss { + import Xss::ReflectedXss + + /** A third-party controllable request input, considered as a flow source for reflected XSS. */ + class ThirdPartyRequestInputAccessAsSource extends Source { + ThirdPartyRequestInputAccessAsSource() { + this.(HTTP::RequestInputAccess).isThirdPartyControllable() + or + this.(HTTP::RequestHeaderAccess).getAHeaderName() = "referer" + } + } +} diff --git a/javascript/ql/test/query-tests/RegExp/UnmatchableCaret/tst.js b/javascript/ql/test/query-tests/RegExp/UnmatchableCaret/tst.js index d2124572861..50d70094dd6 100644 --- a/javascript/ql/test/query-tests/RegExp/UnmatchableCaret/tst.js +++ b/javascript/ql/test/query-tests/RegExp/UnmatchableCaret/tst.js @@ -26,4 +26,7 @@ /^(^y|^z)(u$|v$)$/; // OK -/x*^y/; \ No newline at end of file +/x*^y/; + +// OK +/(?<=(^|\/)(\.|\.\.))$/; diff --git a/javascript/ql/test/query-tests/RegExp/UnmatchableDollar/UnmatchableDollar.expected b/javascript/ql/test/query-tests/RegExp/UnmatchableDollar/UnmatchableDollar.expected index 9b5a291f2e9..6e57f22c18a 100644 --- a/javascript/ql/test/query-tests/RegExp/UnmatchableDollar/UnmatchableDollar.expected +++ b/javascript/ql/test/query-tests/RegExp/UnmatchableDollar/UnmatchableDollar.expected @@ -1,3 +1,4 @@ | tst.js:2:10:2:10 | $ | This assertion can never match. | | tst.js:11:3:11:3 | $ | This assertion can never match. | | tst.js:20:3:20:3 | $ | This assertion can never match. | +| tst.js:38:6:38:6 | $ | This assertion can never match. | diff --git a/javascript/ql/test/query-tests/RegExp/UnmatchableDollar/tst.js b/javascript/ql/test/query-tests/RegExp/UnmatchableDollar/tst.js index 78e6b9f179d..95708b3cd0e 100644 --- a/javascript/ql/test/query-tests/RegExp/UnmatchableDollar/tst.js +++ b/javascript/ql/test/query-tests/RegExp/UnmatchableDollar/tst.js @@ -32,4 +32,7 @@ /x(?!y+$).*y.*/; // OK -/x(?=[yz]+$).*yz.*/; \ No newline at end of file +/x(?=[yz]+$).*yz.*/; + +// NOT OK +/(?<=$x)yz/; diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss.expected new file mode 100644 index 00000000000..bd07494e0f2 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss.expected @@ -0,0 +1,151 @@ +nodes +| exception-xss.js:2:9:2:31 | foo | +| exception-xss.js:2:15:2:31 | document.location | +| exception-xss.js:2:15:2:31 | document.location | +| exception-xss.js:9:11:9:13 | foo | +| exception-xss.js:10:10:10:10 | e | +| exception-xss.js:11:18:11:18 | e | +| exception-xss.js:11:18:11:18 | e | +| exception-xss.js:15:3:15:12 | exceptional return of inner(foo) | +| exception-xss.js:15:9:15:11 | foo | +| exception-xss.js:16:10:16:10 | e | +| exception-xss.js:17:18:17:18 | e | +| exception-xss.js:17:18:17:18 | e | +| exception-xss.js:21:11:21:13 | foo | +| exception-xss.js:21:11:21:21 | foo + "bar" | +| exception-xss.js:22:10:22:10 | e | +| exception-xss.js:23:18:23:18 | e | +| exception-xss.js:23:18:23:18 | e | +| exception-xss.js:33:11:33:22 | ["bar", foo] | +| exception-xss.js:33:19:33:21 | foo | +| exception-xss.js:34:10:34:10 | e | +| exception-xss.js:35:18:35:18 | e | +| exception-xss.js:35:18:35:18 | e | +| exception-xss.js:46:3:46:19 | exceptional return of deep("bar" + foo) | +| exception-xss.js:46:8:46:18 | "bar" + foo | +| exception-xss.js:46:16:46:18 | foo | +| exception-xss.js:47:10:47:10 | e | +| exception-xss.js:48:18:48:18 | e | +| exception-xss.js:48:18:48:18 | e | +| exception-xss.js:81:3:81:19 | exceptional return of myWeirdInner(foo) | +| exception-xss.js:81:16:81:18 | foo | +| exception-xss.js:82:10:82:10 | e | +| exception-xss.js:83:18:83:18 | e | +| exception-xss.js:83:18:83:18 | e | +| exception-xss.js:89:11:89:13 | foo | +| exception-xss.js:89:11:89:26 | foo.match(/foo/) | +| exception-xss.js:90:10:90:10 | e | +| exception-xss.js:91:18:91:18 | e | +| exception-xss.js:91:18:91:18 | e | +| exception-xss.js:95:11:95:22 | [foo, "bar"] | +| exception-xss.js:95:12:95:14 | foo | +| exception-xss.js:96:10:96:10 | e | +| exception-xss.js:97:18:97:18 | e | +| exception-xss.js:97:18:97:18 | e | +| exception-xss.js:102:12:102:14 | foo | +| exception-xss.js:106:10:106:10 | e | +| exception-xss.js:107:18:107:18 | e | +| exception-xss.js:107:18:107:18 | e | +| exception-xss.js:117:13:117:25 | req.params.id | +| exception-xss.js:117:13:117:25 | req.params.id | +| exception-xss.js:118:11:118:11 | e | +| exception-xss.js:119:14:119:30 | "Exception: " + e | +| exception-xss.js:119:14:119:30 | "Exception: " + e | +| exception-xss.js:119:30:119:30 | e | +| exception-xss.js:125:48:125:64 | document.location | +| exception-xss.js:125:48:125:64 | document.location | +| exception-xss.js:125:48:125:71 | documen ... .search | +| exception-xss.js:128:11:128:52 | session ... ssion') | +| exception-xss.js:129:10:129:10 | e | +| exception-xss.js:130:18:130:18 | e | +| exception-xss.js:130:18:130:18 | e | +| tst.js:298:9:298:16 | location | +| tst.js:298:9:298:16 | location | +| tst.js:299:10:299:10 | e | +| tst.js:300:20:300:20 | e | +| tst.js:300:20:300:20 | e | +| tst.js:305:10:305:17 | location | +| tst.js:305:10:305:17 | location | +| tst.js:307:10:307:10 | e | +| tst.js:308:20:308:20 | e | +| tst.js:308:20:308:20 | e | +edges +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:9:11:9:13 | foo | +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:15:9:15:11 | foo | +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:21:11:21:13 | foo | +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:33:19:33:21 | foo | +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:46:16:46:18 | foo | +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:81:16:81:18 | foo | +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:89:11:89:13 | foo | +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:95:12:95:14 | foo | +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:102:12:102:14 | foo | +| exception-xss.js:2:15:2:31 | document.location | exception-xss.js:2:9:2:31 | foo | +| exception-xss.js:2:15:2:31 | document.location | exception-xss.js:2:9:2:31 | foo | +| exception-xss.js:9:11:9:13 | foo | exception-xss.js:10:10:10:10 | e | +| exception-xss.js:10:10:10:10 | e | exception-xss.js:11:18:11:18 | e | +| exception-xss.js:10:10:10:10 | e | exception-xss.js:11:18:11:18 | e | +| exception-xss.js:15:3:15:12 | exceptional return of inner(foo) | exception-xss.js:16:10:16:10 | e | +| exception-xss.js:15:9:15:11 | foo | exception-xss.js:15:3:15:12 | exceptional return of inner(foo) | +| exception-xss.js:16:10:16:10 | e | exception-xss.js:17:18:17:18 | e | +| exception-xss.js:16:10:16:10 | e | exception-xss.js:17:18:17:18 | e | +| exception-xss.js:21:11:21:13 | foo | exception-xss.js:21:11:21:21 | foo + "bar" | +| exception-xss.js:21:11:21:21 | foo + "bar" | exception-xss.js:22:10:22:10 | e | +| exception-xss.js:22:10:22:10 | e | exception-xss.js:23:18:23:18 | e | +| exception-xss.js:22:10:22:10 | e | exception-xss.js:23:18:23:18 | e | +| exception-xss.js:33:11:33:22 | ["bar", foo] | exception-xss.js:34:10:34:10 | e | +| exception-xss.js:33:19:33:21 | foo | exception-xss.js:33:11:33:22 | ["bar", foo] | +| exception-xss.js:34:10:34:10 | e | exception-xss.js:35:18:35:18 | e | +| exception-xss.js:34:10:34:10 | e | exception-xss.js:35:18:35:18 | e | +| exception-xss.js:46:3:46:19 | exceptional return of deep("bar" + foo) | exception-xss.js:47:10:47:10 | e | +| exception-xss.js:46:8:46:18 | "bar" + foo | exception-xss.js:46:3:46:19 | exceptional return of deep("bar" + foo) | +| exception-xss.js:46:16:46:18 | foo | exception-xss.js:46:8:46:18 | "bar" + foo | +| exception-xss.js:47:10:47:10 | e | exception-xss.js:48:18:48:18 | e | +| exception-xss.js:47:10:47:10 | e | exception-xss.js:48:18:48:18 | e | +| exception-xss.js:81:3:81:19 | exceptional return of myWeirdInner(foo) | exception-xss.js:82:10:82:10 | e | +| exception-xss.js:81:16:81:18 | foo | exception-xss.js:81:3:81:19 | exceptional return of myWeirdInner(foo) | +| exception-xss.js:82:10:82:10 | e | exception-xss.js:83:18:83:18 | e | +| exception-xss.js:82:10:82:10 | e | exception-xss.js:83:18:83:18 | e | +| exception-xss.js:89:11:89:13 | foo | exception-xss.js:89:11:89:26 | foo.match(/foo/) | +| exception-xss.js:89:11:89:26 | foo.match(/foo/) | exception-xss.js:90:10:90:10 | e | +| exception-xss.js:90:10:90:10 | e | exception-xss.js:91:18:91:18 | e | +| exception-xss.js:90:10:90:10 | e | exception-xss.js:91:18:91:18 | e | +| exception-xss.js:95:11:95:22 | [foo, "bar"] | exception-xss.js:96:10:96:10 | e | +| exception-xss.js:95:12:95:14 | foo | exception-xss.js:95:11:95:22 | [foo, "bar"] | +| exception-xss.js:96:10:96:10 | e | exception-xss.js:97:18:97:18 | e | +| exception-xss.js:96:10:96:10 | e | exception-xss.js:97:18:97:18 | e | +| exception-xss.js:102:12:102:14 | foo | exception-xss.js:106:10:106:10 | e | +| exception-xss.js:106:10:106:10 | e | exception-xss.js:107:18:107:18 | e | +| exception-xss.js:106:10:106:10 | e | exception-xss.js:107:18:107:18 | e | +| exception-xss.js:117:13:117:25 | req.params.id | exception-xss.js:118:11:118:11 | e | +| exception-xss.js:117:13:117:25 | req.params.id | exception-xss.js:118:11:118:11 | e | +| exception-xss.js:118:11:118:11 | e | exception-xss.js:119:30:119:30 | e | +| exception-xss.js:119:30:119:30 | e | exception-xss.js:119:14:119:30 | "Exception: " + e | +| exception-xss.js:119:30:119:30 | e | exception-xss.js:119:14:119:30 | "Exception: " + e | +| exception-xss.js:125:48:125:64 | document.location | exception-xss.js:125:48:125:71 | documen ... .search | +| exception-xss.js:125:48:125:64 | document.location | exception-xss.js:125:48:125:71 | documen ... .search | +| exception-xss.js:125:48:125:71 | documen ... .search | exception-xss.js:128:11:128:52 | session ... ssion') | +| exception-xss.js:128:11:128:52 | session ... ssion') | exception-xss.js:129:10:129:10 | e | +| exception-xss.js:129:10:129:10 | e | exception-xss.js:130:18:130:18 | e | +| exception-xss.js:129:10:129:10 | e | exception-xss.js:130:18:130:18 | e | +| tst.js:298:9:298:16 | location | tst.js:299:10:299:10 | e | +| tst.js:298:9:298:16 | location | tst.js:299:10:299:10 | e | +| tst.js:299:10:299:10 | e | tst.js:300:20:300:20 | e | +| tst.js:299:10:299:10 | e | tst.js:300:20:300:20 | e | +| tst.js:305:10:305:17 | location | tst.js:307:10:307:10 | e | +| tst.js:305:10:305:17 | location | tst.js:307:10:307:10 | e | +| tst.js:307:10:307:10 | e | tst.js:308:20:308:20 | e | +| tst.js:307:10:307:10 | e | tst.js:308:20:308:20 | e | +#select +| exception-xss.js:11:18:11:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:11:18:11:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value | +| exception-xss.js:17:18:17:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:17:18:17:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value | +| exception-xss.js:23:18:23:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:23:18:23:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value | +| exception-xss.js:35:18:35:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:35:18:35:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value | +| exception-xss.js:48:18:48:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:48:18:48:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value | +| exception-xss.js:83:18:83:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:83:18:83:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value | +| exception-xss.js:91:18:91:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:91:18:91:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value | +| exception-xss.js:97:18:97:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:97:18:97:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value | +| exception-xss.js:107:18:107:18 | e | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:107:18:107:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value | +| exception-xss.js:119:14:119:30 | "Exception: " + e | exception-xss.js:117:13:117:25 | req.params.id | exception-xss.js:119:14:119:30 | "Exception: " + e | Cross-site scripting vulnerability due to $@. | exception-xss.js:117:13:117:25 | req.params.id | user-provided value | +| exception-xss.js:130:18:130:18 | e | exception-xss.js:125:48:125:64 | document.location | exception-xss.js:130:18:130:18 | e | Cross-site scripting vulnerability due to $@. | exception-xss.js:125:48:125:64 | document.location | user-provided value | +| tst.js:300:20:300:20 | e | tst.js:298:9:298:16 | location | tst.js:300:20:300:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:298:9:298:16 | location | user-provided value | +| tst.js:308:20:308:20 | e | tst.js:305:10:305:17 | location | tst.js:308:20:308:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:305:10:305:17 | location | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss.qlref b/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss.qlref new file mode 100644 index 00000000000..5fae24d64c4 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss.qlref @@ -0,0 +1 @@ +Security/CWE-079/ExceptionXss.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected index 018819e7b7c..033406f0df9 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected @@ -15,6 +15,11 @@ nodes | addEventListener.js:12:24:12:28 | event | | addEventListener.js:12:24:12:33 | event.data | | addEventListener.js:12:24:12:33 | event.data | +| exception-xss.js:2:9:2:31 | foo | +| exception-xss.js:2:15:2:31 | document.location | +| exception-xss.js:2:15:2:31 | document.location | +| exception-xss.js:86:17:86:19 | foo | +| exception-xss.js:86:17:86:19 | foo | | jquery.js:2:7:2:40 | tainted | | jquery.js:2:17:2:33 | document.location | | jquery.js:2:17:2:33 | document.location | @@ -313,9 +318,19 @@ nodes | tst.js:282:19:282:29 | window.name | | tst.js:285:59:285:65 | tainted | | tst.js:285:59:285:65 | tainted | -| tst.js:297:35:297:42 | location | -| tst.js:297:35:297:42 | location | -| tst.js:297:35:297:42 | location | +| tst.js:298:9:298:16 | location | +| tst.js:298:9:298:16 | location | +| tst.js:299:10:299:10 | e | +| tst.js:300:20:300:20 | e | +| tst.js:300:20:300:20 | e | +| tst.js:305:10:305:17 | location | +| tst.js:305:10:305:17 | location | +| tst.js:307:10:307:10 | e | +| tst.js:308:20:308:20 | e | +| tst.js:308:20:308:20 | e | +| tst.js:313:35:313:42 | location | +| tst.js:313:35:313:42 | location | +| tst.js:313:35:313:42 | location | | typeahead.js:20:13:20:45 | target | | typeahead.js:20:22:20:38 | document.location | | typeahead.js:20:22:20:38 | document.location | @@ -351,6 +366,10 @@ edges | addEventListener.js:10:21:10:25 | event | addEventListener.js:12:24:12:28 | event | | addEventListener.js:12:24:12:28 | event | addEventListener.js:12:24:12:33 | event.data | | addEventListener.js:12:24:12:28 | event | addEventListener.js:12:24:12:33 | event.data | +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:86:17:86:19 | foo | +| exception-xss.js:2:9:2:31 | foo | exception-xss.js:86:17:86:19 | foo | +| exception-xss.js:2:15:2:31 | document.location | exception-xss.js:2:9:2:31 | foo | +| exception-xss.js:2:15:2:31 | document.location | exception-xss.js:2:9:2:31 | foo | | jquery.js:2:7:2:40 | tainted | jquery.js:4:5:4:11 | tainted | | jquery.js:2:7:2:40 | tainted | jquery.js:4:5:4:11 | tainted | | jquery.js:2:7:2:40 | tainted | jquery.js:7:20:7:26 | tainted | @@ -610,7 +629,15 @@ edges | tst.js:282:9:282:29 | tainted | tst.js:285:59:285:65 | tainted | | tst.js:282:19:282:29 | window.name | tst.js:282:9:282:29 | tainted | | tst.js:282:19:282:29 | window.name | tst.js:282:9:282:29 | tainted | -| tst.js:297:35:297:42 | location | tst.js:297:35:297:42 | location | +| tst.js:298:9:298:16 | location | tst.js:299:10:299:10 | e | +| tst.js:298:9:298:16 | location | tst.js:299:10:299:10 | e | +| tst.js:299:10:299:10 | e | tst.js:300:20:300:20 | e | +| tst.js:299:10:299:10 | e | tst.js:300:20:300:20 | e | +| tst.js:305:10:305:17 | location | tst.js:307:10:307:10 | e | +| tst.js:305:10:305:17 | location | tst.js:307:10:307:10 | e | +| tst.js:307:10:307:10 | e | tst.js:308:20:308:20 | e | +| tst.js:307:10:307:10 | e | tst.js:308:20:308:20 | e | +| tst.js:313:35:313:42 | location | tst.js:313:35:313:42 | location | | typeahead.js:20:13:20:45 | target | typeahead.js:21:12:21:17 | target | | typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search | | typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search | @@ -634,6 +661,7 @@ edges | addEventListener.js:2:20:2:29 | event.data | addEventListener.js:1:43:1:47 | event | addEventListener.js:2:20:2:29 | event.data | Cross-site scripting vulnerability due to $@. | addEventListener.js:1:43:1:47 | event | user-provided value | | addEventListener.js:6:20:6:23 | data | addEventListener.js:5:43:5:48 | {data} | addEventListener.js:6:20:6:23 | data | Cross-site scripting vulnerability due to $@. | addEventListener.js:5:43:5:48 | {data} | user-provided value | | addEventListener.js:12:24:12:33 | event.data | addEventListener.js:10:21:10:25 | event | addEventListener.js:12:24:12:33 | event.data | Cross-site scripting vulnerability due to $@. | addEventListener.js:10:21:10:25 | event | user-provided value | +| exception-xss.js:86:17:86:19 | foo | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:86:17:86:19 | foo | Cross-site scripting vulnerability due to $@. | exception-xss.js:2:15:2:31 | document.location | user-provided value | | jquery.js:4:5:4:11 | tainted | jquery.js:2:17:2:33 | document.location | jquery.js:4:5:4:11 | tainted | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:33 | document.location | user-provided value | | jquery.js:7:5:7:34 | "
    " | jquery.js:2:17:2:33 | document.location | jquery.js:7:5:7:34 | "
    " | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:33 | document.location | user-provided value | | jquery.js:8:18:8:34 | "XSS: " + tainted | jquery.js:2:17:2:33 | document.location | jquery.js:8:18:8:34 | "XSS: " + tainted | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:33 | document.location | user-provided value | @@ -705,7 +733,9 @@ edges | tst.js:261:11:261:21 | window.name | tst.js:261:11:261:21 | window.name | tst.js:261:11:261:21 | window.name | Cross-site scripting vulnerability due to $@. | tst.js:261:11:261:21 | window.name | user-provided value | | tst.js:277:22:277:29 | location | tst.js:277:22:277:29 | location | tst.js:277:22:277:29 | location | Cross-site scripting vulnerability due to $@. | tst.js:277:22:277:29 | location | user-provided value | | tst.js:285:59:285:65 | tainted | tst.js:282:19:282:29 | window.name | tst.js:285:59:285:65 | tainted | Cross-site scripting vulnerability due to $@. | tst.js:282:19:282:29 | window.name | user-provided value | -| tst.js:297:35:297:42 | location | tst.js:297:35:297:42 | location | tst.js:297:35:297:42 | location | Cross-site scripting vulnerability due to $@. | tst.js:297:35:297:42 | location | user-provided value | +| tst.js:300:20:300:20 | e | tst.js:298:9:298:16 | location | tst.js:300:20:300:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:298:9:298:16 | location | user-provided value | +| tst.js:308:20:308:20 | e | tst.js:305:10:305:17 | location | tst.js:308:20:308:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:305:10:305:17 | location | user-provided value | +| tst.js:313:35:313:42 | location | tst.js:313:35:313:42 | location | tst.js:313:35:313:42 | location | Cross-site scripting vulnerability due to $@. | tst.js:313:35:313:42 | location | user-provided value | | typeahead.js:25:18:25:20 | val | typeahead.js:20:22:20:38 | document.location | typeahead.js:25:18:25:20 | val | Cross-site scripting vulnerability due to $@. | typeahead.js:20:22:20:38 | document.location | user-provided value | | v-html.vue:2:8:2:23 | v-html=tainted | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted | Cross-site scripting vulnerability due to $@. | v-html.vue:6:42:6:58 | document.location | user-provided value | | winjs.js:3:43:3:49 | tainted | winjs.js:2:17:2:33 | document.location | winjs.js:3:43:3:49 | tainted | Cross-site scripting vulnerability due to $@. | winjs.js:2:17:2:33 | document.location | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/exception-xss.js b/javascript/ql/test/query-tests/Security/CWE-079/exception-xss.js new file mode 100644 index 00000000000..ac15527209e --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/exception-xss.js @@ -0,0 +1,132 @@ +(function() { + var foo = document.location; + + function inner(x) { + unknown(x); + } + + try { + unknown(foo); + } catch(e) { + $('myId').html(e); // NOT OK! + } + + try { + inner(foo); + } catch(e) { + $('myId').html(e); // NOT OK! + } + + try { + unknown(foo + "bar"); + } catch(e) { + $('myId').html(e); // NOT OK! + } + + try { + unknown({prop: foo}); + } catch(e) { + $('myId').html(e); // We don't flag this for now. + } + + try { + unknown(["bar", foo]); + } catch(e) { + $('myId').html(e); // NOT OK! + } + + function deep(x) { + deep2(x); + } + function deep2(x) { + inner(x); + } + + try { + deep("bar" + foo); + } catch(e) { + $('myId').html(e); // NOT OK! + } + + try { + var tmp = "bar" + foo; + } catch(e) { + $('myId').html(e); // OK + } + + function safe(x) { + var foo = x + "bar"; + } + + try { + safe(foo); + } catch(e) { + $('myId').html(e); // OK + } + + try { + safe.call(null, foo); + } catch(e) { + $('myId').html(e); // OK + } + var myWeirdInner; + try { + myWeirdInner = function (x) { + inner(x); + } + } catch(e) { + $('myId').html(e); // OK + } + try { + myWeirdInner(foo); + } catch(e) { + $('myId').html(e); // NOT OK! + } + + $('myId').html(foo); // Direct leak, reported by other query. + + try { + unknown(foo.match(/foo/)); + } catch(e) { + $('myId').html(e); // NOT OK! + } + + try { + unknown([foo, "bar"]); + } catch(e) { + $('myId').html(e); // NOT OK! + } + + try { + try { + unknown(foo); + } finally { + // nothing + } + } catch(e) { + $('myId').html(e); // NOT OK! + } +}); + +var express = require('express'); + +var app = express(); + +app.get('/user/:id', function(req, res) { + try { + unknown(req.params.id); + } catch(e) { + res.send("Exception: " + e); // NOT OK! + } +}); + + +(function () { + sessionStorage.setItem('exceptionSession', document.location.search); + + try { + unknown(sessionStorage.getItem('exceptionSession')); + } catch(e) { + $('myId').html(e); // NOT OK + } +})(); diff --git a/javascript/ql/test/query-tests/Security/CWE-079/tst.js b/javascript/ql/test/query-tests/Security/CWE-079/tst.js index 780d56e90e1..907f1be9299 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/tst.js @@ -293,6 +293,22 @@ function flowThroughPropertyNames() { $(p); // OK } +function basicExceptions() { + try { + throw location; + } catch(e) { + $("body").append(e); // NOT OK + } + + try { + try { + throw location + } finally {} + } catch(e) { + $("body").append(e); // NOT OK + } +} + function handlebarsSafeString() { return new Handlebars.SafeString(location); // NOT OK! } diff --git a/python/ql/src/Filters/NotGenerated.ql b/python/ql/src/Filters/NotGenerated.ql index 121fc3c7a45..f74432af66c 100644 --- a/python/ql/src/Filters/NotGenerated.ql +++ b/python/ql/src/Filters/NotGenerated.ql @@ -4,6 +4,7 @@ * @kind file-classifier * @id py/not-generated-file-filter */ +import python import external.DefectFilter import semmle.python.filters.GeneratedCode diff --git a/python/ql/src/Filters/NotTest.ql b/python/ql/src/Filters/NotTest.ql index 4d6b0ec5162..c6d76ae1a7b 100644 --- a/python/ql/src/Filters/NotTest.ql +++ b/python/ql/src/Filters/NotTest.ql @@ -4,6 +4,7 @@ * @kind file-classifier * @id py/not-test-file-filter */ +import python import external.DefectFilter import semmle.python.filters.Tests diff --git a/python/ql/src/Security/CWE-798/HardcodedCredentials.ql b/python/ql/src/Security/CWE-798/HardcodedCredentials.ql index 205b477196f..c9fb63ac121 100644 --- a/python/ql/src/Security/CWE-798/HardcodedCredentials.ql +++ b/python/ql/src/Security/CWE-798/HardcodedCredentials.ql @@ -11,6 +11,7 @@ * external/cwe/cwe-798 */ +import python import semmle.python.security.TaintTracking import semmle.python.filters.Tests diff --git a/python/ql/src/external/DuplicateBlock.ql b/python/ql/src/external/DuplicateBlock.ql index 1a892b87900..7bf1bb6da83 100644 --- a/python/ql/src/external/DuplicateBlock.ql +++ b/python/ql/src/external/DuplicateBlock.ql @@ -14,6 +14,7 @@ * @precision medium * @id py/duplicate-block */ +import python import CodeDuplication predicate sorted_by_location(DuplicateBlock x, DuplicateBlock y) { diff --git a/python/ql/src/semmle/python/web/turbogears/Request.qll b/python/ql/src/semmle/python/web/turbogears/Request.qll index 0003c3dc830..dc7ef63ab56 100644 --- a/python/ql/src/semmle/python/web/turbogears/Request.qll +++ b/python/ql/src/semmle/python/web/turbogears/Request.qll @@ -1,21 +1,17 @@ import python import semmle.python.security.strings.Untrusted - import TurboGears private class ValidatedMethodParameter extends Parameter { - ValidatedMethodParameter() { exists(string name, TurboGearsControllerMethod method | method.getArgByName(name) = this and method.getValidationDict().getItem(_).(KeyValuePair).getKey().(StrConst).getText() = name ) } - } class UnvalidatedControllerMethodParameter extends TaintSource { - UnvalidatedControllerMethodParameter() { exists(Parameter p | any(TurboGearsControllerMethod m | not m.getName() = "onerror").getAnArg() = p and @@ -25,9 +21,5 @@ class UnvalidatedControllerMethodParameter extends TaintSource { ) } - override predicate isSourceOf(TaintKind kind) { - kind instanceof UntrustedStringKind - } - + override predicate isSourceOf(TaintKind kind) { kind instanceof UntrustedStringKind } } - diff --git a/python/ql/src/semmle/python/web/turbogears/Response.qll b/python/ql/src/semmle/python/web/turbogears/Response.qll index b8640a6fcdd..ddd3d6f711f 100644 --- a/python/ql/src/semmle/python/web/turbogears/Response.qll +++ b/python/ql/src/semmle/python/web/turbogears/Response.qll @@ -1,14 +1,10 @@ import python - import semmle.python.security.TaintTracking import semmle.python.security.strings.Basic import semmle.python.web.Http import TurboGears - - class ControllerMethodReturnValue extends HttpResponseTaintSink { - ControllerMethodReturnValue() { exists(TurboGearsControllerMethod m | m.getAReturnValueFlowNode() = this and @@ -16,14 +12,10 @@ class ControllerMethodReturnValue extends HttpResponseTaintSink { ) } - override predicate sinks(TaintKind kind) { - kind instanceof StringKind - } - + override predicate sinks(TaintKind kind) { kind instanceof StringKind } } class ControllerMethodTemplatedReturnValue extends HttpResponseTaintSink { - ControllerMethodTemplatedReturnValue() { exists(TurboGearsControllerMethod m | m.getAReturnValueFlowNode() = this and @@ -31,8 +23,5 @@ class ControllerMethodTemplatedReturnValue extends HttpResponseTaintSink { ) } - override predicate sinks(TaintKind kind) { - kind instanceof StringDictKind - } - + override predicate sinks(TaintKind kind) { kind instanceof StringDictKind } } diff --git a/python/ql/src/semmle/python/web/turbogears/TurboGears.qll b/python/ql/src/semmle/python/web/turbogears/TurboGears.qll index c3d417c012e..1cef2f51c84 100644 --- a/python/ql/src/semmle/python/web/turbogears/TurboGears.qll +++ b/python/ql/src/semmle/python/web/turbogears/TurboGears.qll @@ -1,55 +1,33 @@ import python - import semmle.python.security.TaintTracking -private ClassObject theTurboGearsControllerClass() { - result = ModuleObject::named("tg").attr("TGController") -} - - -ClassObject aTurboGearsControllerClass() { - result.getASuperType() = theTurboGearsControllerClass() -} +private ClassValue theTurboGearsControllerClass() { result = Value::named("tg.TGController") } +ClassValue aTurboGearsControllerClass() { result.getABaseType+() = theTurboGearsControllerClass() } class TurboGearsControllerMethod extends Function { - ControlFlowNode decorator; TurboGearsControllerMethod() { - aTurboGearsControllerClass().getPyClass() = this.getScope() and + aTurboGearsControllerClass().getScope() = this.getScope() and decorator = this.getADecorator().getAFlowNode() and /* Is decorated with @expose() or @expose(path) */ ( decorator.(CallNode).getFunction().(NameNode).getId() = "expose" or - decorator.refersTo(_, ModuleObject::named("tg").attr("expose"), _) + decorator.pointsTo().getClass() = Value::named("tg.expose") ) } - private ControlFlowNode templateName() { - result = decorator.(CallNode).getArg(0) - } + private ControlFlowNode templateName() { result = decorator.(CallNode).getArg(0) } - predicate isTemplated() { - exists(templateName()) - } - - string getTemplateName() { - exists(StringObject str | - templateName().refersTo(str) and - result = str.getText() - ) - } + predicate isTemplated() { exists(templateName()) } Dict getValidationDict() { - exists(Call call, Object dict | + exists(Call call, Value dict | call = this.getADecorator() and call.getFunc().(Name).getId() = "validate" and - call.getArg(0).refersTo(dict) and - result = dict.getOrigin() + call.getArg(0).pointsTo(dict, result) ) } - } - diff --git a/python/ql/test/library-tests/web/turbogears/Controller.expected b/python/ql/test/library-tests/web/turbogears/Controller.expected index 9b65e8843fa..549934145a0 100644 --- a/python/ql/test/library-tests/web/turbogears/Controller.expected +++ b/python/ql/test/library-tests/web/turbogears/Controller.expected @@ -2,3 +2,4 @@ | test.py:13:5:13:50 | Function ok_validated | | test.py:18:5:18:57 | Function partially_validated | | test.py:22:5:22:51 | Function not_validated | +| test.py:26:5:26:28 | Function with_template | diff --git a/python/ql/test/library-tests/web/turbogears/Sinks.expected b/python/ql/test/library-tests/web/turbogears/Sinks.expected index 5f2b9b8999f..b528861c340 100644 --- a/python/ql/test/library-tests/web/turbogears/Sinks.expected +++ b/python/ql/test/library-tests/web/turbogears/Sinks.expected @@ -2,3 +2,4 @@ | test.py:14 | BinaryExpr | externally controlled string | | test.py:19 | BinaryExpr | externally controlled string | | test.py:23 | BinaryExpr | externally controlled string | +| test.py:27 | Dict | {externally controlled string} | diff --git a/python/ql/test/library-tests/web/turbogears/test.py b/python/ql/test/library-tests/web/turbogears/test.py index e4347ee5f6f..bae3a460f43 100644 --- a/python/ql/test/library-tests/web/turbogears/test.py +++ b/python/ql/test/library-tests/web/turbogears/test.py @@ -21,3 +21,7 @@ class RootController(TGController): @expose() def not_validated(self, a=None, b=None, *args): return 'Values: %s, %s, %s' % (a, b, args) + + @expose("") + def with_template(self): + return {'template_var': 'foo'}