Merge master into next.

As of 2846d80f1c.

java/ql/src/AlertSuppression.ql

@@ -17,7 +17,8 @@ class SuppressionComment extends Javadoc {
     isEolComment(this) and
     exists(string text | text = getChild(0).getText() |
       // match `lgtm[...]` anywhere in the comment
-      annotation = text.regexpFind("(?i)\\blgtm\\s*\\[[^\\]]*\\]", _, _) or
+      annotation = text.regexpFind("(?i)\\blgtm\\s*\\[[^\\]]*\\]", _, _)
+      or
       // match `lgtm` at the start of the comment and after semicolon
       annotation = text.regexpFind("(?i)(?<=^|;)\\s*lgtm(?!\\B|\\s*\\[)", _, _).trim()
     )

java/ql/src/Frameworks/Spring/Architecture/Refactoring Opportunities/UnusedBean.ql

@@ -156,12 +156,16 @@ class LiveSpringBean extends SpringBean {
       // If the class does not exist for this bean, or the class is not a source bean, then this is
       // likely to be a definition using a library class, in which case we should consider it to be
       // live.
-      not exists(getClass()) or
-      not getClass().fromSource() or
+      not exists(getClass())
+      or
+      not getClass().fromSource()
+      or
       // In alfresco, "webscript" beans should be considered live
-      getBeanParent*().getBeanParentName() = "webscript" or
+      getBeanParent*().getBeanParentName() = "webscript"
+      or
       // A live child bean implies this bean is live
-      exists(LiveSpringBean child | this = child.getBeanParent()) or
+      exists(LiveSpringBean child | this = child.getBeanParent())
+      or
       // Beans constructed by a bean factory are considered live
       exists(SpringBeanFactory beanFactory | this = beanFactory.getAConstructedBean())
     )

java/ql/src/Likely Bugs/Concurrency/NonSynchronizedOverride.ql

@@ -41,7 +41,8 @@ predicate delegatingOverride(Method sub, Method sup) {
     stmt = sub.getBody().(SingletonBlock).getStmt() and
     (
       // ...that is either a delegating call to `sup` (with a possible cast)...
-      delegatingSuperCall(stmt.(ExprStmt).getExpr(), sup) or
+      delegatingSuperCall(stmt.(ExprStmt).getExpr(), sup)
+      or
       // ...or a `return` statement containing such a call.
       delegatingSuperCall(stmt.(ReturnStmt).getResult(), sup)
     )

java/ql/src/Security/CWE/CWE-022/ZipSlip.qhelp

@@ -0,0 +1,70 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Extracting files from a malicious zip archive (or another archive format)
+without validating that the destination file path
+is within the destination directory can cause files outside the destination directory to be
+overwritten, due to the possible presence of directory traversal elements (<code>..</code>) in
+archive paths.</p>
+
+<p>Zip archives contain archive entries representing each file in the archive. These entries
+include a file path for the entry, but these file paths are not restricted and may contain
+unexpected special elements such as the directory traversal element (<code>..</code>). If these
+file paths are used to determine an output file to write the contents of the archive item to, then
+the file may be written to an unexpected location. This can result in sensitive information being
+revealed or deleted, or an attacker being able to influence behavior by modifying unexpected
+files.</p>
+
+<p>For example, if a zip file contains a file entry <code>..\sneaky-file</code>, and the zip file
+is extracted to the directory <code>c:\output</code>, then naively combining the paths would result
+in an output file path of <code>c:\output\..\sneaky-file</code>, which would cause the file to be
+written to <code>c:\sneaky-file</code>.</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that output paths constructed from zip archive entries are validated to prevent writing
+files to unexpected locations.</p>
+
+<p>The recommended way of writing an output file from a zip archive entry is to
+verify that the normalized full path of the output file starts with a prefix that matches the
+destination directory. Path normalization can be done with either
+<code>java.io.File.getCanonicalFile()</code> or <code>java.nio.file.Path.normalize()</code>.
+Prefix checking can be done with <code>String.startsWith(..)</code>, but it is better to use
+<code>java.nio.file.Path.startsWith(..)</code>, as the latter works on complete path segments.
+</p>
+
+<p>Another alternative is to validate archive entries against a whitelist of expected files.</p>
+
+</recommendation>
+<example>
+
+<p>In this example, a file path taken from a zip archive item entry is combined with a
+destination directory. The result is used as the destination file path without verifying that
+the result is within the destination directory. If provided with a zip file containing an archive
+path like <code>..\sneaky-file</code>, then this file would be written outside the destination
+directory.</p>
+
+<sample src="ZipSlipBad.java" />
+
+<p>To fix this vulnerability, we need to verify that the normalized <code>file</code> still has
+<code>destinationDir</code> as its prefix, and throw an exception if this is not the case.</p>
+
+<sample src="ZipSlipGood.java" />
+
+</example>
+<references>
+
+<li>
+Snyk:
+<a href="https://snyk.io/research/zip-slip-vulnerability">Zip Slip Vulnerability</a>.
+</li>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
+</li>
+
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-022/ZipSlip.ql

@@ -0,0 +1,176 @@
+/**
+ * @name Arbitrary file write during archive extraction ("Zip Slip")
+ * @description Extracting files from a malicious archive without validating that the
+ *              destination file path is within the destination directory can cause files outside
+ *              the destination directory to be overwritten.
+ * @kind problem
+ * @id java/zipslip
+ * @problem.severity error
+ * @precision high
+ * @tags security
+ *       external/cwe/cwe-022
+ */
+
+import java
+import semmle.code.java.controlflow.Guards
+import semmle.code.java.dataflow.SSA
+import semmle.code.java.dataflow.TaintTracking
+import DataFlow
+
+/**
+ * A method that returns the name of an archive entry.
+ */
+class ArchiveEntryNameMethod extends Method {
+  ArchiveEntryNameMethod() {
+    exists(RefType archiveEntry |
+      archiveEntry.hasQualifiedName("java.util.zip", "ZipEntry") or
+      archiveEntry.hasQualifiedName("org.apache.commons.compress.archivers", "ArchiveEntry")
+    |
+      this.getDeclaringType().getASupertype*() = archiveEntry and
+      this.hasName("getName")
+    )
+  }
+}
+
+/**
+ * An expression that will be treated as the destination of a write.
+ */
+class WrittenFileName extends Expr {
+  WrittenFileName() {
+    // Constructors that write to their first argument.
+    exists(ConstructorCall ctr | this = ctr.getArgument(0) |
+      exists(Class c | ctr.getConstructor() = c.getAConstructor() |
+        c.hasQualifiedName("java.io", "FileOutputStream") or
+        c.hasQualifiedName("java.io", "RandomAccessFile") or
+        c.hasQualifiedName("java.io", "FileWriter")
+      )
+    )
+    or
+    // Methods that write to their n'th argument
+    exists(MethodAccess call, int n | this = call.getArgument(n) |
+      call.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Files") and
+      (
+        call.getMethod().getName().regexpMatch("new.*Reader|newOutputStream|create.*") and n = 0
+        or
+        call.getMethod().hasName("copy") and n = 1
+        or
+        call.getMethod().hasName("move") and n = 1
+      )
+    )
+  }
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String`,
+ * `File`, and `Path`.
+ */
+predicate filePathStep(ExprNode n1, ExprNode n2) {
+  exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeFile |
+    n1.asExpr() = cc.getAnArgument() and
+    n2.asExpr() = cc
+  )
+  or
+  exists(MethodAccess ma, Method m |
+    ma.getMethod() = m and
+    n1.asExpr() = ma.getQualifier() and
+    n2.asExpr() = ma
+  |
+    m.getDeclaringType() instanceof TypeFile and m.hasName("toPath")
+    or
+    m.getDeclaringType() instanceof TypePath and m.hasName("toAbsolutePath")
+  )
+}
+
+predicate fileTaintStep(ExprNode n1, ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getQualifier() or
+    n1.asExpr() = ma.getAnArgument()
+  |
+    n2.asExpr() = ma and
+    ma.getMethod() = m and
+    m.getDeclaringType() instanceof TypePath and
+    m.hasName("resolve")
+  )
+}
+
+predicate localFileValueStep(Node n1, Node n2) {
+  localFlowStep(n1, n2) or
+  filePathStep(n1, n2)
+}
+
+predicate localFileValueStepPlus(Node n1, Node n2) = fastTC(localFileValueStep/2)(n1, n2)
+
+/**
+ * Holds if `check` is a guard that checks whether `var` is a file path with a
+ * specific prefix when put in canonical form, thus guarding against ZipSlip.
+ */
+predicate validateFilePath(SsaVariable var, Guard check) {
+  // `var.getCanonicalFile().toPath().startsWith(...)`,
+  // `var.getCanonicalPath().startsWith(...)`, or
+  // `var.toPath().normalize().startsWith(...)`
+  exists(MethodAccess normalize, MethodAccess startsWith, Node n1, Node n2, Node n3, Node n4 |
+    n1.asExpr() = var.getAUse() and
+    n2.asExpr() = normalize.getQualifier() and
+    (n1 = n2 or localFileValueStepPlus(n1, n2)) and
+    n3.asExpr() = normalize and
+    n4.asExpr() = startsWith.getQualifier() and
+    (n3 = n4 or localFileValueStepPlus(n3, n4)) and
+    check = startsWith and
+    startsWith.getMethod().hasName("startsWith") and
+    (
+      normalize.getMethod().hasName("getCanonicalFile") or
+      normalize.getMethod().hasName("getCanonicalPath") or
+      normalize.getMethod().hasName("normalize")
+    )
+  )
+}
+
+/**
+ * Holds if `m` validates its `arg`th parameter.
+ */
+predicate validationMethod(Method m, int arg) {
+  exists(Guard check, SsaImplicitInit var, ControlFlowNode exit, ControlFlowNode normexit |
+    validateFilePath(var, check) and
+    var.isParameterDefinition(m.getParameter(arg)) and
+    exit = m and
+    normexit.getANormalSuccessor() = exit and
+    1 = strictcount(ControlFlowNode n | n.getANormalSuccessor() = exit)
+  |
+    check.(ConditionNode).getATrueSuccessor() = exit or
+    check.controls(normexit.getBasicBlock(), true)
+  )
+}
+
+class ZipSlipConfiguration extends TaintTracking::Configuration {
+  ZipSlipConfiguration() { this = "ZipSlip" }
+
+  override predicate isSource(Node source) {
+    source.asExpr().(MethodAccess).getMethod() instanceof ArchiveEntryNameMethod
+  }
+
+  override predicate isSink(Node sink) { sink.asExpr() instanceof WrittenFileName }
+
+  override predicate isAdditionalTaintStep(Node n1, Node n2) {
+    filePathStep(n1, n2) or fileTaintStep(n1, n2)
+  }
+
+  override predicate isSanitizer(Node node) {
+    exists(Guard g, SsaVariable var, RValue varuse | validateFilePath(var, g) |
+      varuse = node.asExpr() and
+      varuse = var.getAUse() and
+      g.controls(varuse.getBasicBlock(), true)
+    )
+    or
+    exists(MethodAccess ma, int pos, RValue rv |
+      validationMethod(ma.getMethod(), pos) and
+      ma.getArgument(pos) = rv and
+      adjacentUseUseSameVar(rv, node.asExpr()) and
+      ma.getBasicBlock().bbDominates(node.asExpr().getBasicBlock())
+    )
+  }
+}
+
+from Node source, Node sink
+where any(ZipSlipConfiguration c).hasFlow(source, sink)
+select source, "Unsanitized archive entry, which may contain '..', is used in a $@.", sink,
+  "file system operation"

java/ql/src/Security/CWE/CWE-022/ZipSlipBad.java

@@ -0,0 +1,5 @@
+void writeZipEntry(ZipEntry entry, File destinationDir) {
+    File file = new File(destinationDir, entry.getName());
+    FileOutputStream fos = new FileOutputStream(file); // BAD
+    // ... write entry to fos ...
+}

java/ql/src/Security/CWE/CWE-022/ZipSlipGood.java

@@ -0,0 +1,7 @@
+void writeZipEntry(ZipEntry entry, File destinationDir) {
+    File file = new File(destinationDir, entry.getName());
+    if (!file.toPath().normalize().startsWith(destinationDir.toPath()))
+        throw new Exception("Bad zip entry");
+    FileOutputStream fos = new FileOutputStream(file); // OK
+    // ... write entry to fos ...
+}

java/ql/src/Security/CWE/CWE-129/BoundingChecks.qll

@@ -58,7 +58,8 @@ predicate lessthanLength(ArrayAccess a) {
 pragma[nomagic]
 private Expr arrayReference(ArrayAccess arrayAccess) {
   // Array is stored in a variable.
-  result = arrayAccess.getArray().(VarAccess).getVariable().getAnAccess() or
+  result = arrayAccess.getArray().(VarAccess).getVariable().getAnAccess()
+  or
   // Array is returned from a method.
   result.(MethodAccess).getMethod() = arrayAccess.getArray().(MethodAccess).getMethod()
 }

java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayIndexCodeSpecified.ql

@@ -32,13 +32,15 @@ where
   not (
     (
       // The input has a lower bound.
-      source.lowerBound() >= 0 or
+      source.lowerBound() >= 0
+      or
       // There is a condition dominating this expression ensuring that the index is >= 0.
       lowerBound(arrayAccess.getIndexExpr()) >= 0
     ) and
     (
       // The input has an upper bound, and the array has a fixed size, and that fixed size is less.
-      source.upperBound() < fixedArraySize(arrayAccess) or
+      source.upperBound() < fixedArraySize(arrayAccess)
+      or
       // There is a condition dominating this expression that ensures the index is less than the length.
       lessthanLength(arrayAccess)
     )

java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql

@@ -18,7 +18,8 @@ class HTTPString extends StringLiteral {
     exists(string s | this.getRepresentedString() = s |
       (
         // Either the literal "http", ...
-        s = "http" or
+        s = "http"
+        or
         // ... or the beginning of a http URL.
         s.matches("http://%")
       ) and

java/ql/src/semmle/code/java/Expr.qll

@@ -1266,7 +1266,8 @@ class VarAccess extends Expr, @varaccess {
    */
   predicate isLocal() {
     // The access has no qualifier, or...
-    not hasQualifier() or
+    not hasQualifier()
+    or
     // the qualifier is either `this` or `A.this`, where `A` is the enclosing type, or
     // the qualifier is either `super` or `A.super`, where `A` is the enclosing type.
     getQualifier().(InstanceAccess).isOwnInstanceAccess()
@@ -1705,7 +1706,8 @@ class Argument extends Expr {
       p.isVarargs() and
       ptyp = p.getType() and
       (
-        hasSubtype*(ptyp, typ) or
+        hasSubtype*(ptyp, typ)
+        or
         // If the types don't match then we'll guess based on whether there are type variables involved.
         hasInstantiation(ptyp.(Array).getComponentType())
       )

java/ql/src/semmle/code/java/JDK.qll

@@ -133,8 +133,8 @@ class TypeObjectOutputStream extends RefType {
 /** The class `java.nio.file.Paths`. */
 class TypePaths extends Class { TypePaths() { this.hasQualifiedName("java.nio.file", "Paths") } }
 
-/** The class `java.nio.file.Path`. */
-class TypePath extends Class { TypePath() { this.hasQualifiedName("java.nio.file", "Path") } }
+/** The type `java.nio.file.Path`. */
+class TypePath extends RefType { TypePath() { this.hasQualifiedName("java.nio.file", "Path") } }
 
 /** The class `java.nio.file.FileSystem`. */
 class TypeFileSystem extends Class {

java/ql/src/semmle/code/java/Member.qll

@@ -370,7 +370,8 @@ class Method extends Callable, @method {
   }
 
   override predicate isStrictfp() {
-    Callable.super.isStrictfp() or
+    Callable.super.isStrictfp()
+    or
     // JLS 8.1.1.3, JLS 9.1.1.2
     getDeclaringType().isStrictfp()
   }
@@ -575,21 +576,24 @@ class Field extends Member, ExprParent, @field, Variable {
   predicate isSourceDeclaration() { this.getSourceDeclaration() = this }
 
   override predicate isPublic() {
-    Member.super.isPublic() or
+    Member.super.isPublic()
+    or
     // JLS 9.3: Every field declaration in the body of an interface is
     // implicitly public, static, and final
     getDeclaringType() instanceof Interface
   }
 
   override predicate isStatic() {
-    Member.super.isStatic() or
+    Member.super.isStatic()
+    or
     // JLS 9.3: Every field declaration in the body of an interface is
     // implicitly public, static, and final
     this.getDeclaringType() instanceof Interface
   }
 
   override predicate isFinal() {
-    Member.super.isFinal() or
+    Member.super.isFinal()
+    or
     // JLS 9.3: Every field declaration in the body of an interface is
     // implicitly public, static, and final
     this.getDeclaringType() instanceof Interface

java/ql/src/semmle/code/java/Reflection.qll

@@ -271,9 +271,11 @@ class NewInstance extends MethodAccess {
     not result instanceof TypeVariable and
     (
       // If this is called on a `Class<T>` instance, return the inferred type `T`.
-      result = inferClassParameterType(getQualifier()) or
+      result = inferClassParameterType(getQualifier())
+      or
       // If this is called on a `Constructor<T>` instance, return the inferred type `T`.
-      result = inferConstructorParameterType(getQualifier()) or
+      result = inferConstructorParameterType(getQualifier())
+      or
       // If the result of this is cast to a particular type, then use that type.
       result = getCastInferredConstructedTypes()
     )

java/ql/src/semmle/code/java/Type.qll

@@ -216,16 +216,19 @@ private predicate typeArgumentContainsAux1(RefType s, RefType t, int n) {
   |
     exists(RefType tUpperBound | tUpperBound = t.(Wildcard).getUpperBound().getType() |
       // ? extends T <= ? extends S if T <: S
-      hasSubtypeStar0(s.(Wildcard).getUpperBound().getType(), tUpperBound) or
+      hasSubtypeStar0(s.(Wildcard).getUpperBound().getType(), tUpperBound)
+      or
       // ? extends T <= ?
       s.(Wildcard).isUnconstrained()
     )
     or
     exists(RefType tLowerBound | tLowerBound = t.(Wildcard).getLowerBound().getType() |
       // ? super T <= ? super S if s <: T
-      hasSubtypeStar0(tLowerBound, s.(Wildcard).getLowerBound().getType()) or
+      hasSubtypeStar0(tLowerBound, s.(Wildcard).getLowerBound().getType())
+      or
       // ? super T <= ?
-      s.(Wildcard).isUnconstrained() or
+      s.(Wildcard).isUnconstrained()
+      or
       // ? super T <= ? extends Object
       wildcardExtendsObject(s)
     )
@@ -736,13 +739,15 @@ class NestedType extends RefType {
   }
 
   override predicate isPublic() {
-    super.isPublic() or
+    super.isPublic()
+    or
     // JLS 9.5: A member type declaration in an interface is implicitly public and static
     exists(Interface i | this = i.getAMember())
   }
 
   override predicate isStrictfp() {
-    super.isStrictfp() or
+    super.isStrictfp()
+    or
     // JLS 8.1.1.3, JLS 9.1.1.2
     getEnclosingType().isStrictfp()
   }
@@ -762,11 +767,14 @@ class NestedType extends RefType {
    * section 8.9 (Enums) and section 9.5 (Member Type Declarations).
    */
   override predicate isStatic() {
-    super.isStatic() or
+    super.isStatic()
+    or
     // JLS 8.5.1: A member interface is implicitly static.
-    this instanceof Interface or
+    this instanceof Interface
+    or
     // JLS 8.9: A nested enum type is implicitly static.
-    this instanceof EnumType or
+    this instanceof EnumType
+    or
     // JLS 9.5: A member type declaration in an interface is implicitly public and static
     exists(Interface i | this = i.getAMember())
   }

java/ql/src/semmle/code/java/UnitTests.qll

@@ -148,7 +148,8 @@ class TestNGTestMethod extends Method {
             .getRepresentedString()
     |
       // Either the data provider should be on the current class, or a supertype
-      getDeclaringType().getAnAncestor() = result.getDeclaringType() or
+      getDeclaringType().getAnAncestor() = result.getDeclaringType()
+      or
       // Or the data provider class should be declared
       result.getDeclaringType() = testAnnotation
             .getValue("dataProviderClass")

java/ql/src/semmle/code/java/deadcode/DeadField.qll

@@ -43,11 +43,14 @@ class LiveField extends SourceField {
         a.getValue(_) = access.getParent*()
       |
         // The annotated element is a live callable.
-        isLive(a.getAnnotatedElement()) or
+        isLive(a.getAnnotatedElement())
+        or
         // The annotated element is in a live callable.
-        isLive(a.getAnnotatedElement().(LocalVariableDecl).getEnclosingCallable()) or
+        isLive(a.getAnnotatedElement().(LocalVariableDecl).getEnclosingCallable())
+        or
         // The annotated element is a live field.
-        a.getAnnotatedElement() instanceof LiveField or
+        a.getAnnotatedElement() instanceof LiveField
+        or
         // The annotated element is a live source class or interface.
         // Note: We ignore annotation values on library classes, because they should only refer to
         // fields in library classes, not `fromSource()` fields.

java/ql/src/semmle/code/java/frameworks/Camel.qll

@@ -58,7 +58,8 @@ class CamelTargetClass extends Class {
   CamelTargetClass() {
     exists(SpringCamelXMLBeanRef camelXMLBeanRef |
       // A target may be defined by referencing an existing Spring Bean.
-      this = camelXMLBeanRef.getRefBean().getClass() or
+      this = camelXMLBeanRef.getRefBean().getClass()
+      or
       // A target may be defined by referencing a class, which Apache Camel will create into a bean.
       this = camelXMLBeanRef.getBeanType()
     )

java/ql/src/semmle/code/java/frameworks/Mockito.qll

@@ -53,7 +53,8 @@ class MockitoInitedTest extends Class {
   MockitoInitedTest() {
     // Tests run with the Mockito runner.
     exists(RunWithAnnotation a | a = this.getAnAncestor().getAnAnnotation() |
-      a.getRunner().(RefType).hasQualifiedName("org.mockito.runners", "MockitoJUnitRunner") or
+      a.getRunner().(RefType).hasQualifiedName("org.mockito.runners", "MockitoJUnitRunner")
+      or
       // Deprecated style.
       a.getRunner().(RefType).hasQualifiedName("org.mockito.runners", "MockitoJUnit44Runner")
     )
@@ -124,7 +125,8 @@ class MockitoAnnotatedField extends Field {
  */
 class MockitoMockedField extends MockitoAnnotatedField {
   MockitoMockedField() {
-    hasAnnotation("org.mockito", "Mock") or
+    hasAnnotation("org.mockito", "Mock")
+    or
     // Deprecated style.
     hasAnnotation("org.mockito", "MockitoAnnotations$Mock")
   }

java/ql/src/semmle/code/java/frameworks/javaee/JavaServerFaces.qll

@@ -61,7 +61,8 @@ class FacesComponent extends Class {
       // Must be registered using either an annotation
       exists(FacesComponentAnnotation componentAnnotation |
         this = componentAnnotation.getFacesComponentClass()
-      ) or
+      )
+      or
       // Or in an XML file
       exists(FacesConfigComponentClass componentClassXML |
         this = componentClassXML.getFacesComponentClass()

java/ql/src/semmle/code/java/frameworks/javaee/ejb/EJB.qll

@@ -153,9 +153,11 @@ class StatelessSessionEJB extends SessionEJB {
 class MessageDrivenBean extends EJB {
   MessageDrivenBean() {
     // Subtype of `javax.ejb.MessageBean`.
-    this instanceof MessageBean or
+    this instanceof MessageBean
+    or
     // EJB annotations.
-    this.getAnAnnotation().getType().hasName("MessageDriven") or
+    this.getAnAnnotation().getType().hasName("MessageDriven")
+    or
     // XML deployment descriptor.
     exists(EjbJarXMLFile f |
       this.getQualifiedName() = f
@@ -173,7 +175,8 @@ class MessageDrivenBean extends EJB {
 class EntityEJB extends EJB {
   EntityEJB() {
     // Subtype of `javax.ejb.EntityBean`.
-    this instanceof EntityBean or
+    this instanceof EntityBean
+    or
     // XML deployment descriptor.
     exists(EjbJarXMLFile f |
       this.getQualifiedName() = f
@@ -294,7 +297,8 @@ class XmlSpecifiedBusinessInterface extends BusinessInterface {
 class AnnotatedBusinessInterface extends BusinessInterface {
   AnnotatedBusinessInterface() {
     // An interface annotated as `@Remote` or `@Local`.
-    this.getAnAnnotation() instanceof BusinessInterfaceAnnotation or
+    this.getAnAnnotation() instanceof BusinessInterfaceAnnotation
+    or
     // An interface named within a `@Local` or `@Remote` annotation of another type.
     exists(BusinessInterfaceAnnotation a | a.getANamedType() = this)
   }

java/ql/src/semmle/code/java/frameworks/spring/SpringComponentScan.qll

@@ -96,7 +96,8 @@ class SpringBasePackage extends string {
 class SpringComponentAnnotation extends AnnotationType {
   SpringComponentAnnotation() {
     // Component used directly as an annotation.
-    hasQualifiedName("org.springframework.stereotype", "Component") or
+    hasQualifiedName("org.springframework.stereotype", "Component")
+    or
     // Component can be used as a meta-annotation on other annotation types.
     getAnAnnotation().getType() instanceof SpringComponentAnnotation
   }

java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll

@@ -6,7 +6,8 @@ import java
 class SpringControllerAnnotation extends AnnotationType {
   SpringControllerAnnotation() {
     // `@Controller` used directly as an annotation.
-    hasQualifiedName("org.springframework.stereotype", "Controller") or
+    hasQualifiedName("org.springframework.stereotype", "Controller")
+    or
     // `@Controller` can be used as a meta-annotation on other annotation types.
     getAnAnnotation().getType() instanceof SpringControllerAnnotation
   }

java/ql/src/semmle/code/java/security/FileWritable.qll

@@ -111,7 +111,8 @@ private predicate fileSetWorldWritable(VarAccess fileAccess, Expr setWorldWritab
     setPosixPerms.getMethod().hasName("setPosixFilePermissions") and
     setPosixPerms.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Files") and
     (
-      fileAccess = setPosixPerms.getArgument(0) or
+      fileAccess = setPosixPerms.getArgument(0)
+      or
       // The argument was a file that has been converted to a path.
       fileAccess = getFileForPathConversion(setPosixPerms.getArgument(0))
     )

java/ql/src/semmle/code/xml/MavenPom.qll

@@ -263,7 +263,8 @@ class PomDependency extends Dependency {
       source.getADependency() = this and
       // Consider dependencies that can be used at compile time.
       (
-        getScope() = "compile" or
+        getScope() = "compile"
+        or
         // Provided dependencies are like compile time dependencies except (a) they are not packaged
         // when creating the jar and (b) they are not transitive.
         getScope() = "provided"

java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected

@@ -0,0 +1,3 @@
+| ZipTest.java:7:19:7:33 | getName(...) | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:9:48:9:51 | file | file system operation |
+| ZipTest.java:7:19:7:33 | getName(...) | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:10:49:10:52 | file | file system operation |
+| ZipTest.java:7:19:7:33 | getName(...) | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:11:36:11:39 | file | file system operation |

java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-022/ZipSlip.ql

java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.java

@@ -0,0 +1,54 @@
+import java.io.*;
+import java.nio.file.*;
+import java.util.zip.*;
+
+public class ZipTest {
+  public void m1(ZipEntry entry, File dir) {
+    String name = entry.getName();
+    File file = new File(dir, name);
+    FileOutputStream os = new FileOutputStream(file); // ZipSlip
+    RandomAccessFile raf = new RandomAccessFile(file, "rw"); // ZipSlip
+    FileWriter fw = new FileWriter(file); // ZipSlip
+  }
+
+  public void m2(ZipEntry entry, File dir) {
+    String name = entry.getName();
+    File file = new File(dir, name);
+    File canFile = file.getCanonicalFile();
+    String canDir = dir.getCanonicalPath();
+    if (!canFile.toPath().startsWith(canDir))
+      throw new Exception();
+    FileOutputStream os = new FileOutputStream(file); // OK
+  }
+
+  public void m3(ZipEntry entry, File dir) {
+    String name = entry.getName();
+    File file = new File(dir, name);
+    if (!file.toPath().normalize().startsWith(dir.toPath()))
+      throw new Exception();
+    FileOutputStream os = new FileOutputStream(file); // OK
+  }
+
+  private void validate(File tgtdir, File file) {
+    File canFile = file.getCanonicalFile();
+    if (!canFile.toPath().startsWith(tgtdir.toPath()))
+      throw new Exception();
+  }
+
+  public void m4(ZipEntry entry, File dir) {
+    String name = entry.getName();
+    File file = new File(dir, name);
+    validate(dir, file);
+    FileOutputStream os = new FileOutputStream(file); // OK
+  }
+
+  public void m5(ZipEntry entry, File dir) {
+    String name = entry.getName();
+    File file = new File(dir, name);
+    Path absfile = file.toPath().toAbsolutePath().normalize();
+    Path absdir = dir.toPath().toAbsolutePath().normalize();
+    if (!absfile.startsWith(absdir))
+      throw new Exception();
+    FileOutputStream os = new FileOutputStream(file); // OK
+  }
+}